Avoid array allocation for *nil, by not calling nil.to_a#12597
Merged
jeremyevans merged 1 commit intoruby:masterfrom Mar 27, 2025
Merged
Avoid array allocation for *nil, by not calling nil.to_a#12597jeremyevans merged 1 commit intoruby:masterfrom
jeremyevans merged 1 commit intoruby:masterfrom
Conversation
a53d283 to
9bc5d59
Compare
Contributor
|
Is this PR for performance? Please show us the benchmark results. |
Contributor
Author
|
This isn't just for performance, though it improves performance for the I've pushed a benchmark, it shows a 4.45x speedup: |
9b9f0d4 to
c98760d
Compare
This comment has been minimized.
This comment has been minimized.
a9a488a to
2e81bc4
Compare
The following method call: ```ruby a(*nil) ``` A method call such as `a(*nil)` previously allocated an array, because it calls `nil.to_a`, but I have determined this array allocation is unnecessary. The instructions in this case are: ``` 0000 putself ( 1)[Li] 0001 putnil 0002 splatarray false 0004 opt_send_without_block <calldata!mid:a, argc:1, ARGS_SPLAT|FCALL> 0006 leave ``` The method call uses `ARGS_SPLAT` without `ARGS_SPLAT_MUT`, so the returned array doesn't need to be mutable. I believe all cases where `splatarray false` are used allow the returned object to be frozen, since the `false` means to not duplicate the array. The optimization in this case is to have `splatarray false` push a shared empty frozen array, instead of calling `nil.to_a` to return a newly allocated array. There is a slightly backwards incompatibility with this optimization, in that `nil.to_a` is not called. However, I believe the new behavior of `*nil` not calling `nil.to_a` is more consistent with how `**nil` does not call `nil.to_hash`. Also, so much Ruby code would break if `nil.to_a` returned something different from the empty hash, that it's difficult to imagine anyone actually doing that in real code, though we have a few tests/specs for that. I think it would be bad for consistency if `*nil` called `nil.to_a` in some cases and not others, so this changes other cases to not call `nil.to_a`: For `[*nil]`, this uses `splatarray true`, which now allocates a new array for a `nil` argument without calling `nil.to_a`. For `[1, *nil]`, this uses `concattoarray`, which now returns the first array if the second array is `nil`. This updates the allocation tests to check that the array allocations are avoided where possible. Implements [Feature #21047]
2e81bc4 to
6b338a6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A method call such as
a(*nil)previously allocated an array, because it callsnil.to_a, but I have determined this array allocation is unnecessary. The instructions in this case are:The method call uses
ARGS_SPLATwithoutARGS_SPLAT_MUT, so the returned array doesn't need to be mutable. I believe all cases wheresplatarray falseare used allow the returned object to be frozen, since thefalsemeans to not duplicate the array. The optimization in this case is to havesplatarray falsepush a shared empty frozen array, instead of callingnil.to_ato return a newly allocated array.There is a slightly backwards incompatibility with this optimization, in that
nil.to_ais not called. However, I believe the new behavior of*nilnot callingnil.to_ais more consistent with how**nildoes not callnil.to_hash. Also, so much Ruby code would break ifnil.to_areturned something different from the empty hash, that it's difficult to imagine anyone actually doing that in real code, though we have a few tests/specs for that.I think it would be bad for consistency if
*nilcallednil.to_ain some cases and not others, so this changes other cases to not callnil.to_a:For
[*nil], this usessplatarray true, which now allocates a new array for anilargument without callingnil.to_a.For
[1, *nil], this usesconcattoarray, which now returns the first array if the second array isnil.This updates the allocation tests to check that the array allocations are avoided where possible.