Issue #17055 has been updated by jeremyevans0 (Jeremy Evans).


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

> And when you don't initialize, reads might become slower, because they ha=
ve to check if warnings are enabled, and it might create polymorphism (see =
the end of my reply).
> It's a two-sides blade.

This is correct.  Whether or not to initialize instance variables depends o=
n the object.  For long lived objects (classes, constants), it definitely m=
akes sense.  For ephemeral objects, it can hurt performance.

> I'd argue allocating without ever accessing or storing objects is an unre=
alistic benchmark.
> =

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

The last time I did testing on this in Sequel, the performance decrease fro=
m initializing instance variables to nil was around 5% for Sequel::Model in=
stance creation depending on the plugins in use. One of the reasons it was =
around 5% was that many plugins had to override initialization methods just=
 to set an instance variable and call `super`.  5% may not sound like a lot=
, but I can't justify a 5% performance decrease (or even a 1% performance d=
ecrease) just to avoid verbose warnings.

> Here is the result on `truffleruby 20.3.0-dev-b7a9b466` with your microbe=
nchmark and 10 instead of 1000.
> It shows the benchmark is bad mostly (i.e., it optimizes away and does no=
thing useful).
> =

> ```
> 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.0980=
08s
>        uninitialized      2.093B (=B1 0.4%) i/s -     10.467B in   5.0002=
14s
>          initialized      2.019B (=B114.7%) i/s -      9.624B in   5.0368=
43s
>        uninitialized      2.122B (=B1 2.3%) i/s -     10.650B in   5.0216=
45s
> =

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

Interesting.  If it is not too much trouble, what are the results with 1000=
 instead of 10, if I may ask?  Alternatively, if you modify the benchmark t=
o access each instance variable once, what the the results of that?  I'd te=
st myself, but it appears TruffleRuby is not yet ported to the operating sy=
stems I use (OpenBSD/Windows).

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

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

In my opinion, a ruby gem should never modify the behavior (e.g. add/overri=
de methods) of core classes, unless that is the purpose of the library.  As=
 such, modifying the Warning class is not something I would consider doing =
by default, and therefore people that use Sequel and want to run tests/deve=
lopment in verbose mode have to filter the warnings themselves.  With the f=
eature I am proposing, each library has control over their own code.

I believe verbose warning for uninitialized instance variables in code you =
have no control over is actively harmful to the user, because it can make i=
t more difficult to find warnings in code you do have control over.

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

> > In the uninitialized instance variable case, I was actually able to red=
uce 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 =
that.  So overall complexity could be lower for the uninitialized variable =
case, 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 =
would quickly become a mess.

Certainly this approach is not appropriate for all warnings.  However, in m=
y 15+ years of Ruby experience, I've seen that these two verbose warnings a=
re by far the most common, and both of them have valid reasons for using th=
e behavior that produces the verbose warnings (performance and safety).

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

> I think well-designed and Ruby-like API is more important than complexity=
 here.

I fail to see how this is not a Ruby-like API.  It's similar to other callb=
acks, such as `method_added`.  It's also the simplest thing I can think of =
that would work.  Additionally, as @byroot mentioned, it may be possible to=
 use this approach with did_you_mean for even more helpful warnings.

> > The advantage of this approach is that it allows you to get the maximum=
 possible performance in production, while suppressing unnecessary warnings=
 in development or testing when you may run with verbose warnings.  Without=
 this, 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 wa=
rnings.
> =

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

This is only half about performance.  You haven't mentioned anything about =
the method redefinition warning yet.  Can you provide your thoughts on that?

> In other words, lazily initializing @ivars causes reads to need some bran=
ching because they need to handle both the initialized and uninitialized ca=
ses.
> So @ivars reads can no longer be straight-line code, which can impact per=
formance as shown above.

As I stated above, whether you want to initialize instance variables lazily=
 or eagerly for performance depends on the object in question.  Not all sit=
uations are the same.  It is faster in some situations and slower in others.

> Also if the natural default is not `nil` but say `0` then `@foo =3D 0` in=
 `initialize` is useful information (notably it gives the type), and it can=
 be a simple `attr_reader :foo` to read it instead of the convoluted:
> ```ruby
> def foo
>   @foo || 0
> end
> ```

I think we can all agree that there are cases where eagerly initializing th=
e object can improve performance.  This says nothing about the cases where =
eagerly initializing the object hurts performance.

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

* 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>