Mailing List Archive: [PATCH v1 1/6] perf annotate: Delete session for debug builds

[PATCH v1 1/6] perf annotate: Delete session for debug builds

Mar 28, 2023, 6:15 PM

Post #1 of 6 (41 views)

Use the debug build indicator as the guide to free the session. This
implements a behavior described in a comment, which is consequentially
removed.

Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/builtin-annotate.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 4750fac7bf93..98d1b6379230 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)

out_delete:
/*
- * Speed up the exit process, for large files this can
- * take quite a while.
- *
- * XXX Enable this when using valgrind or if we ever
- * librarize this command.
- *
- * Also experiment with obstacks to see how much speed
- * up we'll get here.
- *
- * perf_session__delete(session);
+ * Speed up the exit process by only deleting for debug builds. For
+ * large files this can save time.
*/
+#ifndef NDEBUG
+ perf_session__delete(annotate.session);
+#endif
+
return ret;
}
--
2.40.0.348.gf938b09366-goog

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds [ In reply to ]

acme at kernel

Mar 29, 2023, 6:15 AM

Post #2 of 6 (41 views)

Permalink

Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> Use the debug build indicator as the guide to free the session. This
> implements a behavior described in a comment, which is consequentially
> removed.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/builtin-annotate.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 4750fac7bf93..98d1b6379230 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
>
> out_delete:
> /*
> - * Speed up the exit process, for large files this can
> - * take quite a while.
> - *
> - * XXX Enable this when using valgrind or if we ever
> - * librarize this command.
> - *
> - * Also experiment with obstacks to see how much speed
> - * up we'll get here.
> - *
> - * perf_session__delete(session);
> + * Speed up the exit process by only deleting for debug builds. For
> + * large files this can save time.
> */
> +#ifndef NDEBUG
> + perf_session__delete(annotate.session);
> +#endif

So now, but default, we will call this, as we don't have this defined
only if DEBUG=1 is set, right?

