Issue #17055 has been updated by jeremyevans0 (Jeremy Evans). Eregon (Benoit Daloze) wrote in #note-9: > jeremyevans0 (Jeremy Evans) wrote in #note-6: > > Yes. When you initialize an instance variable to nil, it slows things = down, and there is no benefit because the trying to access an uninitialized= instance variable returns nil anyway (plus warning in verbose mode) > = > And when you don't initialize, reads might become slower, because they ha= ve to check if warnings are enabled, and it might create polymorphism (see = the end of my reply). > It's a two-sides blade. This is correct. Whether or not to initialize instance variables depends o= n the object. For long lived objects (classes, constants), it definitely m= akes sense. For ephemeral objects, it can hurt performance. > I'd argue allocating without ever accessing or storing objects is an unre= alistic benchmark. > = > Can you show a significant overhead on a realistic benchmark? (some Seque= l real-world usage maybe?) The last time I did testing on this in Sequel, the performance decrease fro= m initializing instance variables to nil was around 5% for Sequel::Model in= stance 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 d= ecrease) just to avoid verbose warnings. > Here is the result on `truffleruby 20.3.0-dev-b7a9b466` with your microbe= nchmark and 10 instead of 1000. > It shows the benchmark is bad mostly (i.e., it optimizes away and does no= thing useful). > = > ``` > truffleruby bench_ivar_set.rb = > Warming up -------------------------------------- > initialized 423.378M i/100ms > uninitialized 191.211M i/100ms > initialized 209.219M i/100ms > uninitialized 60.855M i/100ms > Calculating ------------------------------------- > initialized 2.093B (=B1 0.2%) i/s - 10.670B in 5.0980= 08s > uninitialized 2.093B (=B1 0.4%) i/s - 10.467B in 5.0002= 14s > initialized 2.019B (=B114.7%) i/s - 9.624B in 5.0368= 43s > uninitialized 2.122B (=B1 2.3%) i/s - 10.650B in 5.0216= 45s > = > Comparison: > uninitialized: 2122273181.8 i/s > initialized: 2018820267.6 i/s - same-ish: difference falls withi= n error > ``` > Here is the file used: [bench_ivar_set.rb](https://gist.github.com/eregon= /282070e6f6686740a0c8e41243fb598b). Interesting. If it is not too much trouble, what are the results with 1000= instead of 10, if I may ask? Alternatively, if you modify the benchmark t= o access each instance variable once, what the the results of that? I'd te= st myself, but it appears TruffleRuby is not yet ported to the operating sy= stems I use (OpenBSD/Windows). > > The warning gem has always supported this. It was the primary reason I= worked on adding the warning support to Ruby 2.4. > = > Isn't it good enough of a solution already? (can easily narrow by file & = ivar name) > Adding a dependency on it doesn't seem a big deal and even deserved if yo= u want such fine control over warnings and purposefully suppress warnings. > One can also `prepend` a module to `Warning` to handle this without an ex= tra dependency. In my opinion, a ruby gem should never modify the behavior (e.g. add/overri= de methods) of core classes, unless that is the purpose of the library. As= such, modifying the Warning class is not something I would consider doing = by default, and therefore people that use Sequel and want to run tests/deve= lopment in verbose mode have to filter the warnings themselves. With the f= eature I am proposing, each library has control over their own code. I believe verbose warning for uninitialized instance variables in code you = have no control over is actively harmful to the user, because it can make i= t more difficult to find warnings in code you do have control over. > > > Calling more methods when doing warnings is adding more boilerplate, = complexity and edge cases to these code paths. > > = > > In the uninitialized instance variable case, I was actually able to red= uce three separate code paths for issuing the warning to a single code path= , plus I found a case that should produce a warning that did not and fixed = that. So overall complexity could be lower for the uninitialized variable = case, at least for MRI. > = > There might be other good things in the PR. > It's irrelevant to the general added complexity of a new callback. > Imagine if we wanted to add such methods for more warnings, I think that = would quickly become a mess. Certainly this approach is not appropriate for all warnings. However, in m= y 15+ years of Ruby experience, I've seen that these two verbose warnings a= re by far the most common, and both of them have valid reasons for using th= e behavior that produces the verbose warnings (performance and safety). > > The proactive approach is substantially less flexible (e.g. can't use a= regexp), and would require storing the values and a significantly more com= plex implementation. Considering you just complained about the complexity = of my patch, I find it strange that you would propose an approach that woul= d definitely require even greater internal complexity. > = > I think well-designed and Ruby-like API is more important than complexity= here. I fail to see how this is not a Ruby-like API. It's similar to other callb= acks, such as `method_added`. It's also the simplest thing I can think of = that would work. Additionally, as @byroot mentioned, it may be possible to= use this approach with did_you_mean for even more helpful warnings. > > The advantage of this approach is that it allows you to get the maximum= possible performance in production, while suppressing unnecessary warnings= in development or testing when you may run with verbose warnings. Without= this, you either need to give up some production performance, or you have = to require the user install a separate library to filter out the verbose wa= rnings. > = > My view is it encourages less readable code by trying to squeak a tiny bi= t of performance when running on current CRuby. This is only half about performance. You haven't mentioned anything about = the method redefinition warning yet. Can you provide your thoughts on that? > In other words, lazily initializing @ivars causes reads to need some bran= ching because they need to handle both the initialized and uninitialized ca= ses. > So @ivars reads can no longer be straight-line code, which can impact per= formance as shown above. As I stated above, whether you want to initialize instance variables lazily= or eagerly for performance depends on the object in question. Not all sit= uations are the same. It is faster in some situations and slower in others. > Also if the natural default is not `nil` but say `0` then `@foo =3D 0` in= `initialize` is useful information (notably it gives the type), and it can= be a simple `attr_reader :foo` to read it instead of the convoluted: > ```ruby > def foo > @foo || 0 > end > ``` I think we can all agree that there are cases where eagerly initializing th= e object can improve performance. This says nothing about the cases where = eagerly initializing the object hurts performance. ---------------------------------------- Feature #17055: Allow suppressing uninitialized instance variable and metho= d redefined verbose mode warnings https://bugs.ruby-lang.org/issues/17055#change-86900 * 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>