[ruby-core:99460] [Ruby master Feature#17055] Allow suppressing uninitialized instance variable and method redefined verbose mode warnings
From:
tenderlove@...
Date:
2020-08-03 18:31:36 UTC
List:
ruby-core #99460
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 datab=
ase in Sequel run through `Sequel::Model.call` (class method, not instance =
method), and all of the plugins that use instance variables must override t=
he 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_se=
t`) is what causes the dramatic difference in performance.
> > =
> > Could the design be improved such that `instance_variable_set` isn't re=
quired? 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 requi=
res high performance. I suppose we could also work on improving the perfor=
mance 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 s=
lower performance in the the no-plugin case.
I guess I need to read the Sequel implementation. It seems possible to des=
ign a system that is no overhead in the no-plugin case that also uses regul=
ar instance variables. In fact it seems like defining one "ClassMethods" m=
odule and many "InstanceMethods" modules would do the trick (with no change=
s to Sequel).
> We could add a private instance method can call that, and the private ins=
tance method could initialize all the instance variables to nil the usual w=
ay. 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 f=
or that, please let me know, but it's a sure bet that even that approach wo=
uld 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_s=
et` 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 =3D @b =3D @c =3D 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 =3D @b =3D @c =3D @d =3D @e =3D @f =3D 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 (=B1 0.2%) i/s - 40.392k in 5.084108s
embedded ivar set 5.108k (=B1 0.2%) i/s - 25.800k in 5.051157s
not embedded 5.310k (=B1 0.5%) i/s - 26.705k in 5.029699s
not embedded ivar set
3.124k (=B1 0.4%) i/s - 15.912k in 5.094197s
```
It looks like `instance_variable_set` requires a significant tax compared t=
o 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_seri=
alizer.rb:132: warning: instance variable @json_serializer_opts not initial=
ized
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/s=
qlite.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/s=
qlite.rb:287: warning: instance variable @transaction_mode not initialized
Warming up --------------------------------------
Retrieve 1000 rows 27.000 i/100ms
Calculating -------------------------------------
Retrieve 1000 rows 278.107 (=B1 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 plug=
in
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_seri=
alizer.rb:132: warning: instance variable @json_serializer_opts not initial=
ized
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/s=
qlite.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/s=
qlite.rb:287: warning: instance variable @transaction_mode not initialized
Warming up --------------------------------------
Retrieve 1000 rows 23.000 i/100ms
Calculating -------------------------------------
Retrieve 1000 rows 239.608 (=B1 0.4%) i/s - 1.219k in 5.087511s
```
These benchmarks were close enough that it made me wonder if setting instan=
ce variables was even the bottleneck of the program, so I deleted all insta=
nce variables from the program but left the method calls [here](https://git=
hub.com/tenderlove/sequel-benchmark/blob/methods-but-no-ivars/t.rb).
```
aaron@whiteclaw ~/thing (methods-but-no-ivars)> ruby -v t.rb eager_initiali=
ze 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_seri=
alizer.rb:132: warning: instance variable @json_serializer_opts not initial=
ized
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/s=
qlite.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/s=
qlite.rb:287: warning: instance variable @transaction_mode not initialized
Warming up --------------------------------------
Retrieve 1000 rows 26.000 i/100ms
Calculating -------------------------------------
Retrieve 1000 rows 267.602 (=B1 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 va=
riable.
btw, `super` didn't use an inline method cache in < 2.8, so the current dev=
elopment branch is much faster for the above case:
```
aaron@whiteclaw ~/thing (methods-but-no-ivars)> ruby -v t.rb eager_initiali=
ze 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/c=
ore_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_seri=
alizer.rb:132: warning: instance variable @json_serializer_opts not initial=
ized
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/s=
qlite.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/s=
qlite.rb:287: warning: instance variable @transaction_mode not initialized
Warming up --------------------------------------
Retrieve 1000 rows 31.000 i/100ms
Calculating -------------------------------------
Retrieve 1000 rows 317.013 (=B1 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 someth=
ing wrong.
----------------------------------------
Feature #17055: Allow suppressing uninitialized instance variable and metho=
d 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 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
---Files--------------------------------
t.rb (5.59 KB)
-- =
https://bugs.ruby-lang.org/
Unsubscribe: <mailto:[email protected]?subject=3Dunsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>