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

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 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 have to check if warnings are enabled, and it might create polymorphism (see the end of my reply).
It's a two-sides blade.
I'd argue allocating without ever accessing or storing objects is an unrealistic 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 microbenchmark and 10 instead of 1000.
It shows the benchmark is bad mostly (i.e., it optimizes away and does nothing 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 (0.2%) i/s -     10.670B in   5.098008s
       uninitialized      2.093B (ア 0.4%) i/s -     10.467B in   5.000214s
         initialized      2.019B (ア14.7%) i/s -      9.624B in   5.036843s
       uninitialized      2.122B (ア 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/282070e6f6686740a0c8e41243fb598b).

> 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 you want such fine control over warnings and purposefully suppress warnings.
One can also `prepend` a module to `Warning` to handle this without an extra dependency.

> > 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 reduce 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.

> The proactive approach is substantially less flexible (e.g. can't use a regexp), and would require storing the values and a significantly more complex 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 here.
But yes, both add complexity that IMHO is unneeded.

> 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 warnings.

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 become 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 uninit+init reads on TruffleRuby due to the created polymorphism, and ~1.16x slower 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 error
  uninitialized read: 12885730.1 i/s - 1.16x  (ア 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 error
  init + uninit read: 214761238.7 i/s - 3.28x  (ア 0.00) slower
```

----------------------------------------
Feature #17055: Allow suppressing uninitialized instance variable and method 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 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:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>