ruby-changes:67337
From: Jeremy <ko1@a...>
Date: Sun, 29 Aug 2021 23:24:03 +0900 (JST)
Subject: [ruby-changes:67337] 2d98593bf5 (master): Support tracing of attr_reader and attr_writer
https://git.ruby-lang.org/ruby.git/commit/?id=2d98593bf5 From 2d98593bf54a37397c6e4886ccc7e3654c2eaf85 Mon Sep 17 00:00:00 2001 From: Jeremy Evans <code@j...> Date: Mon, 23 Aug 2021 15:22:14 -0700 Subject: Support tracing of attr_reader and attr_writer In vm_call_method_each_type, check for c_call and c_return events before dispatching to vm_call_ivar and vm_call_attrset. With this approach, the call cache will still dispatch directly to those functions, so this change will only decrease performance for the first (uncached) call, and even then, the performance decrease is very minimal. This approach requires that we clear the call caches when tracing is enabled or disabled. The approach currently switches all vm_call_ivar and vm_call_attrset call caches to vm_call_general any time tracing is enabled or disabled. So it could theoretically result in a slowdown for code that constantly enables or disables tracing. This approach does not handle targeted tracepoints, but from my testing, c_call and c_return events are not supported for targeted tracepoints, so that shouldn't matter. This includes a benchmark showing the performance decrease is minimal if detectable at all. Fixes [Bug #16383] Fixes [Bug #10470] Co-authored-by: Takashi Kokubun <takashikkbn@g...> --- benchmark/attr_accessor.yml | 29 ++++++++++++++++++++++++ iseq.c | 29 ++++++++++++++++++++++++ test/ruby/test_settracefunc.rb | 50 ++++++++++++++++++++++++++++++++++++++++++ vm_eval.c | 22 +++++++++++++++++-- vm_insnhelper.c | 47 +++++++++++++++++++++++++++++++++++---- vm_trace.c | 5 +++++ 6 files changed, 176 insertions(+), 6 deletions(-) create mode 100644 benchmark/attr_accessor.yml diff --git a/benchmark/attr_accessor.yml b/benchmark/attr_accessor.yml new file mode 100644 index 0000000..82134cd --- /dev/null +++ b/benchmark/attr_accessor.yml @@ -0,0 +1,29 @@ https://github.com/ruby/ruby/blob/trunk/benchmark/attr_accessor.yml#L1 +prelude: | + class C + attr_accessor :x + def initialize + @x = nil + end + class_eval <<-END + def ar + #{'x;'*256} + end + def aw + #{'self.x = nil;'*256} + end + def arm + m = method(:x) + #{'m.call;'*256} + end + def awm + m = method(:x=) + #{'m.call(nil);'*256} + end + END + end + obj = C.new +benchmark: + attr_reader: "obj.ar" + attr_writer: "obj.aw" + attr_reader_method: "obj.arm" + attr_writer_method: "obj.awm" diff --git a/iseq.c b/iseq.c index b08f1bb..3dd7bea 100644 --- a/iseq.c +++ b/iseq.c @@ -3407,6 +3407,32 @@ rb_iseq_trace_set(const rb_iseq_t *iseq, rb_event_flag_t turnon_events) https://github.com/ruby/ruby/blob/trunk/iseq.c#L3407 } } +bool rb_vm_call_ivar_attrset_p(const vm_call_handler ch); +void rb_vm_cc_general(const struct rb_callcache *cc); + +static int +clear_attr_ccs_i(void *vstart, void *vend, size_t stride, void *data) +{ + VALUE v = (VALUE)vstart; + for (; v != (VALUE)vend; v += stride) { + void *ptr = asan_poisoned_object_p(v); + asan_unpoison_object(v, false); + + if (imemo_type_p(v, imemo_callcache) && rb_vm_call_ivar_attrset_p(((const struct rb_callcache *)v)->call_)) { + rb_vm_cc_general((struct rb_callcache *)v); + } + + asan_poison_object_if(ptr, v); + } + return 0; +} + +void +rb_clear_attr_ccs(void) +{ + rb_objspace_each_objects(clear_attr_ccs_i, NULL); +} + static int trace_set_i(void *vstart, void *vend, size_t stride, void *data) { @@ -3420,6 +3446,9 @@ trace_set_i(void *vstart, void *vend, size_t stride, void *data) https://github.com/ruby/ruby/blob/trunk/iseq.c#L3446 if (rb_obj_is_iseq(v)) { rb_iseq_trace_set(rb_iseq_check((rb_iseq_t *)v), turnon_events); } + else if (imemo_type_p(v, imemo_callcache) && rb_vm_call_ivar_attrset_p(((const struct rb_callcache *)v)->call_)) { + rb_vm_cc_general((struct rb_callcache *)v); + } asan_poison_object_if(ptr, v); } diff --git a/test/ruby/test_settracefunc.rb b/test/ruby/test_settracefunc.rb index 1393caa..88488ae 100644 --- a/test/ruby/test_settracefunc.rb +++ b/test/ruby/test_settracefunc.rb @@ -831,6 +831,56 @@ class TestSetTraceFunc < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_settracefunc.rb#L831 } end + def test_tracepoint_attr + c = Class.new do + attr_accessor :x + alias y x + alias y= x= + end + obj = c.new + + ar_meth = obj.method(:x) + aw_meth = obj.method(:x=) + aar_meth = obj.method(:y) + aaw_meth = obj.method(:y=) + events = [] + trace = TracePoint.new(:c_call, :c_return){|tp| + next if !target_thread? + next if tp.path != __FILE__ + next if tp.method_id == :call + case tp.event + when :c_call + assert_raise(RuntimeError) {tp.return_value} + events << [tp.event, tp.method_id, tp.callee_id] + when :c_return + events << [tp.event, tp.method_id, tp.callee_id, tp.return_value] + end + } + test_proc = proc do + obj.x = 1 + obj.x + obj.y = 2 + obj.y + aw_meth.call(1) + ar_meth.call + aaw_meth.call(2) + aar_meth.call + end + test_proc.call # populate call caches + trace.enable(&test_proc) + expected = [ + [:c_call, :x=, :x=], + [:c_return, :x=, :x=, 1], + [:c_call, :x, :x], + [:c_return, :x, :x, 1], + [:c_call, :x=, :y=], + [:c_return, :x=, :y=, 2], + [:c_call, :x, :y], + [:c_return, :x, :y, 2], + ] + assert_equal(expected*2, events) + end + class XYZZYException < Exception; end def method_test_tracepoint_raised_exception err raise err diff --git a/vm_eval.c b/vm_eval.c index 8a0166e..a727ea8 100644 --- a/vm_eval.c +++ b/vm_eval.c @@ -190,7 +190,16 @@ vm_call0_body(rb_execution_context_t *ec, struct rb_calling_info *calling, const https://github.com/ruby/ruby/blob/trunk/vm_eval.c#L190 } rb_check_arity(calling->argc, 1, 1); - ret = rb_ivar_set(calling->recv, vm_cc_cme(cc)->def->body.attr.id, argv[0]); + if (UNLIKELY(ruby_vm_event_flags & (RUBY_EVENT_C_CALL | RUBY_EVENT_C_RETURN))) { + EXEC_EVENT_HOOK(ec, RUBY_EVENT_C_CALL, calling->recv, + vm_cc_cme(cc)->def->original_id, vm_ci_mid(ci), vm_cc_cme(cc)->owner, Qundef); + ret = rb_ivar_set(calling->recv, vm_cc_cme(cc)->def->body.attr.id, argv[0]); + EXEC_EVENT_HOOK(ec, RUBY_EVENT_C_RETURN, calling->recv, + vm_cc_cme(cc)->def->original_id, vm_ci_mid(ci), vm_cc_cme(cc)->owner, ret); + } + else { + ret = rb_ivar_set(calling->recv, vm_cc_cme(cc)->def->body.attr.id, argv[0]); + } goto success; case VM_METHOD_TYPE_IVAR: if (calling->kw_splat && @@ -201,7 +210,16 @@ vm_call0_body(rb_execution_context_t *ec, struct rb_calling_info *calling, const https://github.com/ruby/ruby/blob/trunk/vm_eval.c#L210 } rb_check_arity(calling->argc, 0, 0); - ret = rb_attr_get(calling->recv, vm_cc_cme(cc)->def->body.attr.id); + if (UNLIKELY(ruby_vm_event_flags & (RUBY_EVENT_C_CALL | RUBY_EVENT_C_RETURN))) { + EXEC_EVENT_HOOK(ec, RUBY_EVENT_C_CALL, calling->recv, + vm_cc_cme(cc)->def->original_id, vm_ci_mid(ci), vm_cc_cme(cc)->owner, Qundef); + ret = rb_attr_get(calling->recv, vm_cc_cme(cc)->def->body.attr.id); + EXEC_EVENT_HOOK(ec, RUBY_EVENT_C_RETURN, calling->recv, + vm_cc_cme(cc)->def->original_id, vm_ci_mid(ci), vm_cc_cme(cc)->owner, ret); + } + else { + ret = rb_attr_get(calling->recv, vm_cc_cme(cc)->def->body.attr.id); + } goto success; case VM_METHOD_TYPE_BMETHOD: ret = vm_call_bmethod_body(ec, calling, argv); diff --git a/vm_insnhelper.c b/vm_insnhelper.c index e186376..e08ba34 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -3029,6 +3029,12 @@ vm_call_attrset(rb_execution_context_t *ec, rb_control_frame_t *cfp, struct rb_c https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L3029 return vm_setivar(calling->recv, vm_cc_cme(cc)->def->body.attr.id, val, NULL, NULL, cc, 1); } +bool +rb_vm_call_ivar_attrset_p(const vm_call_handler ch) +{ + return (ch == vm_call_ivar || ch == vm_call_attrset); +} + static inline VALUE vm_call_bmethod_body(rb_execution_context_t *ec, struct rb_calling_info *calling, const VALUE *argv) { @@ -3484,16 +3490,40 @@ vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp, st https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L3490 rb_check_arity(calling->argc, 1, 1); vm_cc_attr_index_set(cc, 0); - CC_SET_FASTPATH(cc, vm_call_attrset, !(vm_ci_flag(ci) & (VM_CALL_ARGS_SPLAT | VM_CALL_KW_SPLAT | VM_CALL_KWARG))); - return vm_call_attrset(ec, cfp, calling); + if (UNLIKELY(ruby_vm_event_flags & (RUBY_EVENT_C_CALL | RUBY_EVENT_C_RETURN))) { + const struct rb_callinfo *ci = calling->ci; + VALUE v; + EXEC_EVENT_HOOK(ec, RUBY_EVENT_C_CALL, calling->recv, vm_cc_cme(cc)->def->original_id, + vm_ci_mid(ci), vm_cc_cme(cc)->owner, Qundef); + v = vm_call_attrset(ec, cfp, calling); + EXEC_EVENT_HOOK(ec, RUBY_EVENT_C_RETURN, calling->recv, vm_cc_cme(cc)->def->original_id, + vm_ci_mid(ci), vm_cc_cme(cc)->owner, v); + return v; + } + else { + CC_SET_FASTPATH(cc, vm_call_attrset, !(vm_ci_flag(ci) & (VM_CALL_ARGS_SPLAT | VM_CALL_KW_SPLAT | VM_CALL_KWARG))); + return vm_call_attrset(ec, cfp, calling); + } case VM_METHOD_TYPE_IVAR: CALLER_SETUP_ARG(cfp, calling, ci); CALLER_REMOVE_EMPTY_KW_SPLAT(cfp, calling, ci); rb_check_arity(calling->argc, 0, 0); vm_cc_attr_index_set(cc, 0); - CC_SET_FASTPATH(cc, vm_call_ivar, !(vm_ci_flag(ci) & (VM_CALL_ARGS_SPLAT | VM_CALL_KW (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/