From 6428ce80f0ef85324a4d0a72ffa4c6fa2db37cdd Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 12 Jul 2024 15:33:59 -0700 Subject: [PATCH] Avoid array allocation for f(*r2k_ary) when def f(x) When calling a method that does not accept a positional splat parameter with a splatted array with a ruby2_keywords flagged hash, there is no need to duplicate the splatted array. Previously, Ruby would duplicate the splatted array and potentially modify it before flattening it to the VM stack Use a similar approach as the f(*ary, **hash) optimization, flattening the splatted array to the VM stack without modifying it, and make any modifications needed to the VM stack. --- test/ruby/test_allocation.rb | 16 ++++++++-------- vm_args.c | 26 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/test/ruby/test_allocation.rb b/test/ruby/test_allocation.rb index 793b041529..60faa44e4f 100644 --- a/test/ruby/test_allocation.rb +++ b/test/ruby/test_allocation.rb @@ -265,7 +265,7 @@ class TestAllocation < Test::Unit::TestCase check_allocations(1, 0, "keyword(*empty_array, *empty_array, **empty_hash#{block})") check_allocations(0, 0, "keyword(*r2k_empty_array#{block})") - check_allocations(1, 0, "keyword(*r2k_array#{block})") + check_allocations(0, 0, "keyword(*r2k_array#{block})") check_allocations(0, 1, "keyword(*empty_array, a: 2, **empty_hash#{block})") check_allocations(0, 1, "keyword(*empty_array, **hash1, **empty_hash#{block})") @@ -291,8 +291,8 @@ class TestAllocation < Test::Unit::TestCase check_allocations(0, 1, "keyword_splat(*empty_array#{block})") check_allocations(1, 1, "keyword_splat(*empty_array, *empty_array, **empty_hash#{block})") - check_allocations(1, 1, "keyword_splat(*r2k_empty_array#{block})") - check_allocations(1, 1, "keyword_splat(*r2k_array#{block})") + check_allocations(0, 1, "keyword_splat(*r2k_empty_array#{block})") + check_allocations(0, 1, "keyword_splat(*r2k_array#{block})") check_allocations(0, 1, "keyword_splat(*empty_array, a: 2, **empty_hash#{block})") check_allocations(0, 1, "keyword_splat(*empty_array, **hash1, **empty_hash#{block})") @@ -318,8 +318,8 @@ class TestAllocation < Test::Unit::TestCase 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})") - check_allocations(1, 1, "keyword_and_keyword_splat(*r2k_empty_array#{block})") - check_allocations(1, 1, "keyword_and_keyword_splat(*r2k_array#{block})") + check_allocations(0, 1, "keyword_and_keyword_splat(*r2k_empty_array#{block})") + check_allocations(0, 1, "keyword_and_keyword_splat(*r2k_array#{block})") check_allocations(0, 1, "keyword_and_keyword_splat(*empty_array, a: 2, **empty_hash#{block})") check_allocations(0, 1, "keyword_and_keyword_splat(*empty_array, **hash1, **empty_hash#{block})") @@ -359,7 +359,7 @@ class TestAllocation < Test::Unit::TestCase check_allocations(1, 1, "required_and_keyword(*array1, *empty_array, **hash1, **empty_hash#{block})") check_allocations(0, 0, "required_and_keyword(*r2k_empty_array1#{block})") - check_allocations(1, 0, "required_and_keyword(*r2k_array1#{block})") + check_allocations(0, 0, "required_and_keyword(*r2k_array1#{block})") check_allocations(0, 1, "required_and_keyword(1, *empty_array, a: 2, **empty_hash#{block})") check_allocations(0, 1, "required_and_keyword(1, *empty_array, **hash1, **empty_hash#{block})") @@ -446,8 +446,8 @@ class TestAllocation < Test::Unit::TestCase check_allocations(1, 1, "required_and_keyword_splat(*array1, *empty_array, a: 2, **empty_hash#{block})") check_allocations(1, 1, "required_and_keyword_splat(*array1, *empty_array, **hash1, **empty_hash#{block})") - check_allocations(1, 1, "required_and_keyword_splat(*r2k_empty_array1#{block})") - check_allocations(1, 1, "required_and_keyword_splat(*r2k_array1#{block})") + check_allocations(0, 1, "required_and_keyword_splat(*r2k_empty_array1#{block})") + check_allocations(0, 1, "required_and_keyword_splat(*r2k_array1#{block})") check_allocations(0, 1, "required_and_keyword_splat(1, *empty_array, a: 2, **empty_hash#{block})") check_allocations(0, 1, "required_and_keyword_splat(1, *empty_array, **hash1, **empty_hash#{block})") diff --git a/vm_args.c b/vm_args.c index abd9cf75f6..84ffd2e983 100644 --- a/vm_args.c +++ b/vm_args.c @@ -734,6 +734,32 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co } given_argc--; } + else if (!ISEQ_BODY(iseq)->param.flags.has_rest) { + // Avoid duping rest when not necessary + // Copy rest elements and converted keyword hash directly to VM stack + const VALUE *argv = RARRAY_CONST_PTR(args->rest); + int j, i=args->argc, rest_len = RARRAY_LENINT(args->rest)-1; + args->argc += rest_len; + if (rest_len) { + CHECK_VM_STACK_OVERFLOW(ec->cfp, rest_len+1); + for (i, j=0; rest_len > 0; rest_len--, i++, j++) { + locals[i] = argv[j]; + } + } + args->rest = Qfalse; + ci_flag &= ~VM_CALL_ARGS_SPLAT; + + if (ISEQ_BODY(iseq)->param.flags.has_kw || ISEQ_BODY(iseq)->param.flags.has_kwrest) { + given_argc--; + keyword_hash = converted_keyword_hash; + } + else { + args->argc += 1; + locals[i] = converted_keyword_hash; + keyword_hash = Qnil; + kw_flag = 0; + } + } else { if (rest_last != converted_keyword_hash) { rest_last = converted_keyword_hash;