[ruby-core:99453] [Ruby master Feature#17055] Allow suppressing uninitialized instance variable and method redefined verbose mode warnings

[eregontp@gmail.com]

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


jeremyevans0 (Jeremy Evans) wrote in #note-11:
> The last time I did testing on this in Sequel, the performance decrease from initializing instance variables to nil was around 5% for Sequel::Model instance 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 decrease) just to avoid verbose warnings.

Could you reproduce that again?
I would like a realistic benchmark, so unless users frequently create Sequel::Model instances themselves, I guess the normal case is the data comes from somewhere (the database, some file, etc).
In such a case I would think the overhead is not measurable.

> Interesting.  If it is not too much trouble, what are the results with 1000 instead of 10, if I may ask?

Duplicating code like that is IMHO a misleading way to benchmark (it makes it all too easy to misinterpret results) and obviously unrepresentative of real code.
I see it used for implementations like CRuby for which block call is a large overhead, but IMHO that should be a reminder there is a point where it's too small to measure on its own, and there is always code around for realistic cases.
Also the Ruby implementation might see that the result is unused due to this repetition (TruffleRuby did above).
I used 10 instead of 1000 since Charles did the same. But it should be just 1, really, and the benchmark should use the result value.

Anyway, this is the result for 1000, not everything is inlined since the method is so large, yet there seems to be no meaningful difference:
```
Calculating -------------------------------------
         initialized      4.257k (±41.1%) i/s -     14.136k in   5.064145s
       uninitialized      6.307k (±62.3%) i/s -     17.864k in   5.002209s
         initialized     12.782k (±23.8%) i/s -     53.010k in   5.013438s
       uninitialized     13.123k (±25.5%) i/s -     53.012k in   5.002126s

Comparison:
       uninitialized:    13123.0 i/s
         initialized:    12782.2 i/s - same-ish: difference falls within error
```

> Alternatively, if you modify the benchmark to access each instance variable once, what the the results of that?

It still optimizes away, because the allocation is never needed:
https://gist.github.com/eregon/f279901e3df450d7a1970b76b9653c71
```
Calculating -------------------------------------
         initialized      1.614B (±43.2%) i/s -      6.184B in   5.236291s
       uninitialized      1.816B (±33.1%) i/s -      7.259B in   5.078576s
         initialized      1.879B (±28.2%) i/s -      7.890B in   5.043604s
       uninitialized      1.811B (±32.4%) i/s -      7.259B in   5.007219s

Comparison:
         initialized: 1878613372.9 i/s
       uninitialized: 1811213500.0 i/s - same-ish: difference falls within error

```

We need a realistic benchmark, then we'll see if there is an observable difference or not and how much it matters in practice.

> I'd test myself, but it appears TruffleRuby is not yet ported to the operating systems I use (OpenBSD/Windows).

Feel free to open an issue about building TruffleRuby on OpenBSD. Maybe it's not so hard (but would need to build most things from source).
Alternatively I guess a more convenient way is to use Docker (but it might affect performance).

> In my opinion, a ruby gem should never modify the behavior (e.g. add/override 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/development in verbose mode have to filter the warnings themselves.  With the feature I am proposing, each library has control over their own code.

My view of that is `Warning.warn` is designed explicitly for this, so that libraries can compose and filters warnings as needed.
Even better if the warning can be avoided in the first place though IMHO.

So, for this case, how about using `def foo; @foo ||= nil; end`? That would avoid the warning and still let you assign the `@ivar` lazily, no?

I have another concern: hiding warnings like this will silently cause much worse performance in `$VERBOSE` mode.
Admittedly this would only happen when `$VERBOSE` is true, but performance might still matter to some degree in such cases, e.g., when running tests (tests frameworks often enable $VERBOSE, and people still want their tests to run reasonably fast).

In such a case, on you branch it's 6.6 times slower for reads:
https://gist.github.com/eregon/32f4119b7796ec7a6243c68990949597
```
Comparison:
         initialized: 14754531.3 i/s
       uninitialized:  2227414.1 i/s - 6.62x  (± 0.00) slower
```

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

Regarding method redefinition I think the `alias`/`alias_method` trick is reasonable, and has the advantage to already work on all Ruby versions.
It's not obvious though so I wonder if we could have something clearer like `suppress_redefinition_warning instance_method(:my_method)`, but anyway method redefinition without caring of the previous definition should be very rare so `alias` seems a fine enough workaround.

I think in general it would be useful to have a thread-safe way to suppress warnings for a block of code, and that could be used in this case and many other cases (e.g., 194 `suppress_warning` in ruby/ruby).
Unfortunately, changing `$VERBOSE` is not thread-safe if there are multiple Threads.
That IMHO would be a valuable addition, and be useful not just for this method-redefinition-ignoring-previous case but in many other cases too.
(it wouldn't be a good solution for the uninitialized ivar case, there I think it's really best to avoid the warning in the first place)

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

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
----------------------------------------
These two verbose mode warnings are both fairly common and have good reasons why you would not want to warn about them in specific cases.  Not initializing instance variables to nil can be much better for performance, and redefining 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 when you didn't expect it to, such as when a file is loaded multiple times when it should only be loaded once.

I propose we keep the default behavior the same, but offer the ability to opt-out of these warnings by defining methods.  For uninitialized instance variables in verbose mode, I propose we call `expected_uninitialized_instance_variable?(iv)` on the object.  If this method doesn't exist or returns false/nil, we issue the warning.  If the method exists and returns true, we suppress the warning.  Similarly, for redefined methods, we call `expected_redefined_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 variables) and safe code (redefining methods without removing) to work without verbose 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=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>