Feature #21274
closed
Show performance warnings for easily avoidable unnecessary implicit splat allocations
Description
In Ruby 3.4, I made many changes to reduce implicit allocations (mostly in method calling). There are still a few cases where Ruby must allocate an array for a positional splat, or a hash for a keyword splat. Some of these allocations are unavoidable, but in other cases, while Ruby cannot avoid the allocation, it is easy for a user to make a small change to their code to avoid the allocation. One example of this is when Ruby allocates to avoid an evaluation order issue. For example:
def kw = nil
ary = []
m(*ary, kw:)
Ruby allocates an array for *ary, even though it does not need to, because kw is a method call, and the method call could potentially modify ary (it doesn't in this example, but Ruby's compiler cannot assume that, as the method may be overridden later). It is simple to avoid the allocation by using a local variable:
def kw = nil
ary = []
kw = self.kw
m(*ary, kw:)
To make it easier for users to find and avoid these unnecessary implicit allocations, I would like to add a performance warning in cases where Ruby allocates solely to avoid an evaluation order issue.
I've submitted a pull request to implement this: https://github.com/ruby/ruby/pull/13135
The current warning messages in the pull request are quite verbose:
$ ruby -W:performance -e 'def kw; {} end; a = []; p(*a, **kw)'
-e: warning: This method call implicitly allocates a potentially unnecessary
array for the positional splat, because a keyword, keyword splat, or block pass
expression could cause an evaluation order issue if an array is not allocated
for the positional splat. You can avoid this allocation by assigning the related
keyword, keyword splat, or block pass expression to a local variable and using
that local variable.
$ ruby -W:performance -e 'def b; ->{} end; h = {}; p(**h, &b)'
-e: warning: This method call implicitly allocates a potentially unnecessary
hash for the keyword splat, because the block pass expression could cause an
evaluation order issue if a hash is not allocated for the keyword splat. You
can avoid this allocation by assigning the block pass expression to a local
variable, and using that local variable.
It may be desirable to shorten these messages, so I would appreciate suggestions.
Updated by jeremyevans0 (Jeremy Evans) 21 days ago
I used this feature to find the following unnecessary allocations in the standard library and a couple popular gems, which I fixed or submitted pull requests to fix:
Updated by Dan0042 (Daniel DeLorme) 4 days ago
I appreciate the effort here, but I wonder if this might be a case of premature micro-optimization. Was there a measureable performance improvement from these changes? If the gains are negligible, we should be cautious about encouraging patterns that may reduce code readability.
And I feel the message could be quite confusing for those who aren't already familiar with Ruby’s internal implementation, but I don't see how it could be improved, precisely because this warning is so tightly coupled to Ruby's internal implementation.
Updated by jeremyevans0 (Jeremy Evans) 4 days ago
Dan0042 (Daniel DeLorme) wrote in #note-2:
I appreciate the effort here, but I wonder if this might be a case of premature micro-optimization. Was there a measureable performance improvement from these changes? If the gains are negligible, we should be cautious about encouraging patterns that may reduce code readability.
The overhead of any method call that does not allocate is generally much lower than the overhead of a method call that does allocate. Is this improvement actually measurable in the above libraries? Probably not in any macro benchmark. However, in all cases, it does result in improved performance. Are there cases where the performance warning could find a place that makes a measurable different? Possibly, assuming it found a case in a hot path/inner loop.
The term premature optimization typically refers to cases where you optimize, and later, the optimization results in a negative effect. Optimizations to avoid allocations, like the type recommended by this warning, may not be measurable in isolation, but can still result in measurable improvements if they are applied throughout a codebase.
And I feel the message could be quite confusing for those who aren't already familiar with Ruby’s internal implementation, but I don't see how it could be improved, precisely because this warning is so tightly coupled to Ruby's internal implementation.
The warning method is quite verbose, and probably intimidating to many users due to its size. Which part of it do you think would be confusing? We could drop the part about why the allocation occurs (the evaluation order issue), and just tell the user to use a local variable to avoid the allocation. Do you think that would be better?
Updated by Dan0042 (Daniel DeLorme) 4 days ago
The term premature optimization typically refers to cases where you optimize, and later, the optimization results in a negative effect.
I have to disagree here. Premature optimization typically refers to cases where you optimize code that was not really a problem or bottleneck in the first place. But anyway we're getting off topic.
Which part of it do you think would be confusing? We could drop the part about why the allocation occurs (the evaluation order issue), and just tell the user to use a local variable to avoid the allocation. Do you think that would be better?
I think the best would be something like "assign 'kw' to a local variable to avoid '*a' causing an extra allocation", but I'm not sure if it's possible to get those "a" and "kw" strings. If you need the message to be generic then... I really don't know... chatgpt says "This method call creates an extra array for the splat (*args) to avoid issues with evaluation order when using keywords or blocks. To prevent this extra array, store the keyword, splat, or block in a local variable first."
Updated by mame (Yusuke Endoh) 3 days ago
This was discussed at the dev meeting, but @matz (Yukihiro Matsumoto) was negative about adding this warning. He prefers fluent code over performance, so from Ruby's perspective, he did not want to encourage rewriting:
order!(argv, **keywords, &nonopts.method(:<<))
to:
method = nonopts.method(:<<)
order!(argv, **keywords, &method)
Updated by jeremyevans0 (Jeremy Evans) 3 days ago
- Status changed from Open to Closed