Message ID | 1398716623-16991-1-git-send-email-marc.khouzam@ericsson.com |
---|---|
State | Changes Requested, archived |
Headers |
Return-Path: <x14314964@homiemail-mx22.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx22.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 9F44D360078 for <siddhesh@wilcox.dreamhost.com>; Mon, 28 Apr 2014 13:24:12 -0700 (PDT) Received: by homiemail-mx22.g.dreamhost.com (Postfix, from userid 14314964) id 06B484FB0E7E; Mon, 28 Apr 2014 13:24:11 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx22.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx22.g.dreamhost.com (Postfix) with ESMTPS id DF1DF50DE964 for <gdb@patchwork.siddhesh.in>; Mon, 28 Apr 2014 13:24:07 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id :mime-version:content-type; q=dns; s=default; b=Rfg9TrUSn4Xa6VV/ WwBwytDpjELu+36jTorad8SY8LK64KKFIJ9QH5eSlldJE/gYYuTHPU9z1WBr+uIx p/Lo0JtbNdAAPCKtC6vCQ6tqSVO+LHIdk8f3cdpMR8TP44Rc591tBXl5/IFRdxjr q100X4NjZwC6n+rt9dO9YkuCxM4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id :mime-version:content-type; s=default; bh=Q6dHy4kOsqVqq1d12YLozK nLHFk=; b=WgjqyKYzscaCoA4KkL2qxxPxP5mT9xl2PT5ys/2C0eSqNkjEAYWKa0 GqKNqy8mS3db5z7jUSplmR+NJgxfWh0kogdcMlLPPChb1JqBBXGnhYMobXZMzBtH wGRY/TQrHvuKMJEBaiwFZjdFsTsiyXabF0RoI82hELYzO289BUQWs= Received: (qmail 32733 invoked by alias); 28 Apr 2014 20:24:05 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-gdb=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 32717 invoked by uid 89); 28 Apr 2014 20:24:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg20.ericsson.net Received: from usevmg20.ericsson.net (HELO usevmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 28 Apr 2014 20:24:03 +0000 Received: from EUSAAHC001.ericsson.se (Unknown_Domain [147.117.188.75]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id 97.9D.07420.15A6E535; Mon, 28 Apr 2014 16:48:50 +0200 (CEST) Received: from elx67nvvz1-ei.dyn.mo.ca.am.ericsson.se (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.75) with Microsoft SMTP Server (TLS) id 14.3.174.1; Mon, 28 Apr 2014 16:24:00 -0400 From: Marc Khouzam <marc.khouzam@ericsson.com> To: <gdb-patches@sourceware.org> CC: Marc Khouzam <marc.khouzam@ericsson.com> Subject: [PATCH] PR breakpoints/15697: Remove =breakpoint-modified when hitting dprintf Date: Mon, 28 Apr 2014 16:23:43 -0400 Message-ID: <1398716623-16991-1-git-send-email-marc.khouzam@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in |
Commit Message
Marc Khouzam
April 28, 2014, 8:23 p.m. UTC
GDB currently sends a =breakpoint-modified for every dprintf hit. This can cause performance degradation at the frontend. The below patch prevents GDB from sending this event for dprintf when the event is triggered by the dprintf being hit. This means the event is not sent when the hit-count is incremented or the ignore-count is decremented due to a hit. https://sourceware.org/bugzilla/show_bug.cgi?id=15697 This was discussed last year: http://sourceware.org/ml/gdb-patches/2013-03/msg00260.html No regressions. Ok? Thanks Marc --- DPrintfs can be hit very often since they don't interrupt the execution. Having a =breakpoint-modified event at every hit can cause performance degradation at the frontend. The only value in this event is to indicate that the hit-count has changed. For the hit-count value however, it is sufficient for a frontend to get the latest value upon request and not through an asynchronous event. We also remove =breakpoint-modified when hitting a dprintf and decrementing the ignore count. If the ignore count is set to a high number, the =breakpoint-modified could also cause performance degradation as it will be sent at every dprintf hit which decrements the ignore count. 2014-04-28 Marc Khouzam <marc.khouzam@ericsson.com> PR breakpoints/15697 * breakpoint.c (bpstat_check_breakpoint_conditions): Don't call observer_notify_breakpoint_modified for dprintf. * breakpoint.c (bpstat_stop_status): Ditto. --- gdb/breakpoint.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Comments
On Mon, Apr 28, 2014 at 1:23 PM, Marc Khouzam <marc.khouzam@ericsson.com> wrote: > GDB currently sends a =breakpoint-modified for every dprintf hit. > This can cause performance degradation at the frontend. The below > patch prevents GDB from sending this event for dprintf when the > event is triggered by the dprintf being hit. This means the event > is not sent when the hit-count is incremented or the ignore-count > is decremented due to a hit. > > https://sourceware.org/bugzilla/show_bug.cgi?id=15697 > > This was discussed last year: > http://sourceware.org/ml/gdb-patches/2013-03/msg00260.html > > No regressions. > > Ok? > > Thanks > > Marc > > --- > > DPrintfs can be hit very often since they don't interrupt > the execution. Having a =breakpoint-modified event at > every hit can cause performance degradation at the > frontend. The only value in this event is to indicate that > the hit-count has changed. For the hit-count value however, > it is sufficient for a frontend to get the latest value upon > request and not through an asynchronous event. > > We also remove =breakpoint-modified when hitting a dprintf > and decrementing the ignore count. If the ignore count is > set to a high number, the =breakpoint-modified could also > cause performance degradation as it will be sent at every > dprintf hit which decrements the ignore count. > > 2014-04-28 Marc Khouzam <marc.khouzam@ericsson.com> > > PR breakpoints/15697 > * breakpoint.c (bpstat_check_breakpoint_conditions): > Don't call observer_notify_breakpoint_modified for dprintf. > * breakpoint.c (bpstat_stop_status): Ditto. Nit. No need to write "* breakpoint.c" twice. > --- > gdb/breakpoint.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index f422998..25615eb 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -5378,7 +5378,8 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid) > bs->stop = 0; > /* Increase the hit count even though we don't stop. */ > ++(b->hit_count); > - observer_notify_breakpoint_modified (b); > + if (b->type != bp_dprintf) > + observer_notify_breakpoint_modified (b); > } > } > > @@ -5515,7 +5516,8 @@ bpstat_stop_status (struct address_space *aspace, > if (bs->stop) > { > ++(b->hit_count); > - observer_notify_breakpoint_modified (b); > + if (b->type != bp_dprintf) > + observer_notify_breakpoint_modified (b); > > /* We will stop here. */ > if (b->disposition == disp_disable) > -- > 1.7.9.5 > Hi. I've read the mentioned thread and I agree with your analysis. The patch is ok with me, but give it a few days for others to comment. Adding a comment explaining why the test is present might be useful, but I'm ok with skipping it. E.g., something like /* Don't send notifications for dprintf, there can be a lot and aren't useful to frontends. */
On 04/28/2014 09:23 PM, Marc Khouzam wrote: > GDB currently sends a =breakpoint-modified for every dprintf hit. > This can cause performance degradation at the frontend. The below > patch prevents GDB from sending this event for dprintf when the > event is triggered by the dprintf being hit. This means the event > is not sent when the hit-count is incremented or the ignore-count > is decremented due to a hit. > > https://sourceware.org/bugzilla/show_bug.cgi?id=15697 > > This was discussed last year: > http://sourceware.org/ml/gdb-patches/2013-03/msg00260.html > > No regressions. > > Ok? > > Thanks > > Marc > > --- > > DPrintfs can be hit very often since they don't interrupt > the execution. Having a =breakpoint-modified event at > every hit can cause performance degradation at the > frontend. The only value in this event is to indicate that > the hit-count has changed. For the hit-count value however, > it is sufficient for a frontend to get the latest value upon > request and not through an asynchronous event. > > We also remove =breakpoint-modified when hitting a dprintf > and decrementing the ignore count. If the ignore count is > set to a high number, the =breakpoint-modified could also > cause performance degradation as it will be sent at every > dprintf hit which decrements the ignore count. > > 2014-04-28 Marc Khouzam <marc.khouzam@ericsson.com> > > PR breakpoints/15697 > * breakpoint.c (bpstat_check_breakpoint_conditions): > Don't call observer_notify_breakpoint_modified for dprintf. > * breakpoint.c (bpstat_stop_status): Ditto. > --- > gdb/breakpoint.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index f422998..25615eb 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -5378,7 +5378,8 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid) > bs->stop = 0; > /* Increase the hit count even though we don't stop. */ > ++(b->hit_count); > - observer_notify_breakpoint_modified (b); > + if (b->type != bp_dprintf) > + observer_notify_breakpoint_modified (b); Sorry to be a naysayer, but I don't think treating dprintf any different from other breakpoints here is a good idea. You get the exact same flood with setting an ignore count an any breakpoint whose condition evaluates false. The simplest being "break foo if 0". So we get to apply some kind of treatment to all kinds of breakpoints, or to none. > @@ -5515,7 +5516,8 @@ bpstat_stop_status (struct address_space *aspace, > if (bs->stop) > { > ++(b->hit_count); > - observer_notify_breakpoint_modified (b); > + if (b->type != bp_dprintf) > + observer_notify_breakpoint_modified (b); This one is tricker -- at first blush, this looks odd -- bs->stop is set, so this path would seem to mean we're stopping. If one emulates dprintfs with "b foo if return_false ()", we notice that that breakpoints doesn't get its hit count incremented, even though internally GDB sees the breakpoints hits trigger. But in the dprintfs case, we get here with bs->stop set, we increment the hit_count, and then a bit below, we call after_condition_true: if (bs->stop) { ++(b->hit_count); observer_notify_breakpoint_modified (b); /* We will stop here. */ ... b->ops->after_condition_true (bs); and dprintf's implementation has: static void dprintf_after_condition_true (struct bpstats *bs) { /* dprintf's never cause a stop. This wasn't set in the check_status hook instead because that would make the dprintf's condition not be evaluated. */ bs->stop = 0; This has the effect that "b foo if return_false()" doesn't result in the hit incrementing, while it does get incremented for dprintfs: (gdb) info breakpoints Num Type Disp Enb Address What 4 dprintf keep y 0x00000000004007b7 in thread_function0 at threads.c:63 breakpoint already hit 1329 times printf "asdf" 5 breakpoint keep y 0x00000000004007b7 in thread_function0 at threads.c:63 stop only if return_false () I suppose that the hit count incrementing for dprintfs does make sense. So this change here would need at least a comment explaining why dprintfs are special here. But even ignoring the comments above, this change here isn't right because just below we have: if (bs->stop) { ++(b->hit_count); observer_notify_breakpoint_modified (b); /* We will stop here. */ if (b->disposition == disp_disable) { --(b->enable_count); if (b->enable_count <= 0 && b->enable_state != bp_permanent) b->enable_state = bp_disabled; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ removed_any = 1; } so if the user sets an enable count on a dprintf-style gdb dprintf, we lose the MI notification when the dprintf ends up disabled. Is that OK? And if OK for dprintfs, why not for other kinds of breakpoints? I'm not sure I agree that "it is sufficient for a frontend to get the latest value upon request and not through an asynchronous event.". Completely disabling this notification means that the frontend gets no notification when the enable or ignore counts are reached. Not sure that's disirable. Also when you have another breakpoint set at the same address, that does cause a stop, the frontend has no clue that the dprintf hit counts changed -- basically you're saying that for dprintfs, frontends _always_ need to refresh the info from gdb after a stop. This at least needs a documentation update. To avoid that, GDB could store the last hit count value it reported to the frontend, and delay sending the breakpoint changed notification to when it tells the frontend about a stop. I mildly wonder also whether a different frontend might have different preferences here. Related, I'm not sure we should filter the observer_notify_breakpoint_modified call -- it would seem better if observers are notified, and the its MI that filters out those modifications that it isn't interested in. E.g., could TUI be interested in still receiving the nofications? Maybe we should split the notification in two (modified / counters-modified). I don't have answers to the above, but I think these issues all need to be considered, and whatever resolutions we end up with need to be cast as comments in the code and manual. Thanks,
> -----Original Message----- > From: Pedro Alves [mailto:palves@redhat.com] > Sent: Tuesday, April 29, 2014 1:01 PM > To: Marc Khouzam > Cc: gdb-patches@sourceware.org > Subject: Re: [PATCH] PR breakpoints/15697: Remove =breakpoint-modified > when hitting dprintf [Bunch of very valid concerns] > Related, I'm not sure we should filter the > observer_notify_breakpoint_modified call -- it would seem better if > observers are notified, and the its MI that filters out those modifications that > it isn't interested in. > E.g., could TUI be interested in still receiving the nofications? Something to think about. Simon made the same comment off-line. > I don't have answers to the above, but I think these issues all need to be > considered, and whatever resolutions we end up with need to be cast as > comments in the code and manual. Thanks Pedro, you are bringing up valid (and tricky :-)) points. I'll look into them and see if I can come up with equally valid solutions. Marc
> -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] On Behalf Of Marc Khouzam > Sent: Tuesday, April 29, 2014 1:48 PM > To: 'Pedro Alves' > Cc: 'gdb-patches@sourceware.org' > Subject: RE: [PATCH] PR breakpoints/15697: Remove =breakpoint-modified > when hitting dprintf > > > -----Original Message----- > > From: Pedro Alves [mailto:palves@redhat.com] > > Sent: Tuesday, April 29, 2014 1:01 PM > > To: Marc Khouzam > > Cc: gdb-patches@sourceware.org > > Subject: Re: [PATCH] PR breakpoints/15697: Remove =breakpoint-modified > > when hitting dprintf > > [Bunch of very valid concerns] > > > Related, I'm not sure we should filter the > > observer_notify_breakpoint_modified call -- it would seem better if > > observers are notified, and the its MI that filters out those > > modifications that it isn't interested in. > > E.g., could TUI be interested in still receiving the nofications? > > Something to think about. Simon made the same comment off-line. > > > I don't have answers to the above, but I think these issues all need > > to be considered, and whatever resolutions we end up with need to be > > cast as comments in the code and manual. > > Thanks Pedro, you are bringing up valid (and tricky :-)) points. I'll look into > them and see if I can come up with equally valid solutions. To follow up on this. After discussing with Pedro offline, he helped point-out that the many =breakpoint-modified notifications were not actually causing any problems to Eclipse. The issue was actually in the corresponding extra *running event. Pedro has already followed-up on a fix for the *running event here (thanks!): https://sourceware.org/ml/gdb-patches/2014-05/msg00273.html And I'm dropping this patch, as it is not helpful. Thanks Marc
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index f422998..25615eb 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -5378,7 +5378,8 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid) bs->stop = 0; /* Increase the hit count even though we don't stop. */ ++(b->hit_count); - observer_notify_breakpoint_modified (b); + if (b->type != bp_dprintf) + observer_notify_breakpoint_modified (b); } } @@ -5515,7 +5516,8 @@ bpstat_stop_status (struct address_space *aspace, if (bs->stop) { ++(b->hit_count); - observer_notify_breakpoint_modified (b); + if (b->type != bp_dprintf) + observer_notify_breakpoint_modified (b); /* We will stop here. */ if (b->disposition == disp_disable)