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

[tenderlove@ruby-lang.org]

Issue #17055 has been updated by tenderlovemaking (Aaron Patterson).


jeremyevans0 (Jeremy Evans) wrote in #note-15:
> tenderlovemaking (Aaron Patterson) wrote in #note-14:
> > jeremyevans0 (Jeremy Evans) wrote in #note-13:
> > > As you'll see by the benchmark, the reason the performance difference is much different than you would expect is that Model loads from the database in Sequel run through `Sequel::Model.call` (class method, not instance method), and all of the plugins that use instance variables must override the method to set the instance variables.  Because it is a class method and not an instance method, `instance_variable_set` must be used.  The overhead of all of those additional method calls (`super` and `instance_variable_set`) is what causes the dramatic difference in performance.
> > 
> > Could the design be improved such that `instance_variable_set` isn't required?  Using `instance_variable_set` means that inline caches will not be used (in MRI anyway), so I'm not sure it should be used in code that requires high performance.  I suppose we could also work on improving the performance of `instance_variable_set`, but I'm not sure usage is _that_ common.
> 
> It certainly could, but it would require more work, and would result in slower performance in the the no-plugin case.

I guess I need to read the Sequel implementation.  It seems possible to design a system that is no overhead in the no-plugin case that also uses regular instance variables.  In fact it seems like defining one "ClassMethods" module and many "InstanceMethods" modules would do the trick (with no changes to Sequel).

> We could add a private instance method can call that, and the private instance method could initialize all the instance variables to nil the usual way.  That would make things somewhat faster.  You still have all the super calls to slow things down, though.  If you want me to work on a benchmark for that, please let me know, but it's a sure bet that even that approach would result in a slowdown significant enough that I wouldn't want to switch to it.

I think we just need to measure the difference between `instance_variable_set` and regular instance variable setting.  That should give us an idea of the potential speed increase by switching to regular instance variables.

``` ruby
require "benchmark/ips"

class Embedded
  def initialize
    @a = @b = @c = nil
  end
end

class EmbeddedIvarSet
  def initialize
    instance_variable_set :@a, nil
    instance_variable_set :@b, nil
    instance_variable_set :@c, nil
  end
end

class NotEmbedded
  def initialize
    @a = @b = @c = @d = @e = @f = nil
  end
end

class NotEmbeddedIvarSet
  def initialize
    instance_variable_set :@a, nil
    instance_variable_set :@b, nil
    instance_variable_set :@c, nil
    instance_variable_set :@d, nil
    instance_variable_set :@e, nil
    instance_variable_set :@f, nil
  end
end

eval "def embedded; #{"Embedded.new;"*1000} end"
eval "def embedded_ivar_set; #{"EmbeddedIvarSet.new;"*1000} end"
eval "def not_embedded; #{"NotEmbedded.new;"*1000} end"
eval "def not_embedded_ivar_set; #{"NotEmbeddedIvarSet.new;"*1000} end"

Benchmark.ips do |x|
  x.report("embedded") { embedded }
  x.report("embedded ivar set") { embedded_ivar_set }
  x.report("not embedded") { not_embedded }
  x.report("not embedded ivar set") { not_embedded_ivar_set }
end
```

On my machine:

```
aaron@whiteclaw ~> ruby -v ivar_speed.rb
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-linux]
Warming up --------------------------------------
            embedded   792.000  i/100ms
   embedded ivar set   516.000  i/100ms
        not embedded   545.000  i/100ms
not embedded ivar set
                       312.000  i/100ms
Calculating -------------------------------------
            embedded      7.945k (± 0.2%) i/s -     40.392k in   5.084108s
   embedded ivar set      5.108k (± 0.2%) i/s -     25.800k in   5.051157s
        not embedded      5.310k (± 0.5%) i/s -     26.705k in   5.029699s
not embedded ivar set
                          3.124k (± 0.4%) i/s -     15.912k in   5.094197s
```

