[ruby-core:99457] [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).

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::Model 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% performance decrease) just to avoid verbose warnings.
> 
> Could you reproduce that again?
> I would like a realistic benchmark, so unless users frequently create Sequel::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 remember it being. With the attached benchmark, on PostgreSQL localhost, the difference is over 100% when not using plugins:

```
$ NO_SEQUEL_PG=1 DATABASE_URL=postgres:///?user=sequel_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=1 DATABASE_URL=postgres:///?user=sequel_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=1 DATABASE_URL=postgres:///?user=sequel_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=1 DATABASE_URL=postgres:///?user=sequel_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 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.

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 ||= nil; end`? That would 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 much worse performance in `$VERBOSE` mode.
> Admittedly this would only happen when `$VERBOSE` is true, but performance 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  (± 0.00) slower
> ```

I think most Ruby users would trade faster production performance for slower tests.  Obviously, for those that wouldn't make that trade, they can initialize 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 suppress warnings for a block of code, and that could be used in this case and many other cases (e.g., 194 `suppress_warning` in ruby/ruby).
> Unfortunately, changing `$VERBOSE` is not thread-safe if there are multiple Threads.
> That IMHO would be a valuable addition, and be useful not just for this method-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 method 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 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>