ruby-changes:62736
From: Jeremy <ko1@a...>
Date: Fri, 28 Aug 2020 07:17:55 +0900 (JST)
Subject: [ruby-changes:62736] 3b24b7914c (master): Improve performance of partial backtraces
https://git.ruby-lang.org/ruby.git/commit/?id=3b24b7914c From 3b24b7914c16930bfadc89d6aff6326a51c54295 Mon Sep 17 00:00:00 2001 From: Jeremy Evans <code@j...> Date: Thu, 27 Aug 2020 15:17:36 -0700 Subject: Improve performance of partial backtraces Previously, backtrace_each fully populated the rb_backtrace_t with all backtrace frames, even if caller only requested a partial backtrace (e.g. Kernel#caller_locations(1, 1)). This changes backtrace_each to only add the requested frames to the rb_backtrace_t. To do this, backtrace_each needs to be passed the starting frame and number of frames values passed to Kernel#caller or #caller_locations. backtrace_each works from the top of the stack to the bottom, where the bottom is the current frame. Due to how the location for cfuncs is tracked using the location of the previous iseq, we need to store an extra frame for the previous iseq if we are limiting the backtrace and final backtrace frame (the first one stored) would be a cfunc and not an iseq. To limit the amount of work in this case, while scanning until the start of the requested backtrace, for each iseq, store the cfp. If the first backtrace frame we care about is a cfunc, use the stored cfp to find the related iseq. Use a function pointer to handle the storage of the cfp in the iteration arg, and also store the location of the extra frame in the iteration arg. backtrace_each needs to return int instead of void in order to signal when a starting frame larger than backtrace size is given, as caller and caller_locations needs to return nil and not the empty array in these cases. To handle cases where a range is provided with a negative end, and the backtrace size is needed to calculate the result to pass to rb_range_beg_len, add a backtrace_size static function to calculate the size, which copies the logic from backtrace_each. As backtrace_each only adds the backtrace lines requested, backtrace_to_*_ary can be simplified to always operate on the entire backtrace. Previously, caller_locations(1,1) was about 6.2 times slower for an 800 deep callstack compared to an empty callstack. With this new approach, it is only 1.3 times slower. It will always be somewhat slower as it still needs to scan the cfps from the top of the stack until it finds the first requested backtrace frame. This initializes the backtrace memory to zero. I do not think this is necessary, as from my analysis, nothing during the setting of the backtrace entries can cause a garbage collection, but it seems the safest approach, and it's unlikely the performance decrease is significant. This removes the rb_backtrace_t backtrace_base member. backtrace and backtrace_base were initialized to the same value, and neither is modified, so it doesn't make sense to have two pointers. This also removes LOCATION_TYPE_IFUNC from vm_backtrace.c, as the value is never set. Fixes [Bug #17031] diff --git a/vm_backtrace.c b/vm_backtrace.c index ba99527..bb650a6 100644 --- a/vm_backtrace.c +++ b/vm_backtrace.c @@ -29,6 +29,9 @@ id2str(ID id) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L29 } #define rb_id2str(id) id2str(id) +#define BACKTRACE_START 0 +#define ALL_BACKTRACE_LINES -1 + inline static int calc_lineno(const rb_iseq_t *iseq, const VALUE *pc) { @@ -87,7 +90,6 @@ typedef struct rb_backtrace_location_struct { https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L90 LOCATION_TYPE_ISEQ = 1, LOCATION_TYPE_ISEQ_CALCED, LOCATION_TYPE_CFUNC, - LOCATION_TYPE_IFUNC } type; union { @@ -126,7 +128,6 @@ location_mark_entry(rb_backtrace_location_t *fi) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L128 rb_gc_mark_movable((VALUE)fi->body.iseq.iseq); break; case LOCATION_TYPE_CFUNC: - case LOCATION_TYPE_IFUNC: default: break; } @@ -196,7 +197,6 @@ location_label(rb_backtrace_location_t *loc) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L197 return loc->body.iseq.iseq->body->location.label; case LOCATION_TYPE_CFUNC: return rb_id2str(loc->body.cfunc.mid); - case LOCATION_TYPE_IFUNC: default: rb_bug("location_label: unreachable"); UNREACHABLE; @@ -245,7 +245,6 @@ location_base_label(rb_backtrace_location_t *loc) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L245 return loc->body.iseq.iseq->body->location.base_label; case LOCATION_TYPE_CFUNC: return rb_id2str(loc->body.cfunc.mid); - case LOCATION_TYPE_IFUNC: default: rb_bug("location_base_label: unreachable"); UNREACHABLE; @@ -275,7 +274,6 @@ location_path(rb_backtrace_location_t *loc) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L274 return location_path(loc->body.cfunc.prev_loc); } return Qnil; - case LOCATION_TYPE_IFUNC: default: rb_bug("location_path: unreachable"); UNREACHABLE; @@ -308,7 +306,6 @@ location_realpath(rb_backtrace_location_t *loc) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L306 return location_realpath(loc->body.cfunc.prev_loc); } return Qnil; - case LOCATION_TYPE_IFUNC: default: rb_bug("location_realpath: unreachable"); UNREACHABLE; @@ -373,7 +370,6 @@ location_to_str(rb_backtrace_location_t *loc) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L370 } name = rb_id2str(loc->body.cfunc.mid); break; - case LOCATION_TYPE_IFUNC: default: rb_bug("location_to_str: unreachable"); } @@ -402,7 +398,6 @@ location_inspect_m(VALUE self) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L398 typedef struct rb_backtrace_struct { rb_backtrace_location_t *backtrace; - rb_backtrace_location_t *backtrace_base; int backtrace_size; VALUE strary; VALUE locary; @@ -425,7 +420,7 @@ static void https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L420 backtrace_free(void *ptr) { rb_backtrace_t *bt = (rb_backtrace_t *)ptr; - if (bt->backtrace) ruby_xfree(bt->backtrace_base); + if (bt->backtrace) ruby_xfree(bt->backtrace); ruby_xfree(bt); } @@ -438,7 +433,6 @@ location_update_entry(rb_backtrace_location_t *fi) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L433 fi->body.iseq.iseq = (rb_iseq_t*)rb_gc_location((VALUE)fi->body.iseq.iseq); break; case LOCATION_TYPE_CFUNC: - case LOCATION_TYPE_IFUNC: default: break; } @@ -484,22 +478,47 @@ backtrace_alloc(VALUE klass) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L478 return obj; } -static void +static long +backtrace_size(const rb_execution_context_t *ec) +{ + const rb_control_frame_t *last_cfp = ec->cfp; + const rb_control_frame_t *start_cfp = RUBY_VM_END_CONTROL_FRAME(ec); + + if (start_cfp == NULL) { + return -1; + } + + start_cfp = + RUBY_VM_NEXT_CONTROL_FRAME( + RUBY_VM_NEXT_CONTROL_FRAME(start_cfp)); /* skip top frames */ + + if (start_cfp < last_cfp) { + return 0; + } + + return start_cfp - last_cfp + 1; +} + +static int backtrace_each(const rb_execution_context_t *ec, + ptrdiff_t from_last, + long num_frames, void (*init)(void *arg, size_t size), void (*iter_iseq)(void *arg, const rb_control_frame_t *cfp), void (*iter_cfunc)(void *arg, const rb_control_frame_t *cfp, ID mid), + void (*iter_skip)(void *arg, const rb_control_frame_t *cfp), void *arg) { const rb_control_frame_t *last_cfp = ec->cfp; const rb_control_frame_t *start_cfp = RUBY_VM_END_CONTROL_FRAME(ec); const rb_control_frame_t *cfp; - ptrdiff_t size, i; + ptrdiff_t size, i, last, start = 0; + int ret = 0; // In the case the thread vm_stack or cfp is not initialized, there is no backtrace. if (start_cfp == NULL) { init(arg, 0); - return; + return ret; } /* <- start_cfp (end control frame) @@ -517,16 +536,43 @@ backtrace_each(const rb_execution_context_t *ec, https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L536 RUBY_VM_NEXT_CONTROL_FRAME(start_cfp)); /* skip top frames */ if (start_cfp < last_cfp) { - size = 0; + size = last = 0; } else { size = start_cfp - last_cfp + 1; + + if (from_last > size) { + size = last = 0; + ret = 1; + } + else if (num_frames >= 0 && num_frames < size) { + if (from_last + num_frames > size) { + size -= from_last; + last = size; + } + else { + start = size - from_last - num_frames; + size = num_frames; + last = start + size; + } + } + else { + size -= from_last; + last = size; + } } init(arg, size); /* SDR(); */ - for (i=0, cfp = start_cfp; i<size; i++, cfp = RUBY_VM_NEXT_CONTROL_FRAME(cfp)) { + for (i=0, cfp = start_cfp; i<last; i++, cfp = RUBY_VM_NEXT_CONTROL_FRAME(cfp)) { + if (i < start) { + if (iter_skip) { + iter_skip(arg, cfp); + } + continue; + } + /* fprintf(stderr, "cfp: %d\n", (rb_control_frame_t *)(ec->vm_stack + ec->vm_stack_size) - cfp); */ if (cfp->iseq) { if (cfp->pc) { @@ -540,12 +586,16 @@ backtrace_each(const rb_execution_context_t *ec, https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L586 iter_cfunc(arg, cfp, mid); } } + + return ret; } struct bt_iter_arg { rb_backtrace_t *bt; VALUE btobj; rb_backtrace_location_t *prev_loc; + const rb_control_frame_t *prev_cfp; + rb_backtrace_location_t *init_loc; }; static void @@ -554,8 +604,10 @@ bt_init(void *ptr, size_t size) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L604 struct bt_iter_arg *arg = (struct bt_iter_arg *)ptr; arg->btobj = backtrace_alloc(rb_cBacktrace); GetCoreDataFromValue(arg->btobj, rb_backtrace_t, arg->bt); - arg->bt->backtrace_base = arg->bt->backtrace = ALLOC_N(r (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/