?[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
tools/perf/util/mutex.c:#ifndef NDEBUG
?[acme@toolbox perf-tools-next]$

- Arnaldo

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds [ In reply to ]

acme at kernel

Mar 29, 2023, 8:15 AM

Post #3 of 6 (41 views)

Permalink

Em Wed, Mar 29, 2023 at 10:09:48AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> > Use the debug build indicator as the guide to free the session. This
> > implements a behavior described in a comment, which is consequentially
> > removed.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/builtin-annotate.c | 16 ++++++----------
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > index 4750fac7bf93..98d1b6379230 100644
> > --- a/tools/perf/builtin-annotate.c
> > +++ b/tools/perf/builtin-annotate.c
> > @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
> >
> > out_delete:
> > /*
> > - * Speed up the exit process, for large files this can
> > - * take quite a while.
> > - *
> > - * XXX Enable this when using valgrind or if we ever
> > - * librarize this command.
> > - *
> > - * Also experiment with obstacks to see how much speed
> > - * up we'll get here.
> > - *
> > - * perf_session__delete(session);
> > + * Speed up the exit process by only deleting for debug builds. For
> > + * large files this can save time.
> > */
> > +#ifndef NDEBUG
> > + perf_session__delete(annotate.session);
> > +#endif
>
> So now, but default, we will call this, as we don't have this defined
> only if DEBUG=1 is set, right?
>
> ?[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
> tools/perf/util/mutex.c:#ifndef NDEBUG
> ?[acme@toolbox perf-tools-next]$

We can discuss this later, applied the series with just that zfree()
change to annotation_options__exit().

- Arnaldo

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds [ In reply to ]

namhyung at kernel

Mar 29, 2023, 6:15 PM

Post #4 of 6 (41 views)

Permalink

On Wed, Mar 29, 2023 at 6:18?AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Mar 29, 2023 at 10:09:48AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> > > Use the debug build indicator as the guide to free the session. This
> > > implements a behavior described in a comment, which is consequentially
> > > removed.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > > tools/perf/builtin-annotate.c | 16 ++++++----------
> > > 1 file changed, 6 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > > index 4750fac7bf93..98d1b6379230 100644
> > > --- a/tools/perf/builtin-annotate.c
> > > +++ b/tools/perf/builtin-annotate.c
> > > @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
> > >
> > > out_delete:
> > > /*
> > > - * Speed up the exit process, for large files this can
> > > - * take quite a while.
> > > - *
> > > - * XXX Enable this when using valgrind or if we ever
> > > - * librarize this command.
> > > - *
> > > - * Also experiment with obstacks to see how much speed
> > > - * up we'll get here.
> > > - *
> > > - * perf_session__delete(session);
> > > + * Speed up the exit process by only deleting for debug builds. For
> > > + * large files this can save time.
> > > */
> > > +#ifndef NDEBUG
> > > + perf_session__delete(annotate.session);
> > > +#endif
> >
> > So now, but default, we will call this, as we don't have this defined
> > only if DEBUG=1 is set, right?
> >
> > ?[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
> > tools/perf/util/mutex.c:#ifndef NDEBUG
> > ?[acme@toolbox perf-tools-next]$
>
> We can discuss this later, applied the series with just that zfree()
> change to annotation_options__exit().

I don't think it's just an issue in the perf annotate. Maybe we can
do the same for perf report and so on.

Anyway we could define NDEBUG=1 for release builds from now on.

Thanks,
Namhyung

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds [ In reply to ]

acme at kernel

Mar 30, 2023, 6:15 AM

Post #5 of 6 (41 views)

Permalink

Em Wed, Mar 29, 2023 at 05:13:17PM -0700, Namhyung Kim escreveu:
> On Wed, Mar 29, 2023 at 6:18?AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Wed, Mar 29, 2023 at 10:09:48AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> > > > Use the debug build indicator as the guide to free the session. This
> > > > implements a behavior described in a comment, which is consequentially
> > > > removed.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > > tools/perf/builtin-annotate.c | 16 ++++++----------
> > > > 1 file changed, 6 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > > > index 4750fac7bf93..98d1b6379230 100644
> > > > --- a/tools/perf/builtin-annotate.c
> > > > +++ b/tools/perf/builtin-annotate.c
> > > > @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
> > > >
> > > > out_delete:
> > > > /*
> > > > - * Speed up the exit process, for large files this can
> > > > - * take quite a while.
> > > > - *
> > > > - * XXX Enable this when using valgrind or if we ever
> > > > - * librarize this command.
> > > > - *
> > > > - * Also experiment with obstacks to see how much speed
> > > > - * up we'll get here.
> > > > - *
> > > > - * perf_session__delete(session);
> > > > + * Speed up the exit process by only deleting for debug builds. For
> > > > + * large files this can save time.
> > > > */
> > > > +#ifndef NDEBUG
> > > > + perf_session__delete(annotate.session);
> > > > +#endif
> > >
> > > So now, but default, we will call this, as we don't have this defined
> > > only if DEBUG=1 is set, right?
> > >
> > > ?[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
> > > tools/perf/util/mutex.c:#ifndef NDEBUG
> > > ?[acme@toolbox perf-tools-next]$
> >
> > We can discuss this later, applied the series with just that zfree()
> > change to annotation_options__exit().
>
> I don't think it's just an issue in the perf annotate. Maybe we can
> do the same for perf report and so on.

Yes, I thought at some point of setting some flag, perf_exiting, and
then we would stop releasing memory, zfree comes to mind, but then we
would still be traversing data structures, taking locks, etc.

And we can't just exit() as we may need to flush caches, etc.

IIRC this specific case appeared in profiles, so was commented out.

> Anyway we could define NDEBUG=1 for release builds from now on.

Yes, we should do it.

- Arnaldo

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds [ In reply to ]

irogers at google

Mar 30, 2023, 10:15 AM

Post #6 of 6 (41 views)

Permalink

On Thu, Mar 30, 2023 at 4:24?AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Mar 29, 2023 at 05:13:17PM -0700, Namhyung Kim escreveu:
> > On Wed, Mar 29, 2023 at 6:18?AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > Em Wed, Mar 29, 2023 at 10:09:48AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> > > > > Use the debug build indicator as the guide to free the session. This
> > > > > implements a behavior described in a comment, which is consequentially
> > > > > removed.
> > > > >
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > > ---
> > > > > tools/perf/builtin-annotate.c | 16 ++++++----------
> > > > > 1 file changed, 6 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > > > > index 4750fac7bf93..98d1b6379230 100644
> > > > > --- a/tools/perf/builtin-annotate.c
> > > > > +++ b/tools/perf/builtin-annotate.c
> > > > > @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
> > > > >
> > > > > out_delete:
> > > > > /*
> > > > > - * Speed up the exit process, for large files this can
> > > > > - * take quite a while.
> > > > > - *
> > > > > - * XXX Enable this when using valgrind or if we ever
> > > > > - * librarize this command.
> > > > > - *
> > > > > - * Also experiment with obstacks to see how much speed
> > > > > - * up we'll get here.
> > > > > - *
> > > > > - * perf_session__delete(session);
> > > > > + * Speed up the exit process by only deleting for debug builds. For
> > > > > + * large files this can save time.
> > > > > */
> > > > > +#ifndef NDEBUG
> > > > > + perf_session__delete(annotate.session);
> > > > > +#endif
> > > >
> > > > So now, but default, we will call this, as we don't have this defined
> > > > only if DEBUG=1 is set, right?
> > > >
> > > > ?[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
> > > > tools/perf/util/mutex.c:#ifndef NDEBUG
> > > > ?[acme@toolbox perf-tools-next]$
> > >
> > > We can discuss this later, applied the series with just that zfree()
> > > change to annotation_options__exit().
> >
> > I don't think it's just an issue in the perf annotate. Maybe we can
> > do the same for perf report and so on.
>
> Yes, I thought at some point of setting some flag, perf_exiting, and
> then we would stop releasing memory, zfree comes to mind, but then we
> would still be traversing data structures, taking locks, etc.
>
> And we can't just exit() as we may need to flush caches, etc.
>
> IIRC this specific case appeared in profiles, so was commented out.
>
> > Anyway we could define NDEBUG=1 for release builds from now on.
>
> Yes, we should do it.

I'll send out a patch. Just a heads up that this will also disable
asserts for non-debug builds.

Thanks,
Ian

> - Arnaldo