From d56470a27c5a8a2e7aee7a76cea445c2d29c0c59 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 16 Aug 2024 16:47:11 -0700 Subject: [PATCH] Revert "Avoid hash allocation for certain proc calls" This reverts commit abc04e898b627ab37fa9dd5e330f239768778d8b. This caused problems in a Rails test. --- test/ruby/test_allocation.rb | 77 +----------------------------------- vm_args.c | 37 ++++++----------- vm_insnhelper.c | 4 +- 3 files changed, 15 insertions(+), 103 deletions(-) diff --git a/test/ruby/test_allocation.rb b/test/ruby/test_allocation.rb index 9ba01dfcf9..60faa44e4f 100644 --- a/test/ruby/test_allocation.rb +++ b/test/ruby/test_allocation.rb @@ -2,16 +2,10 @@ require 'test/unit' class TestAllocation < Test::Unit::TestCase - def munge_checks(checks) - checks - end - def check_allocations(checks) dups = checks.split("\n").reject(&:empty?).tally.select{|_,v| v > 1} raise "duplicate checks:\n#{dups.keys.join("\n")}" unless dups.empty? - checks = munge_checks(checks) - assert_separately([], <<~RUBY) $allocations = [0, 0] $counts = {} @@ -555,8 +549,7 @@ class TestAllocation < Test::Unit::TestCase def test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters check_allocations(<<~RUBY) - def self.t(*, **#{block}); end - def self.anon_splat_and_anon_keyword_splat(*, **#{block}); t(*, **) end + def self.anon_splat_and_anon_keyword_splat(*, **#{block}); t(*, **) end; def self.t(*, **#{block}); end check_allocations(1, 1, "anon_splat_and_anon_keyword_splat(1, a: 2#{block})") check_allocations(1, 0, "anon_splat_and_anon_keyword_splat(1, *empty_array, a: 2#{block})") @@ -646,8 +639,7 @@ class TestAllocation < Test::Unit::TestCase def test_nested_argument_forwarding check_allocations(<<~RUBY) - def self.t(...) end - def self.argument_forwarding(...); t(...) end + def self.argument_forwarding(...); t(...) end; def self.t(...) end check_allocations(0, 0, "argument_forwarding(1, a: 2#{block})") check_allocations(0, 0, "argument_forwarding(1, *empty_array, a: 2#{block})") @@ -774,69 +766,4 @@ class TestAllocation < Test::Unit::TestCase end end end - - class ProcCall < MethodCall - def munge_checks(checks) - return checks if @no_munge - sub = rep = nil - checks.split("\n").map do |line| - case line - when "singleton_class.send(:ruby2_keywords, :r2k)" - "r2k.ruby2_keywords" - when /\Adef self.([a-z0-9_]+)\((.*)\);(.*)end\z/ - sub = $1 + '(' - rep = $1 + '.(' - "#{$1} = #{$1} = proc{ |#{$2}| #{$3} }" - when /check_allocations/ - line.gsub(sub, rep) - else - line - end - end.join("\n") - end - - # Generic argument forwarding not supported in proc definitions - undef_method :test_argument_forwarding - undef_method :test_nested_argument_forwarding - - # Proc anonymous arguments cannot be used directly - undef_method :test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters - - def test_no_array_allocation_with_splat_and_nonstatic_keywords - @no_munge = true - - check_allocations(<<~RUBY) - keyword = keyword = proc{ |a: nil, b: nil #{block}| } - - 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 - check_allocations(0, 1, "@x = empty_array; keyword.(*empty_array, a: @x#{block})") # IVAR - check_allocations(0, 1, "self.class.const_set(:X, empty_array); keyword.(*empty_array, a: X#{block})") # CONST - check_allocations(0, 1, "keyword.(*empty_array, a: Object::X#{block})") # COLON2 - check_allocations(0, 1, "keyword.(*empty_array, a: ::X#{block})") # COLON3 - check_allocations(0, 1, "T = keyword; #{'B = block' unless block.empty?}; class Object; @@x = X; T.(*X, a: @@x#{', &B' unless block.empty?}) end") # CVAR - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 1#{block})") # INTEGER - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 1.0#{block})") # FLOAT - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 1.0r#{block})") # RATIONAL - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 1.0i#{block})") # IMAGINARY - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 'a'#{block})") # STR - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: :b#{block})") # SYM - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: /a/#{block})") # REGX - check_allocations(0, 1, "keyword.(*empty_array, a: self#{block})") # SELF - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: nil#{block})") # NIL - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: true#{block})") # TRUE - check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: false#{block})") # FALSE - check_allocations(0, 1, "keyword.(*empty_array, a: ->{}#{block})") # LAMBDA - check_allocations(0, 1, "keyword.(*empty_array, a: $1#{block})") # NTH_REF - check_allocations(0, 1, "keyword.(*empty_array, a: $`#{block})") # BACK_REF - RUBY - end - - class WithBlock < self - def block - ', &block' - end - end - end end diff --git a/vm_args.c b/vm_args.c index 63749e1c2c..84ffd2e983 100644 --- a/vm_args.c +++ b/vm_args.c @@ -571,22 +571,6 @@ check_kwrestarg(VALUE keyword_hash, unsigned int *kw_flag) } } -static void -flatten_rest_args(rb_execution_context_t * const ec, struct args_info *args, VALUE * const locals, unsigned int *ci_flag) -{ - 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; -} - static int setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * const iseq, struct rb_calling_info *const calling, @@ -743,24 +727,27 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co args->rest_dupped = false; if (ignore_keyword_hash_p(rest_last, iseq, &kw_flag, &converted_keyword_hash)) { - if (ISEQ_BODY(iseq)->param.flags.has_rest) { + if (ISEQ_BODY(iseq)->param.flags.has_rest || arg_setup_type != arg_setup_method) { // Only duplicate/modify splat array if it will be used arg_rest_dup(args); rb_ary_pop(args->rest); } - else if (arg_setup_type == arg_setup_block && !ISEQ_BODY(iseq)->param.flags.has_kwrest) { - // Avoid hash allocation for empty hashes - // Copy rest elements except empty keyword hash directly to VM stack - flatten_rest_args(ec, args, locals, &ci_flag); - keyword_hash = Qnil; - kw_flag = 0; - } 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 - flatten_rest_args(ec, args, locals, &ci_flag); + 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--; diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 227bfa2004..4ce72ea81f 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -2739,11 +2739,9 @@ vm_caller_setup_keyword_hash(const struct rb_callinfo *ci, VALUE keyword_hash) keyword_hash = rb_hash_dup(rb_to_hash_type(keyword_hash)); } } - else if (!IS_ARGS_KW_SPLAT_MUT(ci) && !RHASH_EMPTY_P(keyword_hash)) { + else if (!IS_ARGS_KW_SPLAT_MUT(ci)) { /* Convert a hash keyword splat to a new hash unless * a mutable keyword splat was passed. - * Skip allocating new hash for empty keyword splat, as empty - * keyword splat will be ignored by both callers. */ keyword_hash = rb_hash_dup(keyword_hash); }