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:ruby-core-request / ruby-lang.org?subject=3Dunsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>