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

[merch-redmine@jeremyevans.net]

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


Eregon (Benoit Daloze) wrote in #note-4:
> jeremyevans0 (Jeremy Evans) wrote:
> > Not initializing instance variables to nil can be much better for performance
> 
> Why is that? Because just writing extra instance variables in `initialize` is much slower in MRI?
> It's already an allocation path so it's not that fast anyway (unless escape analyzed, and then writing instance variables should be pretty much free).

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)

The difference is substantial.  With 6 instance variables, it's over twice as fast to skip initializing them to nil.

```ruby
require 'benchmark/ips'

# initialized
class A
  def initialize
    @c = @d = @e = @f = @g = @h = nil
  end
  def b
    @c || @d || @e || @f || @g || @h
  end
end

# not initialized
class B
  def initialize
    # nothing
  end
  def b
    @c || @d || @e || @f || @g || @h
  end
end

eval "def a0; #{"A.new;"*1000} end"
eval "def b0; #{"B.new;"*1000} end"

Benchmark.ips do |x|
  x.warmup = 0
  x.report('initialized'){a0}
  x.report('uninitialized'){b0}
  x.compare!
end
```

Results with Ruby 2.7.1:

```
         initialized    931.400  (_ 6.3%) i/s -      4.628k in   4.991632s
       uninitialized      2.016k (_10.3%) i/s -      9.923k in   4.987151s

Comparison:
       uninitialized:     2016.2 i/s
         initialized:      931.4 i/s - 2.16x  (_ 0.00) slower

```

> It seems a very narrow use-case to me, and extremely MRI-specific.

Results with JRuby 9.2.12 still show a significant speedup, though it is not as dramatic:

```
         initialized      5.865k (_ 9.3%) i/s -     26.736k
       uninitialized      8.712k (_ 4.1%) i/s -     41.098k

Comparison:
       uninitialized:     8712.2 i/s
         initialized:     5865.5 i/s - 1.49x slower

```

> In fact I wouldn't be surprised if on other Ruby implementations initializing led to better performance (e.g., no need to grow the ivar storage dynamically later, or change the shape/hidden class).

Do the above results on JRuby suprise you?

> I would much prefer a Warning category for this.

The issue with using a Warning category is that the change is made globally for all objects/instance variables and modules/methods.  The advantage of my approach is that it allows for a granular approach, where each gem can suppress these verbose warnings as needed for their objects/modules, without turning off the advantages of these verbose warnings for the users of the gems (where the verbose warnings may be helpful in their own code).

> Or probably your `warning` gem could be used to suppress those conveniently?

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

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

I considered the complexity cost of adding the feature before I proposed it, and I think the benefits of this feature outweigh the cost.

> Also callbacks seems a very odd way to handle this, if we really want methods to suppress warnings for specific methods/ivars, let's do it proactively like:
> ```ruby
> ignore_warning_method_redefinition :foo
> def foo
>   ...
> end
> 
> ignore_warning_uninitialized_ivar :@foo
> ```

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.

> Also those warnings only happen in verbose mode, which typically shouldn't be used in production.

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.

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

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