It looks like `instance_variable_set` requires a significant tax compared to just instance variable sets.  But I didn't know if that's the bottleneck, so I rewrote the benchmark you provided to use regular instance variables [here](https://github.com/tenderlove/sequel-benchmark/blob/regular-ivars/t.rb).

Surprisingly it was consistently slower! Not by much, but it was consistent:

```
aaron@whiteclaw ~/thing (master)> ruby -v t.rb eager_initialize plugin
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-linux]
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/plugins/json_serializer.rb:132: warning: instance variable @json_serializer_opts not initialized
t.rb:28: warning: method redefined; discarding old call
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/model/base.rb:221: warning: previous definition of call was here
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/model/base.rb:918: warning: instance variable @overridable_methods_module not initialized
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/adapters/shared/sqlite.rb:287: warning: instance variable @transaction_mode not initialized
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/adapters/shared/sqlite.rb:287: warning: instance variable @transaction_mode not initialized
Warming up --------------------------------------
  Retrieve 1000 rows    27.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows    278.107  (± 0.4%) i/s -      1.404k in   5.048542s
aaron@whiteclaw ~/thing (master)> git checkout -
Switched to branch 'regular-ivars'
aaron@whiteclaw ~/thing (regular-ivars)> ruby -v t.rb eager_initialize plugin
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-linux]
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/plugins/json_serializer.rb:132: warning: instance variable @json_serializer_opts not initialized
t.rb:42: warning: method redefined; discarding old call
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/model/base.rb:221: warning: previous definition of call was here
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/model/base.rb:918: warning: instance variable @overridable_methods_module not initialized
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/adapters/shared/sqlite.rb:287: warning: instance variable @transaction_mode not initialized
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/adapters/shared/sqlite.rb:287: warning: instance variable @transaction_mode not initialized
Warming up --------------------------------------
  Retrieve 1000 rows    23.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows    239.608  (± 0.4%) i/s -      1.219k in   5.087511s
```

These benchmarks were close enough that it made me wonder if setting instance variables was even the bottleneck of the program, so I deleted all instance variables from the program but left the method calls [here](https://github.com/tenderlove/sequel-benchmark/blob/methods-but-no-ivars/t.rb).

```
aaron@whiteclaw ~/thing (methods-but-no-ivars)> ruby -v t.rb eager_initialize plugin
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-linux]
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/plugins/json_serializer.rb:132: warning: instance variable @json_serializer_opts not initialized
t.rb:33: warning: method redefined; discarding old call
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/model/base.rb:221: warning: previous definition of call was here
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/model/base.rb:918: warning: instance variable @overridable_methods_module not initialized
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/adapters/shared/sqlite.rb:287: warning: instance variable @transaction_mode not initialized
/home/aaron/.gem/ruby/2.7.1/gems/sequel-5.35.0/lib/sequel/adapters/shared/sqlite.rb:287: warning: instance variable @transaction_mode not initialized
Warming up --------------------------------------
  Retrieve 1000 rows    26.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows    267.602  (± 0.4%) i/s -      1.352k in   5.052309s
```

This is still close to the same speed as the previous benchmark.  It seems like the cost of calling a method dwarfs the cost of setting an instance variable.

btw, `super` didn't use an inline method cache in < 2.8, so the current development branch is much faster for the above case:

```
aaron@whiteclaw ~/thing (methods-but-no-ivars)> ruby -v t.rb eager_initialize plugin
ruby 2.8.0dev (2020-07-31T12:07:19Z master f80020bc50) [x86_64-linux]
/home/aaron/.gem/ruby/2.8.0/gems/activesupport-6.0.3.2/lib/active_support/core_ext/hash/except.rb:12: warning: method redefined; discarding old except
/home/aaron/.gem/ruby/2.8.0/gems/sequel-5.35.0/lib/sequel/plugins/json_serializer.rb:132: warning: instance variable @json_serializer_opts not initialized
t.rb:33: warning: method redefined; discarding old call
/home/aaron/.gem/ruby/2.8.0/gems/sequel-5.35.0/lib/sequel/model/base.rb:221: warning: previous definition of call was here
/home/aaron/.gem/ruby/2.8.0/gems/sequel-5.35.0/lib/sequel/model/base.rb:918: warning: instance variable @overridable_methods_module not initialized
/home/aaron/.gem/ruby/2.8.0/gems/sequel-5.35.0/lib/sequel/adapters/shared/sqlite.rb:287: warning: instance variable @transaction_mode not initialized
/home/aaron/.gem/ruby/2.8.0/gems/sequel-5.35.0/lib/sequel/adapters/shared/sqlite.rb:287: warning: instance variable @transaction_mode not initialized
Warming up --------------------------------------
  Retrieve 1000 rows    31.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows    317.013  (± 0.0%) i/s -      1.612k in   5.084974s
```

That said, it's not as fast doing no method calls all together.

I'm not sure if I did the benchmarks 100% correctly, so I would appreciate a review.  The "regular ivars is slower" result makes me think I did something wrong.

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

* 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

---Files--------------------------------
t.rb (5.59 KB)


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