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>