summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJeremy Evans <[email protected]>2024-08-19 19:00:37 -0700
committer<[email protected]>2024-08-19 19:00:37 -0700
commitea7ceff82cec98d0c419e9807dcb33dcc08b56fa ()
tree2292aa2fe8f97cd82bc68b3b546d7ae2abdb5226
parent6dccb0131e109e480d6b86c98a14ce8e9f2cad4c (diff)
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 abc04e898b627ab37fa9dd5e330f239768778d8b, which was reverted at d56470a27c5a8a2e7aee7a76cea445c2d29c0c59, with the addition of a bug fix and test. Fixes [Bug #20679]
Notes: Merged: https://.com/ruby/ruby/pull/11409 Merged-By: jeremyevans <[email protected]>
-rw-r--r--test/ruby/test_allocation.rb77
-rw-r--r--test/ruby/test_keyword.rb14
-rw-r--r--vm_args.c39
-rw-r--r--vm_insnhelper.c4
4 files changed, 118 insertions, 16 deletions
@@ -2,10 +2,16 @@
require 'test/unit'
class TestAllocation < Test::Unit::TestCase
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?
assert_separately([], <<~RUBY)
$allocations = [0, 0]
$counts = {}
@@ -549,7 +555,8 @@ class TestAllocation < Test::Unit::TestCase
def test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters
check_allocations(<<~RUBY)
- 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})")
@@ -639,7 +646,8 @@ class TestAllocation < Test::Unit::TestCase
def test_nested_argument_forwarding
check_allocations(<<~RUBY)
- 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})")
@@ -766,4 +774,69 @@ class TestAllocation < Test::Unit::TestCase
end
end
end
end
@@ -2848,6 +2848,20 @@ class TestKeywordArguments < Test::Unit::TestCase
assert_equal(1, process(:foo, bar: :baz))
end
def test_top_ruby2_keywords
assert_in_out_err([], <<-INPUT, ["[1, 2, 3]", "{:k=>1}"], [])
def bar(*a, **kw)
@@ -571,6 +571,22 @@ check_kwrestarg(VALUE keyword_hash, unsigned int *kw_flag)
}
}
static int
setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * const iseq,
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;
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) {
// Only duplicate/modify splat array if it will be used
arg_rest_dup(args);
rb_ary_pop(args->rest);
}
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;
}
@@ -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));
}
}
- else if (!IS_ARGS_KW_SPLAT_MUT(ci)) {
/* Convert a hash keyword splat to a new hash unless
* a mutable keyword splat was passed.
*/
keyword_hash = rb_hash_dup(keyword_hash);
}