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

File t.rb added

Eregon (Benoit Daloze) wrote in #note-12:
> jeremyevans0 (Jeremy Evans) wrote in #note-11:
> > The last time I did testing on this in Sequel, the performance decrease=
 from initializing instance variables to nil was around 5% for Sequel::Mode=
l instance creation depending on the plugins in use. One of the reasons it =
was around 5% was that many plugins had to override initialization methods =
just to set an instance variable and call `super`.  5% may not sound like a=
 lot, but I can't justify a 5% performance decrease (or even a 1% performan=
ce decrease) just to avoid verbose warnings.
> =

> Could you reproduce that again?
> I would like a realistic benchmark, so unless users frequently create Seq=
uel::Model instances themselves, I guess the normal case is the data comes =
from somewhere (the database, some file, etc).
> In such a case I would think the overhead is not measurable.

I'm glad you asked, as the difference is actually much greater than I remem=
ber it being. With the attached benchmark, on PostgreSQL localhost, the dif=
ference is over 100% when not using plugins:

```
$ NO_SEQUEL_PG=3D1 DATABASE_URL=3Dpostgres:///?user=3Dsequel_test ruby t.rb=
 regular noplugin
Warming up --------------------------------------
  Retrieve 1000 rows    31.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows    310.225  (_ 2.6%) i/s -      1.550k in   5.000031s

$ NO_SEQUEL_PG=3D1 DATABASE_URL=3Dpostgres:///?user=3Dsequel_test ruby t.rb=
 eager_initialize noplugin
Warming up --------------------------------------
  Retrieve 1000 rows    15.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows    151.327  (_ 2.0%) i/s -    765.000  in   5.057570s
```

and over 150% when using plugins:

```
$ NO_SEQUEL_PG=3D1 DATABASE_URL=3Dpostgres:///?user=3Dsequel_test ruby t.rb=
 regular plugin
1000
Warming up --------------------------------------
  Retrieve 1000 rows    23.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows    233.096  (_ 2.6%) i/s -      1.173k in   5.036040s

$ NO_SEQUEL_PG=3D1 DATABASE_URL=3Dpostgres:///?user=3Dsequel_test ruby t.rb=
 eager_initialize plugin
Warming up --------------------------------------
  Retrieve 1000 rows     8.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows     90.653  (_ 1.1%) i/s -    456.000  in   5.030471s
```

With an SQLite memory database and no plugins, the difference is over 35%:

```
$ ruby t.rb regular noplugin
Warming up --------------------------------------
  Retrieve 1000 rows     8.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows     84.154  (_ 1.2%) i/s -    424.000  in   5.039503s

$ ruby t.rb eager_initialize noplugin
Warming up --------------------------------------
  Retrieve 1000 rows     6.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows     61.862  (_ 1.6%) i/s -    312.000  in   5.044981s
```

and with plugins added, the difference is over 50%:

```
$ ruby t.rb regular plugin
Warming up --------------------------------------
  Retrieve 1000 rows     7.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows     73.384  (_ 2.7%) i/s -    371.000  in   5.059455s

$ ruby t.rb eager_initialize plugin
Warming up --------------------------------------
  Retrieve 1000 rows     4.000  i/100ms
Calculating -------------------------------------
  Retrieve 1000 rows     47.122  (_ 0.0%) i/s -    236.000  in   5.008490s
```

As you'll see by the benchmark, the reason the performance difference is mu=
ch 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 met=
hod 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 al=
l of those additional method calls (`super` and `instance_variable_set`) is=
 what causes the dramatic difference in performance.

Feel free to play around with the above benchmark, but I hope it shows you =
that I'm asking for this feature for a good reason.

> So, for this case, how about using `def foo; @foo ||=3D nil; end`? That w=
ould avoid the warning and still let you assign the `@ivar` lazily, no?

Then you need a method call instead of an ivar access, which is also slower=
.  It also would require more changes across the library.

> I have another concern: hiding warnings like this will silently cause muc=
h worse performance in `$VERBOSE` mode.
> Admittedly this would only happen when `$VERBOSE` is true, but performanc=
e might still matter to some degree in such cases, e.g., when running tests=
 (tests frameworks often enable $VERBOSE, and people still want their tests=
 to run reasonably fast).
> =

> In such a case, on you branch it's 6.6 times slower for reads:
> https://gist.github.com/eregon/32f4119b7796ec7a6243c68990949597
> ```
> Comparison:
>          initialized: 14754531.3 i/s
>        uninitialized:  2227414.1 i/s - 6.62x  (=B1 0.00) slower
> ```

I think most Ruby users would trade faster production performance for slowe=
r tests.  Obviously, for those that wouldn't make that trade, they can init=
ialize all instance variables and would not need to use this.
 =

> I think in general it would be useful to have a thread-safe way to suppre=
ss warnings for a block of code, and that could be used in this case and ma=
ny other cases (e.g., 194 `suppress_warning` in ruby/ruby).
> Unfortunately, changing `$VERBOSE` is not thread-safe if there are multip=
le Threads.
> That IMHO would be a valuable addition, and be useful not just for this m=
ethod-redefinition-ignoring-previous case but in many other cases too.
> (it wouldn't be a good solution for the uninitialized ivar case, there I =
think it's really best to avoid the warning in the first place)

Possibly useful, but unrelated to the current proposal.  Please post a new =
feature request if you would like that.

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

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