diff --git a/depend b/depend index ecaf33b1c7710c..ea2486e9e8da1d 100644 --- a/depend +++ b/depend @@ -12702,6 +12702,7 @@ ractor.$(OBJEXT): {$(VPATH)}vm_debug.h ractor.$(OBJEXT): {$(VPATH)}vm_opts.h ractor.$(OBJEXT): {$(VPATH)}vm_sync.h ractor.$(OBJEXT): {$(VPATH)}yjit.h +ractor.$(OBJEXT): {$(VPATH)}zjit.h random.$(OBJEXT): $(CCAN_DIR)/check_type/check_type.h random.$(OBJEXT): $(CCAN_DIR)/container_of/container_of.h random.$(OBJEXT): $(CCAN_DIR)/list/list.h diff --git a/ext/objspace/objspace_dump.c b/ext/objspace/objspace_dump.c index f90ad89b5a6ed7..94a9d43f98243b 100644 --- a/ext/objspace/objspace_dump.c +++ b/ext/objspace/objspace_dump.c @@ -451,13 +451,16 @@ dump_object(VALUE obj, struct dump_config *dc) break; case imemo_callcache: - mid = vm_cc_cme((const struct rb_callcache *)obj)->called_id; - if (mid != 0) { - dump_append(dc, ", \"called_id\":"); - dump_append_id(dc, mid); - + { VALUE klass = ((const struct rb_callcache *)obj)->klass; - if (klass != 0) { + if (klass != Qundef) { + mid = vm_cc_cme((const struct rb_callcache *)obj)->called_id; + if (mid != 0) { + dump_append(dc, ", \"called_id\":"); + dump_append_id(dc, mid); + + } + dump_append(dc, ", \"receiver_class\":"); dump_append_ref(dc, klass); } diff --git a/imemo.c b/imemo.c index 7298d78d65cdb7..2c721ca9112250 100644 --- a/imemo.c +++ b/imemo.c @@ -337,28 +337,37 @@ rb_imemo_mark_and_move(VALUE obj, bool reference_updating) * cc->klass (klass) should not be marked because if the klass is * free'ed, the cc->klass will be cleared by `vm_cc_invalidate()`. * - * cc->cme (cme) should not be marked because if cc is invalidated - * when cme is free'ed. + * For "normal" CCs cc->cme (cme) should not be marked because the cc is + * invalidated through the klass when the cme is free'd. * - klass marks cme if klass uses cme. - * - caller classe's ccs->cme marks cc->cme. - * - if cc is invalidated (klass doesn't refer the cc), - * cc is invalidated by `vm_cc_invalidate()` and cc->cme is - * not be accessed. - * - On the multi-Ractors, cme will be collected with global GC + * - caller class's ccs->cme marks cc->cme. + * - if cc is invalidated (klass doesn't refer the cc), cc is + * invalidated by `vm_cc_invalidate()` after which cc->cme must not + * be accessed. + * - With multi-Ractors, cme will be collected with global GC * so that it is safe if GC is not interleaving while accessing * cc and cme. - * - However, cc_type_super and cc_type_refinement are not chained - * from ccs so cc->cme should be marked; the cme might be - * reachable only through cc in these cases. + * + * However cc_type_super and cc_type_refinement are not chained + * from ccs so cc->cme should be marked as long as the cc is valid; + * the cme might be reachable only through cc in these cases. */ struct rb_callcache *cc = (struct rb_callcache *)obj; - if (reference_updating) { + if (UNDEF_P(cc->klass)) { + /* If it's invalidated, we must not mark anything. + * All fields should are considered invalid + */ + } + else if (reference_updating) { if (moved_or_living_object_strictly_p((VALUE)cc->cme_)) { *((VALUE *)&cc->klass) = rb_gc_location(cc->klass); *((struct rb_callable_method_entry_struct **)&cc->cme_) = (struct rb_callable_method_entry_struct *)rb_gc_location((VALUE)cc->cme_); + + RUBY_ASSERT(RB_TYPE_P(cc->klass, T_CLASS) || RB_TYPE_P(cc->klass, T_ICLASS)); + RUBY_ASSERT(IMEMO_TYPE_P((VALUE)cc->cme_, imemo_ment)); } - else if (vm_cc_valid(cc)) { + else { vm_cc_invalidate(cc); } } diff --git a/ractor.c b/ractor.c index dc83ccc1b408c8..096bda5df6dc1a 100644 --- a/ractor.c +++ b/ractor.c @@ -19,6 +19,7 @@ #include "internal/thread.h" #include "variable.h" #include "yjit.h" +#include "zjit.h" VALUE rb_cRactor; static VALUE rb_cRactorSelector; @@ -511,6 +512,7 @@ ractor_create(rb_execution_context_t *ec, VALUE self, VALUE loc, VALUE name, VAL r->debug = cr->debug; rb_yjit_before_ractor_spawn(); + rb_zjit_before_ractor_spawn(); rb_thread_create_ractor(r, args, block); RB_GC_GUARD(rv); diff --git a/test/.excludes-zjit/TestFixnum.rb b/test/.excludes-zjit/TestFixnum.rb deleted file mode 100644 index aaf8760f2f9ded..00000000000000 --- a/test/.excludes-zjit/TestFixnum.rb +++ /dev/null @@ -1,2 +0,0 @@ -# Issue: https://github.com/Shopify/ruby/issues/646 -exclude(/test_/, 'Tests make ZJIT panic on Ubuntu') diff --git a/test/ruby/test_zjit.rb b/test/ruby/test_zjit.rb index e3ef6f65698ad6..8115a601668422 100644 --- a/test/ruby/test_zjit.rb +++ b/test/ruby/test_zjit.rb @@ -887,6 +887,38 @@ def test() = @foo = 1 } end + def test_attr_reader + assert_compiles '[4, 4]', %q{ + class C + attr_reader :foo + + def initialize + @foo = 4 + end + end + + def test(c) = c.foo + c = C.new + [test(c), test(c)] + }, call_threshold: 2, insns: [:opt_send_without_block] + end + + def test_attr_accessor + assert_compiles '[4, 4]', %q{ + class C + attr_accessor :foo + + def initialize + @foo = 4 + end + end + + def test(c) = c.foo + c = C.new + [test(c), test(c)] + }, call_threshold: 2, insns: [:opt_send_without_block] + end + def test_uncached_getconstant_path assert_compiles RUBY_COPYRIGHT.dump, %q{ def test = RUBY_COPYRIGHT @@ -950,6 +982,26 @@ def test = A::B::C RUBY end + def test_single_ractor_mode_invalidation + # Without invalidating the single-ractor mode, the test would crash + assert_compiles '"errored but not crashed"', <<~RUBY, call_threshold: 2, insns: [:opt_getconstant_path] + C = Object.new + + def test + C + rescue Ractor::IsolationError + "errored but not crashed" + end + + test + test + + Ractor.new { + test + }.value + RUBY + end + def test_dupn assert_compiles '[[1], [1, 1], :rhs, [nil, :rhs]]', <<~RUBY, insns: [:dupn] def test(array) = (array[1, 2] ||= :rhs) diff --git a/vm_callinfo.h b/vm_callinfo.h index 3b6880e3205920..79ccbfa7abb7c3 100644 --- a/vm_callinfo.h +++ b/vm_callinfo.h @@ -418,6 +418,8 @@ static inline const struct rb_callable_method_entry_struct * vm_cc_cme(const struct rb_callcache *cc) { VM_ASSERT(IMEMO_TYPE_P(cc, imemo_callcache)); + VM_ASSERT(cc->klass != Qundef || !vm_cc_markable(cc)); + VM_ASSERT(cc_check_class(cc->klass)); VM_ASSERT(cc->call_ == NULL || // not initialized yet !vm_cc_markable(cc) || cc->cme_ != NULL); @@ -430,6 +432,8 @@ vm_cc_call(const struct rb_callcache *cc) { VM_ASSERT(IMEMO_TYPE_P(cc, imemo_callcache)); VM_ASSERT(cc->call_ != NULL); + VM_ASSERT(cc->klass != Qundef || !vm_cc_markable(cc)); + VM_ASSERT(cc_check_class(cc->klass)); return cc->call_; } diff --git a/yjit/src/invariants.rs b/yjit/src/invariants.rs index a1a7d300aa13b3..6ae1342ce38977 100644 --- a/yjit/src/invariants.rs +++ b/yjit/src/invariants.rs @@ -303,7 +303,7 @@ pub extern "C" fn rb_yjit_cme_invalidate(callee_cme: *const rb_callable_method_e }); } -/// Callback for then Ruby is about to spawn a ractor. In that case we need to +/// Callback for when Ruby is about to spawn a ractor. In that case we need to /// invalidate every block that is assuming single ractor mode. #[no_mangle] pub extern "C" fn rb_yjit_before_ractor_spawn() { diff --git a/zjit.h b/zjit.h index 5ce2826d067a2b..adf47046f83b37 100644 --- a/zjit.h +++ b/zjit.h @@ -14,7 +14,7 @@ extern bool rb_zjit_enabled_p; extern uint64_t rb_zjit_call_threshold; extern uint64_t rb_zjit_profile_threshold; void rb_zjit_compile_iseq(const rb_iseq_t *iseq, rb_execution_context_t *ec, bool jit_exception); -void rb_zjit_profile_insn(enum ruby_vminsn_type insn, rb_execution_context_t *ec); +void rb_zjit_profile_insn(uint32_t insn, rb_execution_context_t *ec); void rb_zjit_profile_enable(const rb_iseq_t *iseq); void rb_zjit_bop_redefined(int redefined_flag, enum ruby_basic_operators bop); void rb_zjit_cme_invalidate(const rb_callable_method_entry_t *cme); @@ -22,15 +22,17 @@ void rb_zjit_invalidate_ep_is_bp(const rb_iseq_t *iseq); void rb_zjit_constant_state_changed(ID id); void rb_zjit_iseq_mark(void *payload); void rb_zjit_iseq_update_references(void *payload); +void rb_zjit_before_ractor_spawn(void); #else #define rb_zjit_enabled_p false static inline void rb_zjit_compile_iseq(const rb_iseq_t *iseq, rb_execution_context_t *ec, bool jit_exception) {} -static inline void rb_zjit_profile_insn(enum ruby_vminsn_type insn, rb_execution_context_t *ec) {} +static inline void rb_zjit_profile_insn(uint32_t insn, rb_execution_context_t *ec) {} static inline void rb_zjit_profile_enable(const rb_iseq_t *iseq) {} static inline void rb_zjit_bop_redefined(int redefined_flag, enum ruby_basic_operators bop) {} static inline void rb_zjit_cme_invalidate(const rb_callable_method_entry_t *cme) {} static inline void rb_zjit_invalidate_ep_is_bp(const rb_iseq_t *iseq) {} static inline void rb_zjit_constant_state_changed(ID id) {} -#endif // #if USE_YJIT +static inline void rb_zjit_before_ractor_spawn(void) {} +#endif // #if USE_ZJIT #endif // #ifndef ZJIT_H diff --git a/zjit/src/backend/x86_64/mod.rs b/zjit/src/backend/x86_64/mod.rs index f7aa1acc4dae63..8027c74b182e66 100644 --- a/zjit/src/backend/x86_64/mod.rs +++ b/zjit/src/backend/x86_64/mod.rs @@ -148,6 +148,15 @@ impl Assembler }; } + // When we split an operand, we can create a new VReg not in `live_ranges`. + // So when we see a VReg with out-of-range index, it's created from splitting + // from the loop above and we know it doesn't outlive the current instruction. + let vreg_outlives_insn = |vreg_idx| { + live_ranges + .get(vreg_idx) + .map_or(false, |live_range: &LiveRange| live_range.end() > index) + }; + // We are replacing instructions here so we know they are already // being used. It is okay not to use their output here. #[allow(unused_must_use)] @@ -183,7 +192,7 @@ impl Assembler }, // Instruction output whose live range spans beyond this instruction (Opnd::VReg { idx, .. }, _) => { - if live_ranges[idx].end() > index { + if vreg_outlives_insn(idx) { *left = asm.load(*left); } }, @@ -248,7 +257,7 @@ impl Assembler match opnd { // Instruction output whose live range spans beyond this instruction Opnd::VReg { idx, .. } => { - if live_ranges[*idx].end() > index { + if vreg_outlives_insn(*idx) { *opnd = asm.load(*opnd); } }, @@ -272,7 +281,7 @@ impl Assembler // If we have an instruction output whose live range // spans beyond this instruction, we have to load it. Opnd::VReg { idx, .. } => { - if live_ranges[idx].end() > index { + if vreg_outlives_insn(idx) { *truthy = asm.load(*truthy); } }, @@ -307,7 +316,7 @@ impl Assembler // If we have an instruction output whose live range // spans beyond this instruction, we have to load it. Opnd::VReg { idx, .. } => { - if live_ranges[idx].end() > index { + if vreg_outlives_insn(idx) { *opnd = asm.load(*opnd); } }, diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index 0db4d6b781686a..b05aaca68222ae 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -4,7 +4,7 @@ use std::ffi::{c_int, c_void}; use crate::asm::Label; use crate::backend::current::{Reg, ALLOC_REGS}; -use crate::invariants::{track_bop_assumption, track_cme_assumption, track_stable_constant_names_assumption}; +use crate::invariants::{track_bop_assumption, track_cme_assumption, track_single_ractor_assumption, track_stable_constant_names_assumption}; use crate::gc::{get_or_create_iseq_payload, append_gc_offsets}; use crate::state::ZJITState; use crate::stats::{counter_ptr, Counter}; @@ -542,9 +542,9 @@ fn gen_patch_point(jit: &mut JITState, asm: &mut Assembler, invariant: &Invarian let side_exit_ptr = cb.resolve_label(label); track_stable_constant_names_assumption(idlist, code_ptr, side_exit_ptr); } - _ => { - debug!("ZJIT: gen_patch_point: unimplemented invariant {invariant:?}"); - return; + Invariant::SingleRactorMode => { + let side_exit_ptr = cb.resolve_label(label); + track_single_ractor_assumption(code_ptr, side_exit_ptr); } } }); diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 1a67037ed35733..7e92f5932922a8 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -1537,22 +1537,31 @@ impl Function { // It allows you to use a faster ISEQ if possible. cme = unsafe { rb_check_overloaded_cme(cme, ci) }; let def_type = unsafe { get_cme_def_type(cme) }; - if def_type != VM_METHOD_TYPE_ISEQ { + if def_type == VM_METHOD_TYPE_ISEQ { // TODO(max): Allow non-iseq; cache cme + // Only specialize positional-positional calls + // TODO(max): Handle other kinds of parameter passing + let iseq = unsafe { get_def_iseq_ptr((*cme).def) }; + if !can_direct_send(iseq) { + self.push_insn_id(block, insn_id); continue; + } + self.push_insn(block, Insn::PatchPoint { invariant: Invariant::MethodRedefined { klass, method: mid, cme }, state }); + if let Some(profiled_type) = profiled_type { + self_val = self.push_insn(block, Insn::GuardType { val: self_val, guard_type: Type::from_profiled_type(profiled_type), state }); + } + let send_direct = self.push_insn(block, Insn::SendWithoutBlockDirect { self_val, cd, cme, iseq, args, state }); + self.make_equal_to(insn_id, send_direct); + } else if def_type == VM_METHOD_TYPE_IVAR && args.is_empty() { + self.push_insn(block, Insn::PatchPoint { invariant: Invariant::MethodRedefined { klass, method: mid, cme }, state }); + if let Some(profiled_type) = profiled_type { + self_val = self.push_insn(block, Insn::GuardType { val: self_val, guard_type: Type::from_profiled_type(profiled_type), state }); + } + let id = unsafe { get_cme_def_body_attr_id(cme) }; + let getivar = self.push_insn(block, Insn::GetIvar { self_val, id, state }); + self.make_equal_to(insn_id, getivar); + } else { self.push_insn_id(block, insn_id); continue; } - // Only specialize positional-positional calls - // TODO(max): Handle other kinds of parameter passing - let iseq = unsafe { get_def_iseq_ptr((*cme).def) }; - if !can_direct_send(iseq) { - self.push_insn_id(block, insn_id); continue; - } - self.push_insn(block, Insn::PatchPoint { invariant: Invariant::MethodRedefined { klass, method: mid, cme }, state }); - if let Some(profiled_type) = profiled_type { - self_val = self.push_insn(block, Insn::GuardType { val: self_val, guard_type: Type::from_profiled_type(profiled_type), state }); - } - let send_direct = self.push_insn(block, Insn::SendWithoutBlockDirect { self_val, cd, cme, iseq, args, state }); - self.make_equal_to(insn_id, send_direct); } Insn::GetConstantPath { ic, state, .. } => { let idlist: *const ID = unsafe { (*ic).segments }; @@ -7422,4 +7431,94 @@ mod opt_tests { Return v7 "#]]); } + + #[test] + fn test_inline_attr_reader_constant() { + eval(" + class C + attr_reader :foo + end + + O = C.new + def test = O.foo + test + test + "); + assert_optimized_method_hir("test", expect![[r#" + fn test@:7: + bb0(v0:BasicObject): + PatchPoint SingleRactorMode + PatchPoint StableConstantNames(0x1000, O) + v9:BasicObject[VALUE(0x1008)] = Const Value(VALUE(0x1008)) + PatchPoint MethodRedefined(C@0x1010, foo@0x1018, cme:0x1020) + v11:BasicObject = GetIvar v9, :@foo + Return v11 + "#]]); + } + + #[test] + fn test_inline_attr_accessor_constant() { + eval(" + class C + attr_accessor :foo + end + + O = C.new + def test = O.foo + test + test + "); + assert_optimized_method_hir("test", expect![[r#" + fn test@:7: + bb0(v0:BasicObject): + PatchPoint SingleRactorMode + PatchPoint StableConstantNames(0x1000, O) + v9:BasicObject[VALUE(0x1008)] = Const Value(VALUE(0x1008)) + PatchPoint MethodRedefined(C@0x1010, foo@0x1018, cme:0x1020) + v11:BasicObject = GetIvar v9, :@foo + Return v11 + "#]]); + } + + #[test] + fn test_inline_attr_reader() { + eval(" + class C + attr_reader :foo + end + + def test(o) = o.foo + test C.new + test C.new + "); + assert_optimized_method_hir("test", expect![[r#" + fn test@:6: + bb0(v0:BasicObject, v1:BasicObject): + PatchPoint MethodRedefined(C@0x1000, foo@0x1008, cme:0x1010) + v7:BasicObject[class_exact:C] = GuardType v1, BasicObject[class_exact:C] + v8:BasicObject = GetIvar v7, :@foo + Return v8 + "#]]); + } + + #[test] + fn test_inline_attr_accessor() { + eval(" + class C + attr_accessor :foo + end + + def test(o) = o.foo + test C.new + test C.new + "); + assert_optimized_method_hir("test", expect![[r#" + fn test@:6: + bb0(v0:BasicObject, v1:BasicObject): + PatchPoint MethodRedefined(C@0x1000, foo@0x1008, cme:0x1010) + v7:BasicObject[class_exact:C] = GuardType v1, BasicObject[class_exact:C] + v8:BasicObject = GetIvar v7, :@foo + Return v8 + "#]]); + } } diff --git a/zjit/src/invariants.rs b/zjit/src/invariants.rs index 25cffb970e62dc..c8c91dc45b97a1 100644 --- a/zjit/src/invariants.rs +++ b/zjit/src/invariants.rs @@ -1,7 +1,20 @@ -use std::{collections::{HashMap, HashSet}}; +use std::{collections::{HashMap, HashSet}, mem}; use crate::{backend::lir::{asm_comment, Assembler}, cruby::{rb_callable_method_entry_t, ruby_basic_operators, src_loc, with_vm_lock, IseqPtr, RedefinitionFlag, ID}, hir::Invariant, options::debug, state::{zjit_enabled_p, ZJITState}, virtualmem::CodePtr}; +macro_rules! compile_jumps { + ($cb:expr, $jumps:expr, $($comment_args:tt)*) => { + for jump in $jumps { + $cb.with_write_ptr(jump.from, |cb| { + let mut asm = Assembler::new(); + asm_comment!(asm, $($comment_args)*); + asm.jmp(jump.to.into()); + asm.compile(cb).expect("can write existing code"); + }); + } + }; +} + #[derive(Debug, Eq, Hash, PartialEq)] struct Jump { from: CodePtr, @@ -26,6 +39,9 @@ pub struct Invariants { /// Map from constant ID to patch points that assume the constant hasn't been redefined constant_state_patch_points: HashMap>, + + /// Set of patch points that assume that the interpreter is running with only one ractor + single_ractor_patch_points: HashSet, } /// Called when a basic operator is redefined. Note that all the blocks assuming @@ -46,14 +62,7 @@ pub extern "C" fn rb_zjit_bop_redefined(klass: RedefinitionFlag, bop: ruby_basic debug!("BOP is redefined: {}", bop); // Invalidate all patch points for this BOP - for jump in jumps { - cb.with_write_ptr(jump.from, |cb| { - let mut asm = Assembler::new(); - asm_comment!(asm, "BOP is redefined: {}", bop); - asm.jmp(jump.to.into()); - asm.compile(cb).expect("can write existing code"); - }); - } + compile_jumps!(cb, jumps, "BOP is redefined: {}", bop); cb.mark_all_executable(); } @@ -159,14 +168,8 @@ pub extern "C" fn rb_zjit_cme_invalidate(cme: *const rb_callable_method_entry_t) debug!("CME is invalidated: {:?}", cme); // Invalidate all patch points for this CME - for jump in jumps { - cb.with_write_ptr(jump.from, |cb| { - let mut asm = Assembler::new(); - asm_comment!(asm, "CME is invalidated: {:?}", cme); - asm.jmp(jump.to.into()); - asm.compile(cb).expect("can write existing code"); - }); - } + compile_jumps!(cb, jumps, "CME is invalidated: {:?}", cme); + cb.mark_all_executable(); } }); @@ -187,16 +190,38 @@ pub extern "C" fn rb_zjit_constant_state_changed(id: ID) { debug!("Constant state changed: {:?}", id); // Invalidate all patch points for this constant ID - for jump in jumps { - cb.with_write_ptr(jump.from, |cb| { - let mut asm = Assembler::new(); - asm_comment!(asm, "Constant state changed: {:?}", id); - asm.jmp(jump.to.into()); - asm.compile(cb).expect("can write existing code"); - }); - } + compile_jumps!(cb, jumps, "Constant state changed: {:?}", id); cb.mark_all_executable(); } }); } + +/// Track the JIT code that assumes that the interpreter is running with only one ractor +pub fn track_single_ractor_assumption(patch_point_ptr: CodePtr, side_exit_ptr: CodePtr) { + let invariants = ZJITState::get_invariants(); + invariants.single_ractor_patch_points.insert(Jump { + from: patch_point_ptr, + to: side_exit_ptr, + }); +} + +/// Callback for when Ruby is about to spawn a ractor. In that case we need to +/// invalidate every block that is assuming single ractor mode. +#[unsafe(no_mangle)] +pub extern "C" fn rb_zjit_before_ractor_spawn() { + // If ZJIT isn't enabled, do nothing + if !zjit_enabled_p() { + return; + } + + with_vm_lock(src_loc!(), || { + let cb = ZJITState::get_code_block(); + let jumps = mem::take(&mut ZJITState::get_invariants().single_ractor_patch_points); + + // Invalidate all patch points for single ractor mode + compile_jumps!(cb, jumps, "Another ractor spawned, invalidating single ractor mode assumption"); + + cb.mark_all_executable(); + }); +} diff --git a/zjit/src/profile.rs b/zjit/src/profile.rs index a99229604b0ce2..12b10b98ee57bc 100644 --- a/zjit/src/profile.rs +++ b/zjit/src/profile.rs @@ -39,10 +39,10 @@ impl Profiler { /// API called from zjit_* instruction. opcode is the bare (non-zjit_*) instruction. #[unsafe(no_mangle)] -pub extern "C" fn rb_zjit_profile_insn(bare_opcode: ruby_vminsn_type, ec: EcPtr) { +pub extern "C" fn rb_zjit_profile_insn(bare_opcode: u32, ec: EcPtr) { with_vm_lock(src_loc!(), || { let mut profiler = Profiler::new(ec); - profile_insn(&mut profiler, bare_opcode); + profile_insn(&mut profiler, bare_opcode as ruby_vminsn_type); }); }