Issue #17055 has been updated by Eregon (Benoit Daloze).


jeremyevans0 (Jeremy Evans) wrote in #note-11:
> The last time I did testing on this in Sequel, the performance decrease f=
rom initializing instance variables to nil was around 5% for Sequel::Model =
instance creation depending on the plugins in use. One of the reasons it wa=
s around 5% was that many plugins had to override initialization methods ju=
st to set an instance variable and call `super`.  5% may not sound like a l=
ot, 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 Seque=
l::Model instances themselves, I guess the normal case is the data comes fr=
om somewhere (the database, some file, etc).
In such a case I would think the overhead is not measurable.

> Interesting.  If it is not too much trouble, what are the results with 10=
00 instead of 10, if I may ask?

Duplicating code like that is IMHO a misleading way to benchmark (it makes =
it all too easy to misinterpret results) and obviously unrepresentative of =
real code.
I see it used for implementations like CRuby for which block call is a larg=
e overhead, but IMHO that should be a reminder there is a point where it's =
too small to measure on its own, and there is always code around for realis=
tic cases.
Also the Ruby implementation might see that the result is unused due to thi=
s repetition (TruffleRuby did above).
I used 10 instead of 1000 since Charles did the same. But it should be just=
 1, really, and the benchmark should use the result value.

Anyway, this is the result for 1000, not everything is inlined since the me=
thod is so large, yet there seems to be no meaningful difference:
```
Calculating -------------------------------------
         initialized      4.257k (=B141.1%) i/s -     14.136k in   5.064145s
       uninitialized      6.307k (=B162.3%) i/s -     17.864k in   5.002209s
         initialized     12.782k (=B123.8%) i/s -     53.010k in   5.013438s
       uninitialized     13.123k (=B125.5%) i/s -     53.012k in   5.002126s

Comparison:
       uninitialized:    13123.0 i/s
         initialized:    12782.2 i/s - same-ish: difference falls within er=
ror
```

> Alternatively, if you modify the benchmark to access each instance variab=
le once, what the the results of that?

It still optimizes away, because the allocation is never needed:
https://gist.github.com/eregon/f279901e3df450d7a1970b76b9653c71
```
Calculating -------------------------------------
         initialized      1.614B (=B143.2%) i/s -      6.184B in   5.236291s
       uninitialized      1.816B (=B133.1%) i/s -      7.259B in   5.078576s
         initialized      1.879B (=B128.2%) i/s -      7.890B in   5.043604s
       uninitialized      1.811B (=B132.4%) i/s -      7.259B in   5.007219s

Comparison:
         initialized: 1878613372.9 i/s
       uninitialized: 1811213500.0 i/s - same-ish: difference falls within =
error

```

We need a realistic benchmark, then we'll see if there is an observable dif=
ference or not and how much it matters in practice.

> I'd test myself, but it appears TruffleRuby is not yet ported to the oper=
ating systems I use (OpenBSD/Windows).

Feel free to open an issue about building TruffleRuby on OpenBSD. Maybe it'=
s not so hard (but would need to build most things from source).
Alternatively I guess a more convenient way is to use Docker (but it might =
affect performance).

> In my opinion, a ruby gem should never modify the behavior (e.g. add/over=
ride methods) of core classes, unless that is the purpose of the library.  =
As such, modifying the Warning class is not something I would consider doin=
g by default, and therefore people that use Sequel and want to run tests/de=
velopment in verbose mode have to filter the warnings themselves.  With the=
 feature I am proposing, each library has control over their own code.

My view of that is `Warning.warn` is designed explicitly for this, so that =
libraries can compose and filters warnings as needed.
Even better if the warning can be avoided in the first place though IMHO.

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

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 t=
o 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
```

> This is only half about performance.  You haven't mentioned anything abou=
t the method redefinition warning yet.  Can you provide your thoughts on th=
at?

Regarding method redefinition I think the `alias`/`alias_method` trick is r=
easonable, and has the advantage to already work on all Ruby versions.
It's not obvious though so I wonder if we could have something clearer like=
 `suppress_redefinition_warning instance_method(:my_method)`, but anyway me=
thod redefinition without caring of the previous definition should be very =
rare so `alias` seems a fine enough workaround.

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 met=
hod-redefinition-ignoring-previous case but in many other cases too.
(it wouldn't be a good solution for the uninitialized ivar case, there I th=
ink it's really best to avoid the warning in the first place)

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

* 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



-- =

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>