10000 Avoid hash allocation for certain proc calls · ruby/ruby@abc04e8 · GitHub
[go: up one dir, main page]
Skip to content

Commit abc04e8

Browse files
jeremyevansjeremyevans
committed
Avoid hash allocation for certain proc calls
Previous, 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
1 parent 2c6e16e commit abc04e8

File tree

3 files changed

+103
-15
lines changed

3 files changed

+103
-15
lines changed

‎test/ruby/test_allocation.rb

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,16 @@
22
require 'test/unit'
33

44
class TestAllocation < Test::Unit::TestCase
5+
def munge_checks(checks)
6+
checks
7+
end
8+
59
def check_allocations(checks)
610
dups = checks.split("\n").reject(&:empty?).tally.select{|_,v| v > 1}
711
raise "duplicate checks:\n#{dups.keys.join("\n")}" unless dups.empty?
812

13+
checks = munge_checks(checks)
14+
915
assert_separately([], <<~RUBY)
1016
$allocations = [0, 0]
1117
$counts = {}
@@ -549,7 +555,8 @@ def self.anon_splat_and_anon_keyword_splat(*, **#{block}); end
549555

550556
def test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters
551557
check_allocations(<<~RUBY)
552-
def self.anon_splat_and_anon_keyword_splat(*, **#{block}); t(*, **) end; def self.t(*, **#{block}); end
558+
def self.t(*, **#{block}); end
559+
def self.anon_splat_and_anon_keyword_splat(*, **#{block}); t(*, **) end
553560
554561
check_allocations(1, 1, "anon_splat_and_anon_keyword_splat(1, a: 2#{block})")
555562
check_allocations(1, 0, "anon_splat_and_anon_keyword_splat(1, *empty_array, a: 2#{block})")
@@ -639,7 +646,8 @@ def self.argument_forwarding(...); end
639646

640647
def test_nested_argument_forwarding
641648
check_allocations(<<~RUBY)
642-
def self.argument_forwarding(...); t(...) end; def self.t(...) end
649+
def self.t(...) end
650+
def self.argument_forwarding(...); t(...) end
643651
644652
check_allocations(0, 0, "argument_forwarding(1, a: 2#{block})")
645653
check_allocations(0, 0, "argument_forwarding(1, *empty_array, a: 2#{block})")
@@ -766,4 +774,69 @@ def block
766774
end
767775
end
768776
end
777+
778+
class ProcCall < MethodCall
779+
def munge_checks(checks)
780+
return checks if @no_munge
781+
sub = rep = nil
782+
checks.split("\n").map do |line|
783+
case line
784+
when "singleton_class.send(:ruby2_keywords, :r2k)"
785+
"r2k.ruby2_keywords"
786+
when /\Adef self.([a-z0-9_]+)\((.*)\);(.*)end\z/
787+
sub = $1 + '('
788+
rep = $1 + '.('
789+
"#{$1} = #{$1} = proc{ |#{$2}| #{$3} }"
790+
when /check_allocations/
791+
line.gsub(sub, rep)
792+
else
793+
line
794+
end
795+
end.join("\n")
796+
end
797+
798+
# Generic argument forwarding not supported in proc definitions
799+
undef_method :test_argument_forwarding
800+
undef_method :test_nested_argument_forwarding
801+
802+
# Proc anonymous arguments cannot be used directly
803+
undef_method :test_nested_anonymous_splat_and_anonymous_keyword_splat_parameters
804+
805+
def test_no_array_allocation_with_splat_and_nonstatic_keywords
806+
@no_munge = true
807+
808+
check_allocations(<<~RUBY)
809+
keyword = keyword = proc{ |a: nil, b: nil #{block}| }
810+
811+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array#{block})") # LVAR
812+
check_allocations(0, 1, "->{keyword.(*empty_array, a: empty_array#{block})}.call") # DVAR
813+
check_allocations(0, 1, "$x = empty_array; keyword.(*empty_array, a: $x#{block})") # GVAR
814+
check_allocations(0, 1, "@x = empty_array; keyword.(*empty_array, a: @x#{block})") # IVAR
815+
check_allocations(0, 1, "self.class.const_set(:X, empty_array); keyword.(*empty_array, a: X#{block})") # CONST
816+
check_allocations(0, 1, "keyword.(*empty_array, a: Object::X#{block})") # COLON2
817+
check_allocations(0, 1, "keyword.(*empty_array, a: ::X#{block})") # COLON3
818+
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
819+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 1#{block})") # INTEGER
820+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 1.0#{block})") # FLOAT
821+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 1.0r#{block})") # RATIONAL
822+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 1.0i#{block})") # IMAGINARY
823+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: 'a'#{block})") # STR
824+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: :b#{block})") # SYM
825+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: /a/#{block})") # REGX
826+
check_allocations(0, 1, "keyword.(*empty_array, a: self#{block})") # SELF
827+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: nil#{block})") # NIL
828+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: true#{block})") # TRUE
829+
check_allocations(0, 1, "keyword.(*empty_array, a: empty_array, b: false#{block})") # FALSE
830+
check_allocations(0, 1, "keyword.(*empty_array, a: ->{}#{block})") # LAMBDA
831+
check_allocations(0, 1, "keyword.(*empty_array, a: $1#{block})") # NTH_REF
832+
check_allocations(0, 1, "keyword.(*empty_array, a: $`#{block})") # BACK_REF
833+
RUBY
834+
end
835+
836+
class WithBlock < self
837+
def block
838+
', &block'
839+
end
840+
end
841+
end
769842
end

‎vm_args.c

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,22 @@ check_kwrestarg(VALUE keyword_hash, unsigned int *kw_flag)
571571
}
572572
}
573573

574+
static void
575+
flatten_rest_args(rb_execution_context_t * const ec, struct args_info *args, VALUE * const locals, unsigned int *ci_flag)
576+
{
577+
const VALUE *argv = RARRAY_CONST_PTR(args->rest);
578+
int j, i=args->argc, rest_len = RARRAY_LENINT(args->rest)-1;
579+
args->argc += rest_len;
580+
if (rest_len) {
581+
CHECK_VM_STACK_OVERFLOW(ec->cfp, rest_len+1);
582+
for (i, j=0; rest_len > 0; rest_len--, i++, j++) {
583+
locals[i] = argv[j];
584+
}
585+
}
586+
args->rest = Qfalse;
587+
*ci_flag &= ~VM_CALL_ARGS_SPLAT;
588+
}
589+
574590
static int
575591
setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * const iseq,
576592
struct rb_calling_info *const calling,
@@ -727,27 +743,24 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
727743
args->rest_dupped = false;
728744

729745
if (ignore_keyword_hash_p(rest_last, iseq, &kw_flag, &converted_keyword_hash)) {
730-
if (ISEQ_BODY(iseq)->param.flags.has_rest || arg_setup_type != arg_setup_method) {
746+
if (ISEQ_BODY(iseq)->param.flags.has_rest) {
731747
// Only duplicate/modify splat array if it will be used
732748
arg_rest_dup(args);
733749
rb_ary_pop(args->rest);
734750
}
751+
else if (arg_setup_type == arg_setup_block && !ISEQ_BODY(iseq)->param.flags.has_kwrest) {
752+
// Avoid hash allocation for empty hashes
753+
// Copy rest elements except empty keyword hash directly to VM stack
754+
flatten_rest_args(ec, args, locals, &ci_flag);
755+
keyword_hash = Qnil;
756+
10000 kw_flag = 0;
757+
}
735758
given_argc--;
736759
}
737760
else if (!ISEQ_BODY(iseq)->param.flags.has_rest) {
738761
// Avoid duping rest when not necessary
739762
// Copy rest elements and converted keyword hash directly to VM stack
740-
const VALUE *argv = RARRAY_CONST_PTR(args->rest);
741-
int j, i=args->argc, rest_len = RARRAY_LENINT(args->rest)-1;
742-
args->argc += rest_len;
743-
if (rest_len) {
744-
CHECK_VM_STACK_OVERFLOW(ec->cfp, rest_len+1);
745-
for (i, j=0; rest_len > 0; rest_len--, i++, j++) {
746-
locals[i] = argv[j];
747-
}
748-
}
749-
args->rest = Qfalse;
750-
ci_flag &= ~VM_CALL_ARGS_SPLAT;
763+
flatten_rest_args(ec, args, locals, &ci_flag);
751764

752765
if (ISEQ_BODY(iseq)->param.flags.has_kw || ISEQ_BODY(iseq)->param.flags.has_kwrest) {
753766
given_argc--;

‎vm_insnhelper.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2739,9 +2739,11 @@ vm_caller_setup_keyword_hash(const struct rb_callinfo *ci, VALUE keyword_hash)
27392739
keyword_hash = rb_hash_dup(rb_to_hash_type(keyword_hash));
27402740
}
27412741
}
2742-
else if (!IS_ARGS_KW_SPLAT_MUT(ci)) {
2742+
else if (!IS_ARGS_KW_SPLAT_MUT(ci) && !RHASH_EMPTY_P(keyword_hash)) {
27432743
/* Convert a hash keyword splat to a new hash unless
27442744
* a mutable keyword splat was passed.
2745+
* Skip allocating new hash for empty keyword splat, as empty
2746+
* keyword splat will be ignored by both callers.
27452747
*/
27462748
keyword_hash = rb_hash_dup(keyword_hash);
27472749
}

0 commit comments

Comments
 (0)
0