summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJeremy Evans <code@jeremyevans.net>2020-02-24 12:05:07 -0800
committerJeremy Evans <code@jeremyevans.net>2020-03-17 12:09:43 -0700
commitd2c41b1bff1f3102544bb0d03d4e82356d034d33 (patch)
tree468467ff804e7deb9eca40b7d022c996e929785f
parentac04b778c12120ab91986822b71edf16fea61465 (diff)
Reduce allocations for keyword argument hashes
Previously, passing a keyword splat to a method always allocated a hash on the caller side, and accepting arbitrary keywords in a method allocated a separate hash on the callee side. Passing explicit keywords to a method that accepted a keyword splat did not allocate a hash on the caller side, but resulted in two hashes allocated on the callee side. This commit makes passing a single keyword splat to a method not allocate a hash on the caller side. Passing multiple keyword splats or a mix of explicit keywords and a keyword splat still generates a hash on the caller side. On the callee side, if arbitrary keywords are not accepted, it does not allocate a hash. If arbitrary keywords are accepted, it will allocate a hash, but this commit uses a callinfo flag to indicate whether the caller already allocated a hash, and if so, the callee can use the passed hash without duplicating it. So this commit should make it so that a maximum of a single hash is allocated during method calls. To set the callinfo flag appropriately, method call argument compilation checks if only a single keyword splat is given. If only one keyword splat is given, the VM_CALL_KW_SPLAT_MUT callinfo flag is not set, since in that case the keyword splat is passed directly and not mutable. If more than one splat is used, a new hash needs to be generated on the caller side, and in that case the callinfo flag is set, indicating the keyword splat is mutable by the callee. In compile_hash, used for both hash and keyword argument compilation, if compiling keyword arguments and only a single keyword splat is used, pass the argument directly. On the caller side, in vm_args.c, the callinfo flag needs to be recognized and handled. Because the keyword splat argument may not be a hash, it needs to be converted to a hash first if not. Then, unless the callinfo flag is set, the hash needs to be duplicated. The temporary copy of the callinfo flag, kw_flag, is updated if a hash was duplicated, to prevent the need to duplicate it again. If we are converting to a hash or duplicating a hash, we need to update the argument array, which can including duplicating the positional splat array if one was passed. CALLER_SETUP_ARG and a couple other places needs to be modified to handle similar issues for other types of calls. This includes fairly comprehensive tests for different ways keywords are handled internally, checking that you get equal results but that keyword splats on the caller side result in distinct objects for keyword rest parameters. Included are benchmarks for keyword argument calls. Brief results when compiled without optimization: def kw(a: 1) a end def kws(**kw) kw end h = {a: 1} kw(a: 1) # about same kw(**h) # 2.37x faster kws(a: 1) # 1.30x faster kws(**h) # 2.19x faster kw(a: 1, **h) # 1.03x slower kw(**h, **h) # about same kws(a: 1, **h) # 1.16x faster kws(**h, **h) # 1.14x faster
Notes
Notes: Merged: https://github.com/ruby/ruby/pull/2945
-rw-r--r--NEWS.md5
-rw-r--r--benchmark/keyword_arguments.yml13
-rw-r--r--compile.c62
-rw-r--r--hash.c2
-rw-r--r--iseq.c1
-rw-r--r--test/ruby/test_keyword.rb212
-rw-r--r--vm.c13
-rw-r--r--vm_args.c85
-rw-r--r--vm_callinfo.h6
-rw-r--r--vm_insnhelper.c27
-rw-r--r--vm_insnhelper.h1
11 files changed, 368 insertions, 59 deletions
diff --git a/NEWS.md b/NEWS.md
index 3fabf7b331..7b7427d218 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -139,6 +139,11 @@ Excluding feature bug fixes.
## Implementation improvements
+* The number of hashes allocated when using a keyword splat in
+ a method call has been reduced to a maximum of 1, and passing
+ a keyword splat to a method that accepts specific keywords
+ does not allocate a hash.
+
## Miscellaneous changes
* Methods using `ruby2_keywords` will no longer keep empty keyword
diff --git a/benchmark/keyword_arguments.yml b/benchmark/keyword_arguments.yml
new file mode 100644
index 0000000000..fce6bce0b8
--- /dev/null
+++ b/benchmark/keyword_arguments.yml
@@ -0,0 +1,13 @@
+prelude: |
+ h = {a: 1}
+ def kw(a: 1) a end
+ def kws(**kw) kw end
+benchmark:
+ kw_to_kw: "kw(a: 1)"
+ kw_splat_to_kw: "kw(**h)"
+ kw_to_kw_splat: "kws(a: 1)"
+ kw_splat_to_kw_splat: "kws(**h)"
+ kw_and_splat_to_kw: "kw(a: 1, **h)"
+ kw_splats_to_kw: "kw(**h, **h)"
+ kw_and_splat_to_kw_splat: "kws(a: 1, **h)"
+ kw_splats_to_kw_splat: "kws(**h, **h)"
diff --git a/compile.c b/compile.c
index f55c76f591..efca7179f3 100644
--- a/compile.c
+++ b/compile.c
@@ -3918,20 +3918,27 @@ compile_keyword_arg(rb_iseq_t *iseq, LINK_ANCHOR *const ret,
if (root_node->nd_head && nd_type(root_node->nd_head) == NODE_LIST) {
const NODE *node = root_node->nd_head;
+ int seen_nodes = 0;
while (node) {
const NODE *key_node = node->nd_head;
+ seen_nodes++;
assert(nd_type(node) == NODE_LIST);
- if (!key_node) {
- if (flag) *flag |= VM_CALL_KW_SPLAT;
- return FALSE;
- }
- else if (nd_type(key_node) == NODE_LIT && RB_TYPE_P(key_node->nd_lit, T_SYMBOL)) {
+ if (key_node && nd_type(key_node) == NODE_LIT && RB_TYPE_P(key_node->nd_lit, T_SYMBOL)) {
/* can be keywords */
}
else {
- if (flag) *flag |= VM_CALL_KW_SPLAT;
+ if (flag) {
+ *flag |= VM_CALL_KW_SPLAT;
+ if (seen_nodes > 1 || node->nd_next->nd_next) {
+ /* A new hash will be created for the keyword arguments
+ * in this case, so mark the method as passing mutable
+ * keyword splat.
+ */
+ *flag |= VM_CALL_KW_SPLAT_MUT;
+ }
+ }
return FALSE;
}
node = node->nd_next; /* skip value node */
@@ -4312,31 +4319,47 @@ compile_hash(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *node, int popp
if (empty_kw) {
if (only_kw && method_call_keywords) {
- /* **{} appears at the last, so it won't be modified.
+ /* **{} appears at the only keyword argument in method call,
+ * so it won't be modified.
* kw is a special NODE_LIT that contains a special empty hash,
- * so this emits: putobject {}
+ * so this emits: putobject {}.
+ * This is only done for method calls and not for literal hashes,
+ * because literal hashes should always result in a new hash.
*/
NO_CHECK(COMPILE(ret, "keyword splat", kw));
}
else if (first_kw) {
- /* **{} appears at the first, so it may be modified.
+ /* **{} appears as the first keyword argument, so it may be modified.
* We need to create a fresh hash object.
*/
ADD_INSN1(ret, line, newhash, INT2FIX(0));
}
+ /* Any empty keyword splats that are not the first can be ignored.
+ * since merging an empty hash into the existing hash is the same
+ * as not merging it. */
}
else {
- /* This is not empty hash: **{k:1}.
- * We need to clone the hash (if first), or merge the hash to
- * the accumulated hash (if not first).
- */
- ADD_INSN1(ret, line, putspecialobject, INT2FIX(VM_SPECIAL_OBJECT_VMCORE));
- if (first_kw) ADD_INSN1(ret, line, newhash, INT2FIX(0));
- else ADD_INSN(ret, line, swap);
+ if (only_kw && method_call_keywords) {
+ /* **kw is only keyword argument in method call.
+ * Use directly. This will be not be flagged as mutable.
+ * This is only done for method calls and not for literal hashes,
+ * because literal hashes should always result in a new hash.
+ */
+ NO_CHECK(COMPILE(ret, "keyword splat", kw));
+ }
+ else {
+ /* There is more than one keyword argument, or this is not a method
+ * call. In that case, we need to add an empty hash (if first keyword),
+ * or merge the hash to the accumulated hash (if not the first keyword).
+ */
+ ADD_INSN1(ret, line, putspecialobject, INT2FIX(VM_SPECIAL_OBJECT_VMCORE));
+ if (first_kw) ADD_INSN1(ret, line, newhash, INT2FIX(0));
+ else ADD_INSN(ret, line, swap);
- NO_CHECK(COMPILE(ret, "keyword splat", kw));
+ NO_CHECK(COMPILE(ret, "keyword splat", kw));
- ADD_SEND(ret, line, id_core_hash_merge_kwd, INT2FIX(2));
+ ADD_SEND(ret, line, id_core_hash_merge_kwd, INT2FIX(2));
+ }
}
first_chunk = 0;
@@ -7808,10 +7831,10 @@ iseq_compile_each0(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *node, in
if (local_body->param.flags.has_kwrest) {
int idx = local_body->local_table_size - local_kwd->rest_start;
ADD_GETLOCAL(args, line, idx, lvar_level);
- ADD_SEND (args, line, rb_intern("dup"), INT2FIX(0));
}
else {
ADD_INSN1(args, line, newhash, INT2FIX(0));
+ flag |= VM_CALL_KW_SPLAT_MUT;
}
for (i = 0; i < local_kwd->num; ++i) {
ID id = local_kwd->table[i];
@@ -7831,7 +7854,6 @@ iseq_compile_each0(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *node, in
int idx = local_body->local_table_size - local_kwd->rest_start;
ADD_GETLOCAL(args, line, idx, lvar_level);
- ADD_SEND (args, line, rb_intern("dup"), INT2FIX(0));
if (local_body->param.flags.has_rest) {
ADD_INSN1(args, line, newarray, INT2FIX(1));
ADD_INSN (args, line, concatarray);
diff --git a/hash.c b/hash.c
index 864de0a480..b824414d23 100644
--- a/hash.c
+++ b/hash.c
@@ -1868,7 +1868,7 @@ rb_hash_s_create(int argc, VALUE *argv, VALUE klass)
return hash;
}
-VALUE
+MJIT_FUNC_EXPORTED VALUE
rb_to_hash_type(VALUE hash)
{
return rb_convert_type_with_id(hash, T_HASH, "Hash", idTo_hash);
diff --git a/iseq.c b/iseq.c
index e1c18e7a8a..36f8ef6ccc 100644
--- a/iseq.c
+++ b/iseq.c
@@ -1992,6 +1992,7 @@ rb_insn_operand_intern(const rb_iseq_t *iseq,
CALL_FLAG(ZSUPER);
CALL_FLAG(KWARG);
CALL_FLAG(KW_SPLAT);
+ CALL_FLAG(KW_SPLAT_MUT);
CALL_FLAG(OPT_SEND); /* maybe not reachable */
rb_ary_push(ary, rb_ary_join(flags, rb_str_new2("|")));
}
diff --git a/test/ruby/test_keyword.rb b/test/ruby/test_keyword.rb
index 48d6a7336a..8ec0636d5c 100644
--- a/test/ruby/test_keyword.rb
+++ b/test/ruby/test_keyword.rb
@@ -190,6 +190,218 @@ class TestKeywordArguments < Test::Unit::TestCase
assert_equal(["bar", 111111], f[str: "bar", num: 111111])
end
+ def test_keyword_splat_new
+ kw = {}
+ h = {a: 1}
+
+ def self.assert_equal_not_same(kw, res)
+ assert_instance_of(Hash, res)
+ assert_equal(kw, res)
+ assert_not_same(kw, res)
+ end
+
+ def self.y(**kw) kw end
+ m = method(:y)
+ assert_equal(false, y(**{}).frozen?)
+ assert_equal_not_same(kw, y(**kw))
+ assert_equal_not_same(h, y(**h))
+ assert_equal(false, send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, send(:y, **kw))
+ assert_equal_not_same(h, send(:y, **h))
+ assert_equal(false, public_send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, public_send(:y, **kw))
+ assert_equal_not_same(h, public_send(:y, **h))
+ assert_equal(false, m.(**{}).frozen?)
+ assert_equal_not_same(kw, m.(**kw))
+ assert_equal_not_same(h, m.(**h))
+ assert_equal(false, m.send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, **kw))
+ assert_equal_not_same(h, m.send(:call, **h))
+
+ m = method(:send)
+ assert_equal(false, m.(:y, **{}).frozen?)
+ assert_equal_not_same(kw, m.(:y, **kw))
+ assert_equal_not_same(h, m.(:y, **h))
+ assert_equal(false, m.send(:call, :y, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, :y, **kw))
+ assert_equal_not_same(h, m.send(:call, :y, **h))
+
+ singleton_class.send(:remove_method, :y)
+ define_singleton_method(:y) { |**kw| kw }
+ m = method(:y)
+ assert_equal(false, y(**{}).frozen?)
+ assert_equal_not_same(kw, y(**kw))
+ assert_equal_not_same(h, y(**h))
+ assert_equal(false, send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, send(:y, **kw))
+ assert_equal_not_same(h, send(:y, **h))
+ assert_equal(false, public_send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, public_send(:y, **kw))
+ assert_equal_not_same(h, public_send(:y, **h))
+ assert_equal(false, m.(**{}).frozen?)
+ assert_equal_not_same(kw, m.(**kw))
+ assert_equal_not_same(h, m.(**h))
+ assert_equal(false, m.send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, **kw))
+ assert_equal_not_same(h, m.send(:call, **h))
+
+ y = lambda { |**kw| kw }
+ m = y.method(:call)
+ assert_equal(false, y.(**{}).frozen?)
+ assert_equal_not_same(kw, y.(**kw))
+ assert_equal_not_same(h, y.(**h))
+ assert_equal(false, y.send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, y.send(:call, **kw))
+ assert_equal_not_same(h, y.send(:call, **h))
+ assert_equal(false, y.public_send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, y.public_send(:call, **kw))
+ assert_equal_not_same(h, y.public_send(:call, **h))
+ assert_equal(false, m.(**{}).frozen?)
+ assert_equal_not_same(kw, m.(**kw))
+ assert_equal_not_same(h, m.(**h))
+ assert_equal(false, m.send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, **kw))
+ assert_equal_not_same(h, m.send(:call, **h))
+
+ y = :y.to_proc
+ m = y.method(:call)
+ assert_equal(false, y.(self, **{}).frozen?)
+ assert_equal_not_same(kw, y.(self, **kw))
+ assert_equal_not_same(h, y.(self, **h))
+ assert_equal(false, y.send(:call, self, **{}).frozen?)
+ assert_equal_not_same(kw, y.send(:call, self, **kw))
+ assert_equal_not_same(h, y.send(:call, self, **h))
+ assert_equal(false, y.public_send(:call, self, **{}).frozen?)
+ assert_equal_not_same(kw, y.public_send(:call, self, **kw))
+ assert_equal_not_same(h, y.public_send(:call, self, **h))
+ assert_equal(false, m.(self, **{}).frozen?)
+ assert_equal_not_same(kw, m.(self, **kw))
+ assert_equal_not_same(h, m.(self, **h))
+ assert_equal(false, m.send(:call, self, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, self, **kw))
+ assert_equal_not_same(h, m.send(:call, self, **h))
+
+ c = Class.new do
+ def y(**kw) kw end
+ end
+ o = c.new
+ def o.y(**kw) super end
+ m = o.method(:y)
+ assert_equal(false, o.y(**{}).frozen?)
+ assert_equal_not_same(kw, o.y(**kw))
+ assert_equal_not_same(h, o.y(**h))
+ assert_equal(false, o.send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, o.send(:y, **kw))
+ assert_equal_not_same(h, o.send(:y, **h))
+ assert_equal(false, o.public_send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, o.public_send(:y, **kw))
+ assert_equal_not_same(h, o.public_send(:y, **h))
+ assert_equal(false, m.(**{}).frozen?)
+ assert_equal_not_same(kw, m.(**kw))
+ assert_equal_not_same(h, m.(**h))
+ assert_equal(false, m.send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, **kw))
+ assert_equal_not_same(h, m.send(:call, **h))
+
+ o.singleton_class.send(:remove_method, :y)
+ def o.y(**kw) super(**kw) end
+ assert_equal(false, o.y(**{}).frozen?)
+ assert_equal_not_same(kw, o.y(**kw))
+ assert_equal_not_same(h, o.y(**h))
+ assert_equal(false, o.send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, o.send(:y, **kw))
+ assert_equal_not_same(h, o.send(:y, **h))
+ assert_equal(false, o.public_send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, o.public_send(:y, **kw))
+ assert_equal_not_same(h, o.public_send(:y, **h))
+ assert_equal(false, m.(**{}).frozen?)
+ assert_equal_not_same(kw, m.(**kw))
+ assert_equal_not_same(h, m.(**h))
+ assert_equal(false, m.send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, **kw))
+ assert_equal_not_same(h, m.send(:call, **h))
+
+ c = Class.new do
+ def method_missing(_, **kw) kw end
+ end
+ o = c.new
+ def o.y(**kw) super end
+ m = o.method(:y)
+ assert_equal(false, o.y(**{}).frozen?)
+ assert_equal_not_same(kw, o.y(**kw))
+ assert_equal_not_same(h, o.y(**h))
+ assert_equal(false, o.send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, o.send(:y, **kw))
+ assert_equal_not_same(h, o.send(:y, **h))
+ assert_equal(false, o.public_send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, o.public_send(:y, **kw))
+ assert_equal_not_same(h, o.public_send(:y, **h))
+ assert_equal(false, m.(**{}).frozen?)
+ assert_equal_not_same(kw, m.(**kw))
+ assert_equal_not_same(h, m.(**h))
+ assert_equal(false, m.send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, **kw))
+ assert_equal_not_same(h, m.send(:call, **h))
+
+ o.singleton_class.send(:remove_method, :y)
+ def o.y(**kw) super(**kw) end
+ assert_equal(false, o.y(**{}).frozen?)
+ assert_equal_not_same(kw, o.y(**kw))
+ assert_equal_not_same(h, o.y(**h))
+ assert_equal(false, o.send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, o.send(:y, **kw))
+ assert_equal_not_same(h, o.send(:y, **h))
+ assert_equal(false, o.public_send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, o.public_send(:y, **kw))
+ assert_equal_not_same(h, o.public_send(:y, **h))
+ assert_equal(false, m.(**{}).frozen?)
+ assert_equal_not_same(kw, m.(**kw))
+ assert_equal_not_same(h, m.(**h))
+ assert_equal(false, m.send(:call, **{}).frozen?)
+ assert_equal_not_same(kw, m.send(:call, **kw))
+ assert_equal_not_same(h, m.send(:call, **h))
+
+ c = Class.new do
+ attr_reader :kw
+ def initialize(**kw) @kw = kw end
+ end
+ m = c.method(:new)
+ assert_equal(false, c.new(**{}).kw.frozen?)
+ assert_equal_not_same(kw, c.new(**kw).kw)
+ assert_equal_not_same(h, c.new(**h).kw)
+ assert_equal(false, c.send(:new, **{}).kw.frozen?)
+ assert_equal_not_same(kw, c.send(:new, **kw).kw)
+ assert_equal_not_same(h, c.send(:new, **h).kw)
+ assert_equal(false, c.public_send(:new, **{}).kw.frozen?)
+ assert_equal_not_same(kw, c.public_send(:new, **kw).kw)
+ assert_equal_not_same(h, c.public_send(:new, **h).kw)
+ assert_equal(false, m.(**{}).kw.frozen?)
+ assert_equal_not_same(kw, m.(**kw).kw)
+ assert_equal_not_same(h, m.(**h).kw)
+ assert_equal(false, m.send(:call, **{}).kw.frozen?)
+ assert_equal_not_same(kw, m.send(:call, **kw).kw)
+ assert_equal_not_same(h, m.send(:call, **h).kw)
+
+ singleton_class.send(:attr_writer, :y)
+ m = method(:y=)
+ assert_equal_not_same(h, send(:y=, **h))
+ assert_equal_not_same(h, public_send(:y=, **h))
+ assert_equal_not_same(h, m.(**h))
+ assert_equal_not_same(h, m.send(:call, **h))
+
+ singleton_class.send(:remove_method, :y)
+ def self.method_missing(_, **kw) kw end
+ assert_equal(false, y(**{}).frozen?)
+ assert_equal_not_same(kw, y(**kw))
+ assert_equal_not_same(h, y(**h))
+ assert_equal(false, send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, send(:y, **kw))
+ assert_equal_not_same(h, send(:y, **h))
+ assert_equal(false, public_send(:y, **{}).frozen?)
+ assert_equal_not_same(kw, public_send(:y, **kw))
+ assert_equal_not_same(h, public_send(:y, **h))
+ end
+
def test_regular_kwsplat
kw = {}
h = {:a=>1}
diff --git a/vm.c b/vm.c
index 03dfda3ce0..8441c9216f 100644
--- a/vm.c
+++ b/vm.c
@@ -1223,9 +1223,16 @@ invoke_block_from_c_proc(rb_execution_context_t *ec, const rb_proc_t *proc,
case block_type_iseq:
return invoke_iseq_block_from_c(ec, &block->as.captured, self, argc, argv, kw_splat, passed_block_handler, NULL, is_lambda, me);
case block_type_ifunc:
- if (kw_splat == 1 && RHASH_EMPTY_P(argv[argc-1])) {
- argc--;
- kw_splat = 2;
+ if (kw_splat == 1) {
+ VALUE keyword_hash = argv[argc-1];
+ if (!RB_TYPE_P(keyword_hash, T_HASH)) {
+ keyword_hash = rb_to_hash_type(keyword_hash);
+ }
+ if (RHASH_EMPTY_P(keyword_hash)) {
+ argc--;
+ } else {
+ ((VALUE *)argv)[argc-1] = rb_hash_dup(keyword_hash);
+ }
}
return vm_yield_with_cfunc(ec, &block->as.captured, self, argc, argv, kw_splat, passed_block_handler, me);
case block_type_symbol:
diff --git a/vm_args.c b/vm_args.c
index 50bb02311d..5e46231b95 100644
--- a/vm_args.c
+++ b/vm_args.c
@@ -400,9 +400,15 @@ args_setup_kw_parameters(rb_execution_context_t *const ec, const rb_iseq_t *cons
}
static inline void
-args_setup_kw_rest_parameter(VALUE keyword_hash, VALUE *locals)
+args_setup_kw_rest_parameter(VALUE keyword_hash, VALUE *locals, int kw_flag)
{
- locals[0] = NIL_P(keyword_hash) ? rb_hash_new() : rb_hash_dup(keyword_hash);
+ if (NIL_P(keyword_hash)) {
+ keyword_hash = rb_hash_new();
+ }
+ else if (!(kw_flag & VM_CALL_KW_SPLAT_MUT)) {
+ keyword_hash = rb_hash_dup(keyword_hash);
+ }
+ locals[0] = keyword_hash;
}
static inline void
@@ -429,11 +435,20 @@ fill_keys_values(st_data_t key, st_data_t val, st_data_t ptr)
}
static inline int
-ignore_keyword_hash_p(VALUE keyword_hash, const rb_iseq_t * const iseq)
+ignore_keyword_hash_p(VALUE keyword_hash, const rb_iseq_t * const iseq, unsigned int * kw_flag, VALUE * converted_keyword_hash)
{
+ if (!RB_TYPE_P(keyword_hash, T_HASH)) {
+ keyword_hash = rb_to_hash_type(keyword_hash);
+ }
+ if (!(*kw_flag & VM_CALL_KW_SPLAT_MUT) &&
+ (iseq->body->param.flags.has_kwrest ||
+ iseq->body->param.flags.ruby2_keywords)) {
+ *kw_flag |= VM_CALL_KW_SPLAT_MUT;
+ keyword_hash = rb_hash_dup(keyword_hash);
+ }
+ *converted_keyword_hash = keyword_hash;
return !(iseq->body->param.flags.has_kw) &&
!(iseq->body->param.flags.has_kwrest) &&
- RB_TYPE_P(keyword_hash, T_HASH) &&
RHASH_EMPTY_P(keyword_hash);
}
@@ -446,13 +461,14 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
const int min_argc = iseq->body->param.lead_num + iseq->body->param.post_num;
const int max_argc = (iseq->body->param.flags.has_rest == FALSE) ? min_argc + iseq->body->param.opt_num : UNLIMITED_ARGUMENTS;
int given_argc;
- unsigned int kw_flag = vm_ci_flag(ci) & (VM_CALL_KWARG | VM_CALL_KW_SPLAT);
+ unsigned int kw_flag = vm_ci_flag(ci) & (VM_CALL_KWARG | VM_CALL_KW_SPLAT | VM_CALL_KW_SPLAT_MUT);
int opt_pc = 0, allow_autosplat = !kw_flag;
struct args_info args_body, *args;
VALUE keyword_hash = Qnil;
VALUE * const orig_sp = ec->cfp->sp;
unsigned int i;
VALUE flag_keyword_hash = 0;
+ VALUE converted_keyword_hash = 0;
vm_check_canary(ec, orig_sp);
/*
@@ -494,7 +510,7 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
else {
args->kw_argv = NULL;
given_argc = args_kw_argv_to_hash(args);
- kw_flag |= VM_CALL_KW_SPLAT;
+ kw_flag |= VM_CALL_KW_SPLAT | VM_CALL_KW_SPLAT_MUT;
}
}
else {
@@ -515,7 +531,7 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
if (RB_TYPE_P(rest_last, T_HASH) &&
(((struct RHash *)rest_last)->basic.flags & RHASH_PASS_AS_KEYWORDS)) {
rest_last = rb_hash_dup(rest_last);
- kw_flag |= VM_CALL_KW_SPLAT;
+ kw_flag |= VM_CALL_KW_SPLAT | VM_CALL_KW_SPLAT_MUT;
}
else {
rest_last = 0;
@@ -523,38 +539,53 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
}
if (kw_flag & VM_CALL_KW_SPLAT) {
- if (len > 0 && ignore_keyword_hash_p(rest_last, iseq)) {
+ if (ignore_keyword_hash_p(rest_last, iseq, &kw_flag, &converted_keyword_hash)) {
arg_rest_dup(args);
rb_ary_pop(args->rest);
given_argc--;
- kw_flag &= ~VM_CALL_KW_SPLAT;
+ kw_flag &= ~(VM_CALL_KW_SPLAT | VM_CALL_KW_SPLAT_MUT);
}
- else if (iseq->body->param.flags.ruby2_keywords && rest_last) {
- flag_keyword_hash = rest_last;
- }
- else if (iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest) {
- arg_rest_dup(args);
- rb_ary_pop(args->rest);
- given_argc--;
- keyword_hash = rest_last;
+ else {
+ if (rest_last != converted_keyword_hash) {
+ rest_last = converted_keyword_hash;
+ arg_rest_dup(args);
+ RARRAY_ASET(args->rest, len - 1, rest_last);
+ }
+
+ if (iseq->body->param.flags.ruby2_keywords && rest_last) {
+ flag_keyword_hash = rest_last;
+ }
+ else if (iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest) {
+ arg_rest_dup(args);
+ rb_ary_pop(args->rest);
+ given_argc--;
+ keyword_hash = rest_last;
+ }
}
}
}
else {
if (kw_flag & VM_CALL_KW_SPLAT) {
VALUE last_arg = args->argv[args->argc-1];
- if (ignore_keyword_hash_p(last_arg, iseq)) {
+ if (ignore_keyword_hash_p(last_arg, iseq, &kw_flag, &converted_keyword_hash)) {
args->argc--;
given_argc--;
- kw_flag &= ~VM_CALL_KW_SPLAT;
+ kw_flag &= ~(VM_CALL_KW_SPLAT | VM_CALL_KW_SPLAT_MUT);
}
- else if (iseq->body->param.flags.ruby2_keywords) {
- flag_keyword_hash = last_arg;
- }
- else if (iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest) {
- args->argc--;
- given_argc--;
- keyword_hash = last_arg;
+ else {
+ if (last_arg != converted_keyword_hash) {
+ last_arg = converted_keyword_hash;
+ args->argv[args->argc-1] = last_arg;
+ }
+
+ if (iseq->body->param.flags.ruby2_keywords) {
+ flag_keyword_hash = last_arg;
+ }
+ else if (iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest) {
+ args->argc--;
+ given_argc--;
+ keyword_hash = last_arg;
+ }
}
}
args->rest = Qfalse;
@@ -651,7 +682,7 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
}
}
else if (iseq->body->param.flags.has_kwrest) {
- args_setup_kw_rest_parameter(keyword_hash, locals + iseq->body->param.keyword->rest_start);
+ args_setup_kw_rest_parameter(keyword_hash, locals + iseq->body->param.keyword->rest_start, kw_flag);
}
else if (!NIL_P(keyword_hash) && RHASH_SIZE(keyword_hash) > 0 && arg_setup_type == arg_setup_method) {
argument_kw_error(ec, iseq, "unknown", rb_hash_keys(keyword_hash));
diff --git a/vm_callinfo.h b/vm_callinfo.h
index 30c4f703ba..f99f808a83 100644
--- a/vm_callinfo.h
+++ b/vm_callinfo.h
@@ -13,6 +13,7 @@ enum vm_call_flag_bits {
VM_CALL_SUPER_bit, /* super */
VM_CALL_ZSUPER_bit, /* zsuper */
VM_CALL_OPT_SEND_bit, /* internal flag */
+ VM_CALL_KW_SPLAT_MUT_bit, /* kw splat hash can be modified (to avoid allocating a new one) */
VM_CALL__END
};
@@ -28,6 +29,7 @@ enum vm_call_flag_bits {
#define VM_CALL_SUPER (0x01 << VM_CALL_SUPER_bit)
#define VM_CALL_ZSUPER (0x01 << VM_CALL_ZSUPER_bit)
#define VM_CALL_OPT_SEND (0x01 << VM_CALL_OPT_SEND_bit)
+#define VM_CALL_KW_SPLAT_MUT (0x01 << VM_CALL_KW_SPLAT_MUT_bit)
struct rb_callinfo_kwarg {
int keyword_len;
@@ -64,8 +66,8 @@ struct rb_callinfo {
#define CI_EMBED_ID_bits 32
#elif SIZEOF_VALUE == 4
#define CI_EMBED_TAG_bits 1
-#define CI_EMBED_ARGC_bits 4
-#define CI_EMBED_FLAG_bits 12
+#define CI_EMBED_ARGC_bits 3
+#define CI_EMBED_FLAG_bits 13
#define CI_EMBED_ID_bits 15
#endif
diff --git a/vm_insnhelper.c b/vm_insnhelper.c
index e0e1f9e971..626557cb45 100644
--- a/vm_insnhelper.c
+++ b/vm_insnhelper.c
@@ -1985,12 +1985,27 @@ CALLER_SETUP_ARG(struct rb_control_frame_struct *restrict cfp,
calling->kw_splat = 1;
}
}
- if (UNLIKELY(IS_ARGS_KEYWORD(ci))) {
- /* This converts VM_CALL_KWARG style to VM_CALL_KW_SPLAT style
- * by creating a keyword hash.
- * So, vm_ci_flag(ci) & VM_CALL_KWARG is now inconsistent.
- */
- vm_caller_setup_arg_kw(cfp, calling, ci);
+ if (UNLIKELY(IS_ARGS_KW_OR_KW_SPLAT(ci))) {
+ if (IS_ARGS_KEYWORD(ci)) {
+ /* This converts VM_CALL_KWARG style to VM_CALL_KW_SPLAT style
+ * by creating a keyword hash.
+ * So, vm_ci_flag(ci) & VM_CALL_KWARG is now inconsistent.
+ */
+ vm_caller_setup_arg_kw(cfp, calling, ci);
+ }
+ else {
+ VALUE keyword_hash = cfp->sp[-1];
+ if (!RB_TYPE_P(keyword_hash, T_HASH)) {
+ /* Convert a non-hash keyword splat to a new hash */
+ cfp->sp[-1] = 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.
+ */
+ cfp->sp[-1] = rb_hash_dup(keyword_hash);
+ }
+ }
}
}
diff --git a/vm_insnhelper.h b/vm_insnhelper.h
index 07b38ea9d9..e20a6effa4 100644
--- a/vm_insnhelper.h
+++ b/vm_insnhelper.h
@@ -247,6 +247,7 @@ THROW_DATA_CONSUMED_SET(struct vm_throw_data *obj)
#define IS_ARGS_KEYWORD(ci) (vm_ci_flag(ci) & VM_CALL_KWARG)
#define IS_ARGS_KW_SPLAT(ci) (vm_ci_flag(ci) & VM_CALL_KW_SPLAT)
#define IS_ARGS_KW_OR_KW_SPLAT(ci) (vm_ci_flag(ci) & (VM_CALL_KWARG | VM_CALL_KW_SPLAT))
+#define IS_ARGS_KW_SPLAT_MUT(ci) (vm_ci_flag(ci) & VM_CALL_KW_SPLAT_MUT)
/* If this returns true, an optimized function returned by `vm_call_iseq_setup_func`
can be used as a fastpath. */