Avoid hash allocation for certain proc calls

Previously, proc calls such as:

```ruby
proc{|| }.(**empty_hash)
proc{|b: 1| }.(**r2k_array_with_empty_hash)
```

both allocated hashes unnecessarily, due to two separate code paths.

The first call goes through CALLER_SETUP_ARG/vm_caller_setup_keyword_hash,
and is simple to fix by not duping an empty keyword hash that will be
dropped.

The second case is more involved, in setup_parameters_complex, but is
fixed the exact same way as when the ruby2_keywords hash is not empty,
by flattening the rest array to the VM stack, ignoring the last
element (the empty keyword splat).  Add a flatten_rest_array static
function to handle this case.

Update test_allocation.rb to automatically convert the method call
allocation tests to proc allocation tests, at least for the calls
that can be converted.  With the code changes, all proc call
allocation tests pass, showing that proc calls and method calls
now allocate the same number of objects.

I've audited the allocation tests, and I believe that all of the low
hanging fruit has been collected.  All remaining allocations are
either caller side:

* Positional splat + post argument
* Multiple positional splats
* Literal keywords + keyword splat
* Multiple keyword splats

Or callee side:

* Positional splat parameter
* Keyword splat parameter
* Keyword to positional argument conversion for methods that don't accept keywords
* ruby2_keywords method called with keywords

Reapplies abc04e898b, which was reverted at
d56470a27c, with the addition of a bug fix and
test.

Fixes [Bug #20679]
This commit is contained in:
Jeremy Evans 2024-08-19 19:00:37 -07:00 коммит произвёл GitHub
Родитель 6dccb0131e
Коммит ea7ceff82c
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: B5690EEEBB952194
4 изменённых файлов: 118 добавлений и 16 удалений

Просмотреть файл

@ -2,10 +2,16 @@
require 'test/unit' require 'test/unit'
class TestAllocation < Test::Unit::TestCase class TestAllocation < Test::Unit::TestCase
def munge_checks(checks)
checks
end
def check_allocations(checks) def check_allocations(checks)
dups = checks.split("\n").reject(&:empty?).tally.select{|_,v| v > 1} dups = checks.split("\n").reject(&:empty?).tally.select{|_,v| v > 1}
raise "duplicate checks:\n#{dups.keys.join("\n")}" unless dups.empty? raise "duplicate checks:\n#{dups.keys.join("\n")}" unless dups.empty?
checks = munge_checks(checks)
assert_separately([], <<~RUBY) assert_separately([], <<~RUBY)
$allocations = [0, 0] $allocations = [0, 0]
$counts = {} $counts = {}
@ -549,7 +555,8 @@ class TestAllocation < Test::Unit::TestCase
def test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters def test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters
check_allocations(<<~RUBY) check_allocations(<<~RUBY)
def self.anon_splat_and_anon_keyword_splat(*, **#{block}); t(*, **) end; def self.t(*, **#{block}); end def self.t(*, **#{block}); end
def self.anon_splat_and_anon_keyword_splat(*, **#{block}); t(*, **) end
check_allocations(1, 1, "anon_splat_and_anon_keyword_splat(1, a: 2#{block})") 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})") check_allocations(1, 0, "anon_splat_and_anon_keyword_splat(1, *empty_array, a: 2#{block})")
@ -639,7 +646,8 @@ class TestAllocation < Test::Unit::TestCase
def test_nested_argument_forwarding def test_nested_argument_forwarding
check_allocations(<<~RUBY) check_allocations(<<~RUBY)
def self.argument_forwarding(...); t(...) end; def self.t(...) end def self.t(...) end
def self.argument_forwarding(...); t(...) end
check_allocations(0, 0, "argument_forwarding(1, a: 2#{block})") check_allocations(0, 0, "argument_forwarding(1, a: 2#{block})")
check_allocations(0, 0, "argument_forwarding(1, *empty_array, a: 2#{block})") check_allocations(0, 0, "argument_forwarding(1, *empty_array, a: 2#{block})")
@ -766,4 +774,69 @@ class TestAllocation < Test::Unit::TestCase
end end
end 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 end

Просмотреть файл

@ -2848,6 +2848,20 @@ class TestKeywordArguments < Test::Unit::TestCase
assert_equal(1, process(:foo, bar: :baz)) assert_equal(1, process(:foo, bar: :baz))
end end
def test_ruby2_keywords_bug_20679
c = Class.new do
def self.get(_, _, h, &block)
h[1]
end
ruby2_keywords def get(*args, &block)
self.class.get(*args, &block)
end
end
assert_equal 2, c.new.get(true, {}, 1 => 2)
end
def test_top_ruby2_keywords def test_top_ruby2_keywords
assert_in_out_err([], <<-INPUT, ["[1, 2, 3]", "{:k=>1}"], []) assert_in_out_err([], <<-INPUT, ["[1, 2, 3]", "{:k=>1}"], [])
def bar(*a, **kw) def bar(*a, **kw)

Просмотреть файл

@ -571,6 +571,22 @@ 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 static int
setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * const iseq, setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * const iseq,
struct rb_calling_info *const calling, struct rb_calling_info *const calling,
@ -727,35 +743,32 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
args->rest_dupped = false; args->rest_dupped = false;
if (ignore_keyword_hash_p(rest_last, iseq, &kw_flag, &converted_keyword_hash)) { if (ignore_keyword_hash_p(rest_last, iseq, &kw_flag, &converted_keyword_hash)) {
if (ISEQ_BODY(iseq)->param.flags.has_rest || arg_setup_type != arg_setup_method) { if (ISEQ_BODY(iseq)->param.flags.has_rest) {
// Only duplicate/modify splat array if it will be used // Only duplicate/modify splat array if it will be used
arg_rest_dup(args); arg_rest_dup(args);
rb_ary_pop(args->rest); 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--; given_argc--;
} }
else if (!ISEQ_BODY(iseq)->param.flags.has_rest) { else if (!ISEQ_BODY(iseq)->param.flags.has_rest) {
// Avoid duping rest when not necessary // Avoid duping rest when not necessary
// Copy rest elements and converted keyword hash directly to VM stack // Copy rest elements and converted keyword hash directly to VM stack
const VALUE *argv = RARRAY_CONST_PTR(args->rest); flatten_rest_args(ec, args, locals, &ci_flag);
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) { if (ISEQ_BODY(iseq)->param.flags.has_kw || ISEQ_BODY(iseq)->param.flags.has_kwrest) {
given_argc--; given_argc--;
keyword_hash = converted_keyword_hash; keyword_hash = converted_keyword_hash;
} }
else { else {
locals[args->argc] = converted_keyword_hash;
args->argc += 1; args->argc += 1;
locals[i] = converted_keyword_hash;
keyword_hash = Qnil; keyword_hash = Qnil;
kw_flag = 0; kw_flag = 0;
} }

Просмотреть файл

@ -2739,9 +2739,11 @@ vm_caller_setup_keyword_hash(const struct rb_callinfo *ci, VALUE keyword_hash)
keyword_hash = rb_hash_dup(rb_to_hash_type(keyword_hash)); keyword_hash = rb_hash_dup(rb_to_hash_type(keyword_hash));
} }
} }
else if (!IS_ARGS_KW_SPLAT_MUT(ci)) { else if (!IS_ARGS_KW_SPLAT_MUT(ci) && !RHASH_EMPTY_P(keyword_hash)) {
/* Convert a hash keyword splat to a new hash unless /* Convert a hash keyword splat to a new hash unless
* a mutable keyword splat was passed. * 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); keyword_hash = rb_hash_dup(keyword_hash);
} }