Issue #17055 has been updated by Eregon (Benoit Daloze).


jeremyevans0 (Jeremy Evans) wrote in #note-6:
> Yes.  When you initialize an instance variable to nil, it slows things do=
wn, and there is no benefit because the trying to access an uninitialized i=
nstance variable returns nil anyway (plus warning in verbose mode)

And when you don't initialize, reads might become slower, because they have=
 to check if warnings are enabled, and it might create polymorphism (see th=
e end of my reply).
It's a two-sides blade.
I'd argue allocating without ever accessing or storing objects is an unreal=
istic benchmark.

Can you show a significant overhead on a realistic benchmark? (some Sequel =
real-world usage maybe?)

Here is the result on `truffleruby 20.3.0-dev-b7a9b466` with your microbenc=
hmark and 10 instead of 1000.
It shows the benchmark is bad mostly (i.e., it optimizes away and does noth=
ing useful).
But also that apparently there seems to be no significant difference.

```
truffleruby bench_ivar_set.rb =

Warming up --------------------------------------
         initialized   423.378M i/100ms
       uninitialized   191.211M i/100ms
         initialized   209.219M i/100ms
       uninitialized    60.855M i/100ms
Calculating -------------------------------------
         initialized      2.093B (=B1 0.2%) i/s -     10.670B in   5.098008s
       uninitialized      2.093B (=B1 0.4%) i/s -     10.467B in   5.000214s
         initialized      2.019B (=B114.7%) i/s -      9.624B in   5.036843s
       uninitialized      2.122B (=B1 2.3%) i/s -     10.650B in   5.021645s

Comparison:
       uninitialized: 2122273181.8 i/s
         initialized: 2018820267.6 i/s - same-ish: difference falls within =
error
```
Here is the file used: [bench_ivar_set.rb](https://gist.github.com/eregon/2=
82070e6f6686740a0c8e41243fb598b).

> The warning gem has always supported this.  It was the primary reason I w=
orked on adding the warning support to Ruby 2.4.

Isn't it good enough of a solution already? (can easily narrow by file & iv=
ar name)
Adding a dependency on it doesn't seem a big deal and even deserved if you =
want such fine control over warnings and purposefully suppress warnings.
One can also `prepend` a module to `Warning` to handle this without an extr=
a dependency.

> > Calling more methods when doing warnings is adding more boilerplate, co=
mplexity and edge cases to these code paths.
> =

> In the uninitialized instance variable case, I was actually able to reduc=
e three separate code paths for issuing the warning to a single code path, =
plus I found a case that should produce a warning that did not and fixed th=
at.  So overall complexity could be lower for the uninitialized variable ca=
se, at least for MRI.

There might be other good things in the PR.
It's irrelevant to the general added complexity of a new callback.
Imagine if we wanted to add such methods for more warnings, I think that wo=
uld quickly become a mess.

> The proactive approach is substantially less flexible (e.g. can't use a r=
egexp), and would require storing the values and a significantly more compl=
ex implementation.  Considering you just complained about the complexity of=
 my patch, I find it strange that you would propose an approach that would =
definitely require even greater internal complexity.

I think well-designed and Ruby-like API is more important than complexity h=
ere.
But yes, both add complexity that IMHO is unneeded.

> The advantage of this approach is that it allows you to get the maximum p=
ossible performance in production, while suppressing unnecessary warnings i=
n development or testing when you may run with verbose warnings.  Without t=
his, you either need to give up some production performance, or you have to=
 require the user install a separate library to filter out the verbose warn=
ings.

My view is it encourages less readable code by trying to squeak a tiny bit =
of performance when running on current CRuby.

And it can make reads of uninitialized variables slower, if they later beco=
me initialized (simply because the inline cache needs to care about 2 cases=
 instead of 1).
Here is a benchmark attempting to illustrate that, it's ~3x slower for unin=
it+init reads on TruffleRuby due to the created polymorphism, and ~1.16x sl=
ower for uninitialized reads on CRuby 2.6.6:
https://gist.github.com/eregon/561c09e0156a5530f5a100d3e2351c4b
```
ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-linux]
  init + uninit read: 14989101.7 i/s
    initialized read: 14921107.5 i/s - same-ish: difference falls within er=
ror
  uninitialized read: 12885730.1 i/s - 1.16x  (=B1 0.00) slower

truffleruby 20.3.0-dev-b7a9b466, like ruby 2.6.6, GraalVM CE Native [x86_64=
-linux]
Comparison:
  uninitialized read: 704396153.8 i/s
    initialized read: 700673745.0 i/s - same-ish: difference falls within e=
rror
  init + uninit read: 214761238.7 i/s - 3.28x  (=B1 0.00) slower
```

----------------------------------------
Feature #17055: Allow suppressing uninitialized instance variable and metho=
d redefined verbose mode warnings
https://bugs.ruby-lang.org/issues/17055#change-86894

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
----------------------------------------
These two verbose mode warnings are both fairly common and have good reason=
s why you would not want to warn about them in specific cases.  Not initial=
izing instance variables to nil can be much better for performance, and red=
efining methods without removing the method first is the only safe approach=
 in multi-threaded code.

There are reasons that you may want to issue verbose warnings by default in=
 these cases.  For uninitialized instance variables, it helps catch typos. =
For method redefinition, it could alert you that a method already exists wh=
en you didn't expect it to, such as when a file is loaded multiple times wh=
en it should only be loaded once.

I propose we keep the default behavior the same, but offer the ability to o=
pt-out of these warnings by defining methods.  For uninitialized instance v=
ariables in verbose mode, I propose we call `expected_uninitialized_instanc=
e_variable?(iv)` on the object.  If this method doesn't exist or returns fa=
lse/nil, we issue the warning.  If the method exists and returns true, we s=
uppress the warning.  Similarly, for redefined methods, we call `expected_r=
edefined_method?(method_name)` on the class or module.  If the method doesn=
't exist or returns false/nil, we issue the warning.  If the method exists =
and returns true, we suppress the warning.

This approach allows high performance code (uninitialized instance variable=
s) and safe code (redefining methods without removing) to work without verb=
ose mode warnings.

I have implemented this support in a pull request: https://github.com/ruby/=
ruby/pull/3371



-- =

https://bugs.ruby-lang.org/

Unsubscribe: <mailto:ruby-core-request / ruby-lang.org?subject=3Dunsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>