diff --git a/doc/zjit.md b/doc/zjit.md index 57a95457d304e3..ba65739e3ff697 100644 --- a/doc/zjit.md +++ b/doc/zjit.md @@ -155,10 +155,10 @@ make -j ### Tracing side exits -Through [Stackprof](https://github.com/tmm1/stackprof), detailed information about the methods that the JIT side-exits from can be displayed after some execution of a program. Note that the use of `--zjit-trace-exits` must be used alongside `--zjit-stats`. +Through [Stackprof](https://github.com/tmm1/stackprof), detailed information about the methods that the JIT side-exits from can be displayed after some execution of a program. ```bash -./miniruby --zjit-stats --zjit-trace-exits script.rb +./miniruby --zjit-trace-exits script.rb ``` A file called `zjit_exit_locations.dump` will be created in the same directory as `script.rb`. Viewing the side exited methods can be done with Stackprof: diff --git a/prism/prism.c b/prism/prism.c index 6dd0b2ae738588..875968f06be820 100644 --- a/prism/prism.c +++ b/prism/prism.c @@ -2622,10 +2622,11 @@ pm_break_node_create(pm_parser_t *parser, const pm_token_t *keyword, pm_argument // There are certain flags that we want to use internally but don't want to // expose because they are not relevant beyond parsing. Therefore we'll define // them here and not define them in config.yml/a header file. -static const pm_node_flags_t PM_WRITE_NODE_FLAGS_IMPLICIT_ARRAY = 0x4; -static const pm_node_flags_t PM_CALL_NODE_FLAGS_IMPLICIT_ARRAY = 0x40; -static const pm_node_flags_t PM_CALL_NODE_FLAGS_COMPARISON = 0x80; -static const pm_node_flags_t PM_CALL_NODE_FLAGS_INDEX = 0x100; +static const pm_node_flags_t PM_WRITE_NODE_FLAGS_IMPLICIT_ARRAY = (1 << 2); + +static const pm_node_flags_t PM_CALL_NODE_FLAGS_IMPLICIT_ARRAY = ((PM_CALL_NODE_FLAGS_LAST - 1) << 1); +static const pm_node_flags_t PM_CALL_NODE_FLAGS_COMPARISON = ((PM_CALL_NODE_FLAGS_LAST - 1) << 2); +static const pm_node_flags_t PM_CALL_NODE_FLAGS_INDEX = ((PM_CALL_NODE_FLAGS_LAST - 1) << 3); /** * Allocate and initialize a new CallNode node. This sets everything to NULL or diff --git a/prism/templates/include/prism/ast.h.erb b/prism/templates/include/prism/ast.h.erb index 087eb81890eb45..e82cb78fe3106f 100644 --- a/prism/templates/include/prism/ast.h.erb +++ b/prism/templates/include/prism/ast.h.erb @@ -212,6 +212,8 @@ typedef enum pm_<%= flag.human %> { /** <%= value.comment %> */ PM_<%= flag.human.upcase %>_<%= value.name %> = <%= 1 << (index + Prism::Template::COMMON_FLAGS_COUNT) %>, <%- end -%> + + PM_<%= flag.human.upcase %>_LAST, } pm_<%= flag.human %>_t; <%- end -%> diff --git a/test/ruby/test_zjit.rb b/test/ruby/test_zjit.rb index 683d53f339f8c2..937cf44e190a56 100644 --- a/test/ruby/test_zjit.rb +++ b/test/ruby/test_zjit.rb @@ -1642,7 +1642,7 @@ def test(c) = c.foo }, call_threshold: 2, insns: [:opt_send_without_block] end - def test_attr_accessor + def test_attr_accessor_getivar assert_compiles '[4, 4]', %q{ class C attr_accessor :foo @@ -1658,6 +1658,47 @@ def test(c) = c.foo }, call_threshold: 2, insns: [:opt_send_without_block] end + def test_attr_accessor_setivar + assert_compiles '[5, 5]', %q{ + class C + attr_accessor :foo + + def initialize + @foo = 4 + end + end + + def test(c) + c.foo = 5 + c.foo + end + + c = C.new + [test(c), test(c)] + }, call_threshold: 2, insns: [:opt_send_without_block] + end + + def test_attr_writer + assert_compiles '[5, 5]', %q{ + class C + attr_writer :foo + + def initialize + @foo = 4 + end + + def get_foo = @foo + end + + def test(c) + c.foo = 5 + c.get_foo + end + 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 diff --git a/zjit.c b/zjit.c index 4bc27d9fe2dd15..d877c0bacbd507 100644 --- a/zjit.c +++ b/zjit.c @@ -59,6 +59,10 @@ rb_zjit_add_frame(VALUE hash, VALUE frame) rb_hash_aset(frame_info, ID2SYM(rb_intern("name")), name); rb_hash_aset(frame_info, ID2SYM(rb_intern("file")), file); + rb_hash_aset(frame_info, ID2SYM(rb_intern("samples")), INT2NUM(0)); + rb_hash_aset(frame_info, ID2SYM(rb_intern("total_samples")), INT2NUM(0)); + rb_hash_aset(frame_info, ID2SYM(rb_intern("edges")), rb_hash_new()); + rb_hash_aset(frame_info, ID2SYM(rb_intern("lines")), rb_hash_new()); if (line != INT2FIX(0)) { rb_hash_aset(frame_info, ID2SYM(rb_intern("line")), line); @@ -89,8 +93,9 @@ rb_zjit_exit_locations_dict(VALUE *zjit_raw_samples, int *zjit_line_samples, int int line_num = (int)zjit_line_samples[idx]; idx++; - rb_ary_push(raw_samples, SIZET2NUM(num)); - rb_ary_push(line_samples, INT2NUM(line_num)); + // + 1 as we append an additional sample for the insn + rb_ary_push(raw_samples, SIZET2NUM(num + 1)); + rb_ary_push(line_samples, INT2NUM(line_num + 1)); // Loop through the length of samples_len and add data to the // frames hash. Also push the current value onto the raw_samples diff --git a/zjit.rb b/zjit.rb index 83f8adb866c663..2ff4cf2a5b2a6f 100644 --- a/zjit.rb +++ b/zjit.rb @@ -9,10 +9,10 @@ module RubyVM::ZJIT # Avoid calling a Ruby method here to avoid interfering with compilation tests if Primitive.rb_zjit_print_stats_p - at_exit { - print_stats - dump_locations - } + at_exit { print_stats } + end + if Primitive.rb_zjit_trace_exit_locations_enabled_p + at_exit { dump_locations } end end @@ -40,16 +40,11 @@ def exit_locations frames = results[:frames].dup samples_count = 0 - frames.each do |frame_id, frame| - frame[:samples] = 0 - frame[:edges] = {} - end - # Loop through the instructions and set the frame hash with the data. # We use nonexistent.def for the file name, otherwise insns.def will be displayed # and that information isn't useful in this context. RubyVM::INSTRUCTION_NAMES.each_with_index do |name, frame_id| - frame_hash = { samples: 0, total_samples: 0, edges: {}, name: name, file: "nonexistent.def", line: nil } + frame_hash = { samples: 0, total_samples: 0, edges: {}, name: name, file: "nonexistent.def", line: nil, lines: {} } results[:frames][frame_id] = frame_hash frames[frame_id] = frame_hash end @@ -57,39 +52,51 @@ def exit_locations # Loop through the raw_samples and build the hashes for StackProf. # The loop is based off an example in the StackProf documentation and therefore # this functionality can only work with that library. - while raw_samples.length > 0 - stack_trace = raw_samples.shift(raw_samples.shift + 1) - lines = line_samples.shift(line_samples.shift + 1) + # + # Raw Samples: + # [ length, frame1, frame2, frameN, ..., instruction, count + # + # Line Samples + # [ length, line_1, line_2, line_n, ..., dummy value, count + i = 0 + while i < raw_samples.length + stack_length = raw_samples[i] + i += 1 # consume the stack length + + sample_count = raw_samples[i + stack_length] + prev_frame_id = nil + stack_length.times do |idx| + idx += i + frame_id = raw_samples[idx] - stack_trace.each_with_index do |frame_id, idx| if prev_frame_id prev_frame = frames[prev_frame_id] prev_frame[:edges][frame_id] ||= 0 - prev_frame[:edges][frame_id] += 1 + prev_frame[:edges][frame_id] += sample_count end frame_info = frames[frame_id] - frame_info[:total_samples] ||= 0 - frame_info[:total_samples] += 1 + frame_info[:total_samples] += sample_count - frame_info[:lines] ||= {} - frame_info[:lines][lines[idx]] ||= [0, 0] - frame_info[:lines][lines[idx]][0] += 1 + frame_info[:lines][line_samples[idx]] ||= [0, 0] + frame_info[:lines][line_samples[idx]][0] += sample_count prev_frame_id = frame_id end - top_frame_id = stack_trace.last + i += stack_length # consume the stack + + top_frame_id = prev_frame_id top_frame_line = 1 - frames[top_frame_id][:samples] += 1 + frames[top_frame_id][:samples] += sample_count frames[top_frame_id][:lines] ||= {} frames[top_frame_id][:lines][top_frame_line] ||= [0, 0] - frames[top_frame_id][:lines][top_frame_line][1] += 1 + frames[top_frame_id][:lines][top_frame_line][1] += sample_count - samples_count += raw_samples.shift - line_samples.shift + samples_count += sample_count + i += 1 end results[:samples] = samples_count @@ -119,7 +126,7 @@ def dump_exit_locations(filename) raise ArgumentError, "--zjit-trace-exits must be enabled to use dump_exit_locations." end - File.write(filename, Marshal.dump(RubyVM::ZJIT.exit_locations)) + File.binwrite(filename, Marshal.dump(RubyVM::ZJIT.exit_locations)) end # Check if `--zjit-stats` is used diff --git a/zjit/src/codegen.rs b/zjit/src/codegen.rs index c62fef73dea6be..b8d527fb8d6494 100644 --- a/zjit/src/codegen.rs +++ b/zjit/src/codegen.rs @@ -573,7 +573,7 @@ fn gen_setlocal(asm: &mut Assembler, val: Opnd, val_type: Type, local_ep_offset: } else { // We're potentially writing a reference to an IMEMO/env object, // so take care of the write barrier with a function. - let local_index = local_ep_offset * -1; + let local_index = -local_ep_offset; asm_ccall!(asm, rb_vm_env_write, ep, local_index.into(), val); } } diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index 8f6e92d6539dd0..5588544b4f5a66 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -2043,6 +2043,23 @@ impl Function { } let getivar = self.push_insn(block, Insn::GetIvar { self_val: recv, id, state }); self.make_equal_to(insn_id, getivar); + } else if let (VM_METHOD_TYPE_ATTRSET, &[val]) = (def_type, args.as_slice()) { + self.push_insn(block, Insn::PatchPoint { invariant: Invariant::MethodRedefined { klass, method: mid, cme }, state }); + if let Some(profiled_type) = profiled_type { + recv = self.push_insn(block, Insn::GuardType { val: recv, guard_type: Type::from_profiled_type(profiled_type), state }); + } + let id = unsafe { get_cme_def_body_attr_id(cme) }; + + // Check if we're accessing ivars of a Class or Module object as they require single-ractor mode. + // We omit gen_prepare_non_leaf_call on gen_setivar, so it's unsafe to raise for multi-ractor mode. + if unsafe { rb_zjit_singleton_class_p(klass) } { + let attached = unsafe { rb_class_attached_object(klass) }; + if unsafe { RB_TYPE_P(attached, RUBY_T_CLASS) || RB_TYPE_P(attached, RUBY_T_MODULE) } { + self.push_insn(block, Insn::PatchPoint { invariant: Invariant::SingleRactorMode, state }); + } + } + self.push_insn(block, Insn::SetIvar { self_val: recv, id, val, state }); + self.make_equal_to(insn_id, val); } else { self.set_dynamic_send_reason(insn_id, SendWithoutBlockNotOptimizedMethodType(MethodType::from(def_type))); self.push_insn_id(block, insn_id); continue; @@ -3263,7 +3280,7 @@ struct BytecodeInfo { has_blockiseq: bool, } -fn compute_bytecode_info(iseq: *const rb_iseq_t, opt_table: &Vec) -> BytecodeInfo { +fn compute_bytecode_info(iseq: *const rb_iseq_t, opt_table: &[u32]) -> BytecodeInfo { let iseq_size = unsafe { get_iseq_encoded_size(iseq) }; let mut insn_idx = 0; let mut jump_targets: HashSet = opt_table.iter().copied().collect(); @@ -4251,7 +4268,7 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result { } /// Compile an entry_block for the interpreter -fn compile_entry_block(fun: &mut Function, jit_entry_insns: &Vec) { +fn compile_entry_block(fun: &mut Function, jit_entry_insns: &[u32]) { let entry_block = fun.entry_block; fun.push_insn(entry_block, Insn::EntryPoint { jit_entry_idx: None }); @@ -12190,4 +12207,66 @@ mod opt_tests { Return v25 "); } + + #[test] + fn test_inline_attr_accessor_set() { + eval(" + class C + attr_accessor :foo + end + + def test(o) = o.foo = 5 + test C.new + test C.new + "); + assert_snapshot!(hir_string("test"), @r" + fn test@:6: + bb0(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:BasicObject = GetLocal l0, SP@4 + Jump bb2(v1, v2) + bb1(v5:BasicObject, v6:BasicObject): + EntryPoint JIT(0) + Jump bb2(v5, v6) + bb2(v8:BasicObject, v9:BasicObject): + v14:Fixnum[5] = Const Value(5) + PatchPoint MethodRedefined(C@0x1000, foo=@0x1008, cme:0x1010) + v23:HeapObject[class_exact:C] = GuardType v9, HeapObject[class_exact:C] + SetIvar v23, :@foo, v14 + CheckInterrupts + Return v14 + "); + } + + #[test] + fn test_inline_attr_writer_set() { + eval(" + class C + attr_writer :foo + end + + def test(o) = o.foo = 5 + test C.new + test C.new + "); + assert_snapshot!(hir_string("test"), @r" + fn test@:6: + bb0(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:BasicObject = GetLocal l0, SP@4 + Jump bb2(v1, v2) + bb1(v5:BasicObject, v6:BasicObject): + EntryPoint JIT(0) + Jump bb2(v5, v6) + bb2(v8:BasicObject, v9:BasicObject): + v14:Fixnum[5] = Const Value(5) + PatchPoint MethodRedefined(C@0x1000, foo=@0x1008, cme:0x1010) + v23:HeapObject[class_exact:C] = GuardType v9, HeapObject[class_exact:C] + SetIvar v23, :@foo, v14 + CheckInterrupts + Return v14 + "); + } } diff --git a/zjit/src/hir_type/mod.rs b/zjit/src/hir_type/mod.rs index 3e690c79d0c04f..ffde7e458d3b28 100644 --- a/zjit/src/hir_type/mod.rs +++ b/zjit/src/hir_type/mod.rs @@ -355,8 +355,7 @@ impl Type { fn is_builtin(class: VALUE) -> bool { types::ExactBitsAndClass .iter() - .find(|&(_, class_object)| unsafe { **class_object } == class) - .is_some() + .any(|&(_, class_object)| unsafe { *class_object } == class) } /// Union both types together, preserving specialization if possible. diff --git a/zjit/src/options.rs b/zjit/src/options.rs index ab9d1960ebaa20..4040c85907de4c 100644 --- a/zjit/src/options.rs +++ b/zjit/src/options.rs @@ -314,7 +314,7 @@ fn update_profile_threshold() { /// Update --zjit-call-threshold for testing #[cfg(test)] pub fn set_call_threshold(call_threshold: u64) { - unsafe { rb_zjit_call_threshold = call_threshold as u64; } + unsafe { rb_zjit_call_threshold = call_threshold; } rb_zjit_prepare_options(); update_profile_threshold(); } diff --git a/zjit/src/state.rs b/zjit/src/state.rs index 50c3f4b1c18fa2..206a7b3b618105 100644 --- a/zjit/src/state.rs +++ b/zjit/src/state.rs @@ -392,7 +392,7 @@ pub extern "C" fn rb_zjit_record_exit_stack(exit_pc: *const VALUE) { /// Mark `raw_samples` so they can be used by rb_zjit_add_frame. pub fn gc_mark_raw_samples() { // Return if ZJIT is not enabled - if !zjit_enabled_p() || !get_option!(stats) || !get_option!(trace_side_exits) { + if !zjit_enabled_p() || !get_option!(trace_side_exits) { return; } diff --git a/zjit/src/stats.rs b/zjit/src/stats.rs index 05ae231dad067a..a9cf1bde7cd131 100644 --- a/zjit/src/stats.rs +++ b/zjit/src/stats.rs @@ -2,6 +2,7 @@ use std::time::Instant; use std::sync::atomic::Ordering; +use crate::options::OPTIONS; #[cfg(feature = "stats_allocator")] #[path = "../../jit/src/lib.rs"] @@ -475,11 +476,11 @@ pub struct SideExitLocations { /// Primitive called in zjit.rb /// -/// Check if trace_exits generation is enabled. Requires the stats feature -/// to be enabled. +/// Check if trace_exits generation is enabled. #[unsafe(no_mangle)] pub extern "C" fn rb_zjit_trace_exit_locations_enabled_p(_ec: EcPtr, _ruby_self: VALUE) -> VALUE { - if get_option!(stats) && get_option!(trace_side_exits) { + // Builtin zjit.rb calls this even if ZJIT is disabled, so OPTIONS may not be set. + if unsafe { OPTIONS.as_ref() }.is_some_and(|opts| opts.trace_side_exits) { Qtrue } else { Qfalse @@ -490,7 +491,7 @@ pub extern "C" fn rb_zjit_trace_exit_locations_enabled_p(_ec: EcPtr, _ruby_self: /// into raw, lines, and frames hash for RubyVM::YJIT.exit_locations. #[unsafe(no_mangle)] pub extern "C" fn rb_zjit_get_exit_locations(_ec: EcPtr, _ruby_self: VALUE) -> VALUE { - if !zjit_enabled_p() || !get_option!(stats) || !get_option!(trace_side_exits) { + if !zjit_enabled_p() || !get_option!(trace_side_exits) { return Qnil; }