ruby-changes:73953
From: Jemma <ko1@a...>
Date: Wed, 12 Oct 2022 00:42:07 +0900 (JST)
Subject: [ruby-changes:73953] 913979bede (master): Make inline cache reads / writes atomic with object shapes
https://git.ruby-lang.org/ruby.git/commit/?id=913979bede From 913979bede2a1b79109fa2072352882560d55fe0 Mon Sep 17 00:00:00 2001 From: Jemma Issroff <jemmaissroff@g...> Date: Mon, 3 Oct 2022 13:52:40 -0400 Subject: Make inline cache reads / writes atomic with object shapes Prior to this commit, we were reading and writing ivar index and shape ID in inline caches in two separate instructions when getting and setting ivars. This meant there was a race condition with ractors and these caches where one ractor could change a value in the cache while another was still reading from it. This commit instead reads and writes shape ID and ivar index to inline caches atomically so there is no longer a race condition. Co-Authored-By: Aaron Patterson <tenderlove@r...> Co-Authored-By: John Hawthorn <john@h...> --- lib/mjit/compiler.rb | 30 ++++++--- mjit_c.rb | 11 +-- object.c | 2 +- shape.c | 82 ++++++++++++----------- shape.h | 18 +---- test/ruby/test_shapes.rb | 57 +++++++++------- variable.c | 6 +- vm.c | 4 +- vm_callinfo.h | 81 ++++++---------------- vm_core.h | 4 +- vm_insnhelper.c | 148 +++++++++++++++++++++-------------------- yjit/src/codegen.rs | 2 +- yjit/src/cruby_bindings.inc.rs | 7 +- 13 files changed, 215 insertions(+), 237 deletions(-) diff --git a/lib/mjit/compiler.rb b/lib/mjit/compiler.rb index 06f018c934..0d2910bf4e 100644 --- a/lib/mjit/compiler.rb +++ b/lib/mjit/compiler.rb @@ -351,20 +351,27 @@ module RubyVM::MJIT https://github.com/ruby/ruby/blob/trunk/lib/mjit/compiler.rb#L351 # _mjit_compile_ivar.erb def compile_ivar(insn_name, stack_size, pos, status, operands, body) ic_copy = (status.is_entries + (C.iseq_inline_storage_entry.new(operands[1]) - body.is_entries)).iv_cache + dest_shape_id = ic_copy.value >> C.SHAPE_FLAG_SHIFT + attr_index = ic_copy.value & ((1 << C.SHAPE_FLAG_SHIFT) - 1) + source_shape_id = if dest_shape_id == C.INVALID_SHAPE_ID + dest_shape_id + else + RubyVM::Shape.find_by_id(dest_shape_id).parent_id + end src = +'' - if !status.compile_info.disable_ivar_cache && ic_copy.source_shape_id != C.INVALID_SHAPE_ID + if !status.compile_info.disable_ivar_cache && source_shape_id != C.INVALID_SHAPE_ID # JIT: optimize away motion of sp and pc. This path does not call rb_warning() and so it's always leaf and not `handles_sp`. # compile_pc_and_sp(src, insn, stack_size, sp_inc, local_stack_p, next_pos) # JIT: prepare vm_getivar/vm_setivar arguments and variables src << "{\n" src << " VALUE obj = GET_SELF();\n" - src << " const shape_id_t source_shape_id = (rb_serial_t)#{ic_copy.source_shape_id};\n" # JIT: cache hit path of vm_getivar/vm_setivar, or cancel JIT (recompile it with exivar) if insn_name == :setinstancevariable - src << " const uint32_t index = #{ic_copy.attr_index - 1};\n" - src << " const shape_id_t dest_shape_id = (rb_serial_t)#{ic_copy.dest_shape_id};\n" + src << " const shape_id_t source_shape_id = (shape_id_t)#{source_shape_id};\n" + src << " const uint32_t index = #{attr_index - 1};\n" + src << " const shape_id_t dest_shape_id = (shape_id_t)#{dest_shape_id};\n" src << " if (source_shape_id == ROBJECT_SHAPE_ID(obj) && \n" src << " dest_shape_id != ROBJECT_SHAPE_ID(obj)) {\n" src << " if (UNLIKELY(index >= ROBJECT_NUMIV(obj))) {\n" @@ -374,14 +381,19 @@ module RubyVM::MJIT https://github.com/ruby/ruby/blob/trunk/lib/mjit/compiler.rb#L381 src << " VALUE *ptr = ROBJECT_IVPTR(obj);\n" src << " RB_OBJ_WRITE(obj, &ptr[index], stack[#{stack_size - 1}]);\n" src << " }\n" + src << " else if (dest_shape_id == ROBJECT_SHAPE_ID(obj)) {\n" + src << " VALUE *ptr = ROBJECT_IVPTR(obj);\n" + src << " RB_OBJ_WRITE(obj, &ptr[index], stack[#{stack_size - 1}]);\n" + src << " }\n" else - if ic_copy.attr_index == 0 # cache hit, but uninitialized iv + src << " const shape_id_t source_shape_id = (shape_id_t)#{dest_shape_id};\n" + if attr_index == 0 # cache hit, but uninitialized iv src << " /* Uninitialized instance variable */\n" src << " if (source_shape_id == ROBJECT_SHAPE_ID(obj)) {\n" src << " stack[#{stack_size}] = Qnil;\n" src << " }\n" else - src << " const uint32_t index = #{ic_copy.attr_index - 1};\n" + src << " const uint32_t index = #{attr_index - 1};\n" src << " if (source_shape_id == ROBJECT_SHAPE_ID(obj)) {\n" src << " stack[#{stack_size}] = ROBJECT_IVPTR(obj)[index];\n" src << " }\n" @@ -394,15 +406,15 @@ module RubyVM::MJIT https://github.com/ruby/ruby/blob/trunk/lib/mjit/compiler.rb#L406 src << " }\n" src << "}\n" return src - elsif insn_name == :getinstancevariable && !status.compile_info.disable_exivar_cache && ic_copy.source_shape_id != C.INVALID_SHAPE_ID + elsif insn_name == :getinstancevariable && !status.compile_info.disable_exivar_cache && source_shape_id != C.INVALID_SHAPE_ID # JIT: optimize away motion of sp and pc. This path does not call rb_warning() and so it's always leaf and not `handles_sp`. # compile_pc_and_sp(src, insn, stack_size, sp_inc, local_stack_p, next_pos) # JIT: prepare vm_getivar's arguments and variables src << "{\n" src << " VALUE obj = GET_SELF();\n" - src << " const shape_id_t source_shape_id = (rb_serial_t)#{ic_copy.source_shape_id};\n" - src << " const uint32_t index = #{ic_copy.attr_index - 1};\n" + src << " const shape_id_t source_shape_id = (shape_id_t)#{dest_shape_id};\n" + src << " const uint32_t index = #{attr_index - 1};\n" # JIT: cache hit path of vm_getivar, or cancel JIT (recompile it without any ivar optimization) src << " struct gen_ivtbl *ivtbl;\n" src << " if (LIKELY(FL_TEST_RAW(obj, FL_EXIVAR) && source_shape_id == rb_shape_get_shape_id(obj) && rb_ivar_generic_ivtbl_lookup(obj, &ivtbl))) {\n" diff --git a/mjit_c.rb b/mjit_c.rb index 0f8e11cbe5..1858f86e4d 100644 --- a/mjit_c.rb +++ b/mjit_c.rb @@ -9,6 +9,10 @@ module RubyVM::MJIT https://github.com/ruby/ruby/blob/trunk/mjit_c.rb#L9 RubyVM::Shape::SHAPE_BITS end + def SHAPE_FLAG_SHIFT + RubyVM::Shape::SHAPE_FLAG_SHIFT + end + def ROBJECT_EMBED_LEN_MAX Primitive.cexpr! 'INT2NUM(RBIMPL_EMBED_LEN_MAX_OF(VALUE))' end @@ -255,9 +259,7 @@ module RubyVM::MJIT https://github.com/ruby/ruby/blob/trunk/mjit_c.rb#L259 def C.iseq_inline_iv_cache_entry @iseq_inline_iv_cache_entry ||= CType::Struct.new( "iseq_inline_iv_cache_entry", Primitive.cexpr!("SIZEOF(struct iseq_inline_iv_cache_entry)"), - source_shape_id: [self.shape_id_t, Primitive.cexpr!("OFFSETOF((*((struct iseq_inline_iv_cache_entry *)NULL)), source_shape_id)")], - dest_shape_id: [self.shape_id_t, Primitive.cexpr!("OFFSETOF((*((struct iseq_inline_iv_cache_entry *)NULL)), dest_shape_id)")], - attr_index: [self.attr_index_t, Primitive.cexpr!("OFFSETOF((*((struct iseq_inline_iv_cache_entry *)NULL)), attr_index)")], + value: [CType::Immediate.parse("uintptr_t"), Primitive.cexpr!("OFFSETOF((*((struct iseq_inline_iv_cache_entry *)NULL)), value)")], ) end @@ -332,8 +334,7 @@ module RubyVM::MJIT https://github.com/ruby/ruby/blob/trunk/mjit_c.rb#L334 "", Primitive.cexpr!("SIZEOF(((struct rb_callcache *)NULL)->aux_)"), attr: CType::Struct.new( "", Primitive.cexpr!("SIZEOF(((struct rb_callcache *)NULL)->aux_.attr)"), - index: [self.attr_index_t, Primitive.cexpr!("OFFSETOF(((struct rb_callcache *)NULL)->aux_.attr, index)")], - dest_shape_id: [self.shape_id_t, Primitive.cexpr!("OFFSETOF(((struct rb_callcache *)NULL)->aux_.attr, dest_shape_id)")], + value: [CType::Immediate.parse("uintptr_t"), Primitive.cexpr!("OFFSETOF(((struct rb_callcache *)NULL)->aux_.attr, value)")], ), method_missing_reason: self.method_missing_reason, v: self.VALUE, diff --git a/object.c b/object.c index 648946beb3..929968c4cb 100644 --- a/object.c +++ b/object.c @@ -319,7 +319,7 @@ init_copy(VALUE dest, VALUE obj) https://github.com/ruby/ruby/blob/trunk/object.c#L319 // If the object is frozen, the "dup"'d object will *not* be frozen, // so we need to copy the frozen shape's parent to the new object. if (rb_shape_frozen_shape_p(shape_to_set)) { - shape_to_set = shape_to_set->parent; + shape_to_set = rb_shape_get_shape_by_id(shape_to_set->parent_id); } // shape ids are different diff --git a/shape.c b/shape.c index a6a5adf854..0f48fa1a86 100644 --- a/shape.c +++ b/shape.c @@ -91,7 +91,7 @@ rb_shape_get_shape(VALUE obj) https://github.com/ruby/ruby/blob/trunk/shape.c#L91 static rb_shape_t * rb_shape_lookup_id(rb_shape_t* shape, ID id, enum shape_type shape_type) { - while (shape->parent) { + while (shape->parent_id != INVALID_SHAPE_ID) { if (shape->edge_name == id) { // If the shape type is different, we don't // want this to count as a "found" ID @@ -102,7 +102,7 @@ rb_shape_lookup_id(rb_shape_t* shape, ID id, enum shape_type shape_type) { https://github.com/ruby/ruby/blob/trunk/shape.c#L102 return NULL; } } - shape = shape->parent; + shape = rb_shape_get_shape_by_id(shape->pare (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/