1berry/ruby
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Avoid array allocation for *nil, by not calling nil.to_a

Browse Source 
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]
This commit is contained in:
Jeremy Evans 2025-01-18 01:40:05 -08:00
parent 6ecfe643b5
commit 67d1dd2ebd
Notes: git 2025-03-27 18:17:57 +00:00 
Merged: https://github.com/ruby/ruby/pull/12597
6 changed files with 63 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
NEWS.md
View File

@ -7,6 +7,9 @@ Note that each entry is kept to a minimum, see links for details.
## Language changes
* `*nil` no longer calls `nil.to_a`, similar to how `**nil` does
not call `nil.to_hash`. [[Feature #21047]]
## Core classes updates
Note: We're only listing outstanding class updates.
@ -84,4 +87,5 @@ The following bundled gems are updated.
## JIT
[Feature #19908]: https://bugs.ruby-lang.org/issues/19908
[Feature #21047]: https://bugs.ruby-lang.org/issues/21047
[Bug #21049]: https://bugs.ruby-lang.org/issues/21049

4
benchmark/nilclass.yml
View File

@ -1,6 +1,10 @@
prelude: |
def a = nil
benchmark:
to_i: |
nil.to_i
to_f: |
nil.to_f
splat: |
a(*nil)
loop_count: 100000

4
bootstraptest/test_literal.rb
View File

@ -117,8 +117,8 @@ assert_equal '1', 'a = [obj = Object.new]; a.size'
assert_equal 'true', 'a = [obj = Object.new]; a[0] == obj'
assert_equal '5', 'a = [1,2,3]; a[1] = 5; a[1]'
assert_equal 'bar', '[*:foo];:bar'
assert_equal '[1, 2]', 'def nil.to_a; [2]; end; [1, *nil]'
assert_equal '[1, 2]', 'def nil.to_a; [1, 2]; end; [*nil]'
assert_equal '[]', 'def nil.to_a; [1, 2]; end; [*nil]'
assert_equal '[1]', 'def nil.to_a; [2]; end; [1, *nil]'
assert_equal '[0, 1, {2 => 3}]', '[0, *[1], 2=>3]', "[ruby-dev:31592]"

19
spec/ruby/language/variables_spec.rb
View File

@ -363,11 +363,22 @@ describe "Multiple assignment" do
a.should == []
end
it "calls #to_a to convert nil to an empty Array" do
nil.should_receive(:to_a).and_return([])
ruby_version_is "3.5" do
it "converts nil to empty array without calling a method" do
nil.should_not_receive(:to_a)
(*a = *nil).should == []
a.should == []
(*a = *nil).should == []
a.should == []
end
end
ruby_version_is ""..."3.5" do
it "calls #to_a to convert nil to an empty Array" do
nil.should_receive(:to_a).and_return([])
(*a = *nil).should == []
a.should == []
end
end
it "does not call #to_a on an Array" do

34
test/ruby/test_allocation.rb
View File

@ -94,13 +94,14 @@ class TestAllocation < Test::Unit::TestCase
def block
''
end
alias only_block block
def test_no_parameters
only_block = block.empty? ? block : block[2..]
check_allocations(<<~RUBY)
def self.none(#{only_block}); end
check_allocations(0, 0, "none(#{only_block})")
check_allocations(0, 0, "none(*nil#{block})")
check_allocations(0, 0, "none(*empty_array#{block})")
check_allocations(0, 0, "none(**empty_hash#{block})")
check_allocations(0, 0, "none(*empty_array, **empty_hash#{block})")
@ -156,6 +157,9 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 0, "optional(*r2k_empty_array1#{block})")
check_allocations(0, 1, "optional(*r2k_array#{block})")
check_allocations(0, 0, "optional(*empty_array#{block})")
check_allocations(0, 0, "optional(*nil#{block})")
check_allocations(0, 0, "optional(#{only_block})")
check_allocations(0, 1, "optional(*empty_array, **hash1, **empty_hash#{block})")
RUBY
end
@ -179,6 +183,8 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(1, 0, "splat(1, *array1, **empty_hash#{block})")
check_allocations(1, 0, "splat(1, *array1, *empty_array, **empty_hash#{block})")
check_allocations(1, 0, "splat(*nil#{block})")
check_allocations(1, 0, "splat(#{only_block})")
check_allocations(1, 1, "splat(**hash1#{block})")
check_allocations(1, 1, "splat(**hash1, **empty_hash#{block})")
@ -196,6 +202,7 @@ class TestAllocation < Test::Unit::TestCase
def self.req_splat(x, *y#{block}); end
check_allocations(1, 0, "req_splat(1#{block})")
check_allocations(1, 0, "req_splat(1, *nil#{block})")
check_allocations(1, 0, "req_splat(1, *empty_array#{block})")
check_allocations(1, 0, "req_splat(1, **empty_hash#{block})")
check_allocations(1, 0, "req_splat(1, *empty_array, **empty_hash#{block})")
@ -226,6 +233,7 @@ class TestAllocation < Test::Unit::TestCase
def self.splat_post(*x, y#{block}); end
check_allocations(1, 0, "splat_post(1#{block})")
check_allocations(1, 0, "splat_post(1, *nil#{block})")
check_allocations(1, 0, "splat_post(1, *empty_array#{block})")
check_allocations(1, 0, "splat_post(1, **empty_hash#{block})")
check_allocations(1, 0, "splat_post(1, *empty_array, **empty_hash#{block})")
@ -267,6 +275,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 1, "keyword(**hash1, **empty_hash#{block})")
check_allocations(0, 1, "keyword(**empty_hash, **hash1#{block})")
check_allocations(0, 0, "keyword(*nil#{block})")
check_allocations(0, 0, "keyword(*empty_array#{block})")
check_allocations(1, 0, "keyword(*empty_array, *empty_array, **empty_hash#{block})")
@ -294,6 +303,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 1, "keyword_splat(**hash1, **empty_hash#{block})")
check_allocations(0, 1, "keyword_splat(**empty_hash, **hash1#{block})")
check_allocations(0, 1, "keyword_splat(*nil#{block})")
check_allocations(0, 1, "keyword_splat(*empty_array#{block})")
check_allocations(1, 1, "keyword_splat(*empty_array, *empty_array, **empty_hash#{block})")
@ -321,6 +331,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 1, "keyword_and_keyword_splat(**hash1, **empty_hash#{block})")
check_allocations(0, 1, "keyword_and_keyword_splat(**empty_hash, **hash1#{block})")
check_allocations(0, 1, "keyword_and_keyword_splat(*nil#{block})")
check_allocations(0, 1, "keyword_and_keyword_splat(*empty_array#{block})")
check_allocations(1, 1, "keyword_and_keyword_splat(*empty_array, *empty_array, **empty_hash#{block})")
@ -348,6 +359,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 1, "required_and_keyword(1, **hash1, **empty_hash#{block})")
check_allocations(0, 1, "required_and_keyword(1, **empty_hash, **hash1#{block})")
check_allocations(0, 0, "required_and_keyword(1, *nil#{block})")
check_allocations(0, 0, "required_and_keyword(1, *empty_array#{block})")
check_allocations(1, 0, "required_and_keyword(1, *empty_array, *empty_array, **empty_hash#{block})")
@ -391,6 +403,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(1, 1, "splat_and_keyword(1, **hash1, **empty_hash#{block})")
check_allocations(1, 1, "splat_and_keyword(1, **empty_hash, **hash1#{block})")
check_allocations(1, 0, "splat_and_keyword(1, *nil#{block})")
check_allocations(1, 0, "splat_and_keyword(1, *empty_array#{block})")
check_allocations(1, 0, "splat_and_keyword(1, *empty_array, *empty_array, **empty_hash#{block})")
@ -436,6 +449,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 1, "required_and_keyword_splat(1, **hash1, **empty_hash#{block})")
check_allocations(0, 1, "required_and_keyword_splat(1, **empty_hash, **hash1#{block})")
check_allocations(0, 1, "required_and_keyword_splat(1, *nil#{block})")
check_allocations(0, 1, "required_and_keyword_splat(1, *empty_array#{block})")
check_allocations(1, 1, "required_and_keyword_splat(1, *empty_array, *empty_array, **empty_hash#{block})")
@ -479,6 +493,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(1, 1, "splat_and_keyword_splat(1, **hash1, **empty_hash#{block})")
check_allocations(1, 1, "splat_and_keyword_splat(1, **empty_hash, **hash1#{block})")
check_allocations(1, 1, "splat_and_keyword_splat(1, *nil#{block})")
check_allocations(1, 1, "splat_and_keyword_splat(1, *empty_array#{block})")
check_allocations(1, 1, "splat_and_keyword_splat(1, *empty_array, *empty_array, **empty_hash#{block})")
@ -529,6 +544,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*array1, a: 2#{block})")
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*nil, **nill#{block})")
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*array1, **nill#{block})")
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*array1, **empty_hash#{block})")
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*array1, **hash1#{block})")
@ -575,6 +591,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*array1, a: 2#{block})")
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*nil, **nill#{block})")
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*array1, **nill#{block})")
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*array1, **empty_hash#{block})")
check_allocations(0, 0, "anon_splat_and_anon_keyword_splat(*array1, **hash1#{block})")
@ -620,6 +637,8 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 0, "argument_forwarding(*array1, a: 2#{block})")
check_allocations(0, 0, "argument_forwarding(**nill#{block})")
check_allocations(0, 0, "argument_forwarding(*nil, **nill#{block})")
check_allocations(0, 0, "argument_forwarding(*array1, **nill#{block})")
check_allocations(0, 0, "argument_forwarding(*array1, **empty_hash#{block})")
check_allocations(0, 0, "argument_forwarding(*array1, **hash1#{block})")
@ -666,6 +685,8 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(0, 0, "argument_forwarding(*array1, a: 2#{block})")
check_allocations(0, 0, "argument_forwarding(**nill#{block})")
check_allocations(0, 0, "argument_forwarding(*nil, **nill#{block})")
check_allocations(0, 0, "argument_forwarding(*array1, **nill#{block})")
check_allocations(0, 0, "argument_forwarding(*array1, **empty_hash#{block})")
check_allocations(0, 0, "argument_forwarding(*array1, **hash1#{block})")
@ -712,6 +733,8 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(1, 1, "r2k(*array1, a: 2#{block})")
check_allocations(1, 0, "r2k(**nill#{block})")
check_allocations(1, 0, "r2k(*nil, **nill#{block})")
check_allocations(1, 0, "r2k(*array1, **nill#{block})")
check_allocations(1, 0, "r2k(*array1, **empty_hash#{block})")
check_allocations(1, 1, "r2k(*array1, **hash1#{block})")
@ -730,9 +753,9 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(1, 0, "r2k(*array1, **nil#{block})")
check_allocations(1, 0, "r2k(*r2k_empty_array#{block})")
check_allocations(1, 1, "r2k(*r2k_array#{block})")
unless defined?(RubyVM::YJIT.enabled?) && RubyVM::YJIT.enabled?
# YJIT may or may not allocate depending on arch?
check_allocations(1, 1, "r2k(*r2k_array#{block})")
check_allocations(1, 0, "r2k(*r2k_empty_array1#{block})")
check_allocations(1, 1, "r2k(*r2k_array1#{block})")
end
@ -743,6 +766,7 @@ class TestAllocation < Test::Unit::TestCase
check_allocations(<<~RUBY)
def self.keyword(a: nil, b: nil#{block}); end
check_allocations(0, 1, "keyword(*nil, a: empty_array#{block})") # LVAR
check_allocations(0, 1, "keyword(*empty_array, a: empty_array#{block})") # LVAR
check_allocations(0, 1, "->{keyword(*empty_array, a: empty_array#{block})}.call") # DVAR
check_allocations(0, 1, "$x = empty_array; keyword(*empty_array, a: $x#{block})") # GVAR
@ -772,6 +796,9 @@ class TestAllocation < Test::Unit::TestCase
def block
', &block'
end
def only_block
'&block'
end
end
end
@ -837,6 +864,9 @@ class TestAllocation < Test::Unit::TestCase
def block
', &block'
end
def only_block
'&block'
end
end
end
end

6
vm_insnhelper.c
View File

@ -5578,6 +5578,9 @@ vm_concat_to_array(VALUE ary1, VALUE ary2st)
{
/* ary1 must be a newly created array */
const VALUE ary2 = ary2st;
if (NIL_P(ary2)) return ary1;
VALUE tmp2 = rb_check_to_array(ary2);
if (NIL_P(tmp2)) {
@ -5604,6 +5607,9 @@ rb_vm_concat_to_array(VALUE ary1, VALUE ary2st)
static VALUE
vm_splat_array(VALUE flag, VALUE ary)
{
if (NIL_P(ary)) {
return RTEST(flag) ? rb_ary_new() : rb_cArray_empty_frozen;
}
VALUE tmp = rb_check_to_array(ary);
if (NIL_P(tmp)) {
return rb_ary_new3(1, ary);