From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24125 invoked by alias); 22 Sep 2011 17:12:38 -0000 Received: (qmail 24117 invoked by uid 22791); 22 Sep 2011 17:12:37 -0000 X-SWARE-Spam-Status: No, hits=-0.3 required=5.0 tests=AWL,BAYES_00,SARE_SUB_IMPROVE,TO_NO_BRKTS_DIRECT X-Spam-Check-By: sourceware.org Received: from gbenson.demon.co.uk (HELO gbenson.demon.co.uk) (80.177.220.214) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Sep 2011 17:12:17 +0000 Date: Thu, 22 Sep 2011 17:35:00 -0000 From: Gary Benson To: gdb-patches@sourceware.org Subject: [RFA] Improve performance with lots of shared libraries Message-ID: <20110922171206.GB5874@redhat.com> Mail-Followup-To: gdb-patches@sourceware.org MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="ReaqsoxgOBHFXBhH" Content-Disposition: inline X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-09/txt/msg00411.txt.bz2 --ReaqsoxgOBHFXBhH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 2542 Hi all, Back in July I submitted a patch[1] that improved performance when debugging inferiors that load lots of shared libraries. It hinged on avoiding calling find_pc_partial_function and skip_inline_frames in handle_inferior_event when the event being handled was a stop at the solib event breakpoint. That patch was kind of hacky in that it relied on second-guessing some of the logic that followed. I started working on splitting the patch, and committed a patch to make find_pc_partial_function lazy[2] in July. Attached is a patch to make gdb avoid the call to skip_inline_frames. With a small benchmark I put together (a little program that loads 1000 shared libraries) gdb ran the application in 36s with the patch against 46s without. This patch is superficially similar to the original patch, but much simpler and different in scope. The initial patch worked on the premise that if the stop was specifially a stop for the solib event breakpoint then the calls could be skipped (because I'd walked through the code ahead and seen that their results were not used). The result was a fairly complex conditional that would have needed to be kept updated if other parts of handle_inferior_event changed. This patch works on the simpler premise that the solib event breakpoint is by definition the address of a function, ie not inline, so regardless of why the stop occurred the call to skip_inline_frames can be omitted because there are no inline frames to skip. The heuristic is therefore simpler and very much less fragile. I have tried a two of other approaches to this, but neither worked. My preferred method was to make skip_inline_frames lazy, but this caused problems because the skipping, when it occurred, invalidated the frame cache. handle_inferior_event has several places where local "frame" variables are recreated because some call or another has invalidated the frame cache and left the pointer dangling, and making skip_inline_frames added many more. It was looking like a maintainence nightmaire long before I even got it to work. The other approach I tried was to use bpstat_stop_status to identify the solib event breakpoint, rather than writing my own code to do it. This looked really clean, but failed a huge number of tests. Apparently bpstat_stop_status has side effects! How is this patch do you think? Is it ok to commit? Cheers, Gary [1] http://www.cygwin.com/ml/gdb-patches/2011-07/msg00026.html [2] http://www.cygwin.com/ml/gdb-patches/2011-07/msg00460.html -- http://gbenson.net/ --ReaqsoxgOBHFXBhH Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=patch Content-length: 2438 2011-09-22 Gary Benson * infrun.c (stopped_at_solib_event_breakpoint): New function. (stopped_at_solib_event_breakpoint_helper): Likewise. (handle_inferior_event): Avoid calling skip_inline_frames when at the solib event breakpoint. diff --git a/gdb/infrun.c b/gdb/infrun.c index 225034c..2e49470 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3102,6 +3102,39 @@ fill_in_stop_func (struct gdbarch *gdbarch, } } +/* Helper for stopped_at_solib_event_breakpoint. */ + +static int +stopped_at_solib_event_breakpoint_helper (struct breakpoint *b, void *arg) +{ + struct execution_control_state *ecs + = (struct execution_control_state *) arg; + + if (b->type == bp_shlib_event) + { + CORE_ADDR prev_pc = ecs->event_thread->prev_pc; + struct bp_location *loc; + + for (loc = b->loc; loc; loc = loc->next) + { + if (loc->pspace == current_program_space + && (loc->address == stop_pc || loc->address == prev_pc)) + return 1; + } + } + + return 0; +} + +/* Nonzero if we are stopped at the solib event breakpoint. */ + +static int +stopped_at_solib_event_breakpoint (struct execution_control_state *ecs) +{ + return iterate_over_breakpoints (stopped_at_solib_event_breakpoint_helper, + ecs) != NULL; +} + /* Given an execution control state that has been freshly filled in by an event from the inferior, figure out what it means and take appropriate action. */ @@ -4010,9 +4043,14 @@ handle_inferior_event (struct execution_control_state *ecs) stopped_by_random_signal = 0; /* Hide inlined functions starting here, unless we just performed stepi or - nexti. After stepi and nexti, always show the innermost frame (not any - inline function call sites). */ - if (ecs->event_thread->control.step_range_end != 1) + nexti, or we are at the solib event breakpoint. After stepi and nexti, + always show the innermost frame (not any inline function call sites). + This call is expensive, and we avoid it if we are at the solib event + breakpoint which is defined as the address of a function (i.e., not + inline). This improves performance with inferiors that load a lot of + shared libraries. */ + if (ecs->event_thread->control.step_range_end != 1 + && !stopped_at_solib_event_breakpoint (ecs)) skip_inline_frames (ecs->ptid); if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP --ReaqsoxgOBHFXBhH--