From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from neon.ruby-lang.org (neon.ruby-lang.org [221.186.184.75]) by dcvr.yhbt.net (Postfix) with ESMTP id BB9111F5AE for ; Sun, 2 Aug 2020 15:38:39 +0000 (UTC) Received: from neon.ruby-lang.org (localhost [IPv6:::1]) by neon.ruby-lang.org (Postfix) with ESMTP id BB283120A07; Mon, 3 Aug 2020 00:38:06 +0900 (JST) Received: from xtrwkhkc.outbound-mail.sendgrid.net (xtrwkhkc.outbound-mail.sendgrid.net [167.89.16.28]) by neon.ruby-lang.org (Postfix) with ESMTPS id 7049B120A05 for ; Mon, 3 Aug 2020 00:38:04 +0900 (JST) Received: by filterdrecv-p3iad2-d8cc6d457-97jrw with SMTP id filterdrecv-p3iad2-d8cc6d457-97jrw-19-5F26DDF7-4A 2020-08-02 15:38:31.979389668 +0000 UTC m=+336605.960682194 Received: from herokuapp.com (unknown) by ismtpd0026p1iad2.sendgrid.net (SG) with ESMTP id imcsPa2yRsSQEarA6ZHM6A for ; Sun, 02 Aug 2020 15:38:31.965 +0000 (UTC) Date: Sun, 02 Aug 2020 15:38:31 +0000 (UTC) From: merch-redmine@jeremyevans.net Message-ID: References: Mime-Version: 1.0 X-Redmine-MailingListIntegration-Message-Ids: 75274 X-Redmine-Project: ruby-master X-Redmine-Issue-Tracker: Feature X-Redmine-Issue-Id: 17055 X-Redmine-Issue-Author: jeremyevans0 X-Redmine-Sender: jeremyevans0 X-Mailer: Redmine X-Redmine-Host: bugs.ruby-lang.org X-Redmine-Site: Ruby Issue Tracking System X-Auto-Response-Suppress: All Auto-Submitted: auto-generated X-SG-EID: =?us-ascii?Q?RVE3t853K5scBhbmJHUzZTFFeVC=2FZSUmHZ0Dc+26wcEi2CTgsF1oz0wTSSxGGN?= =?us-ascii?Q?BIJyU84k0sl3+n9IamchWL+zU19P5fTGdyFtel8?= =?us-ascii?Q?n=2FQ7oPqEPDqWjh3JJZ4r=2FJQaM+zMhVg6OKD15n9?= =?us-ascii?Q?GwZxtULMVRk51NTiFhY+6enxRnBf=2FaQR+I0VWAt?= =?us-ascii?Q?RvsubNjx3CJraG2g0ddOD90tj4ZMGil4MGlBcDX?= =?us-ascii?Q?ULMVXcWU61dfZgHNs=3D?= To: ruby-core@ruby-lang.org X-ML-Name: ruby-core X-Mail-Count: 99445 Subject: [ruby-core:99445] [Ruby master Feature#17055] Allow suppressing uninitialized instance variable and method redefined verbose mode warnings X-BeenThere: ruby-core@ruby-lang.org X-Mailman-Version: 2.1.15 Precedence: list Reply-To: Ruby developers List-Id: Ruby developers List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Errors-To: ruby-core-bounces@ruby-lang.org Sender: "ruby-core" 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: