Messages in this thread | | | From | Namhyung Kim <> | Date | Wed, 7 Jun 2023 09:50:01 -0700 | Subject | Re: [PATCH 2/2] perf tools: allow running annotation browser from c2c-report |
| |
Hello,
Sorry for the late reply.
On Fri, Jun 2, 2023 at 1:18 AM Artem Savkov <asavkov@redhat.com> wrote: > > Hello, > > Thank you for the review. > > On Thu, Jun 01, 2023 at 02:26:48PM -0700, Namhyung Kim wrote: > > Hello, > > > > On Wed, May 31, 2023 at 4:50 AM Artem Savkov <asavkov@redhat.com> wrote: > > > > > > Add a shortcut to run annotation browser for selected symbol from > > > c2c report TUI. > > > > > > Signed-off-by: Artem Savkov <asavkov@redhat.com> > > > --- > > > tools/perf/builtin-c2c.c | 76 +++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 71 insertions(+), 5 deletions(-) > > > > > > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c > > > index 05dfd98af170b..96e66289c2576 100644 > > > --- a/tools/perf/builtin-c2c.c > > > +++ b/tools/perf/builtin-c2c.c > > > @@ -19,11 +19,13 @@ > > snip > > > > struct c2c_hists { > > > @@ -79,6 +83,7 @@ struct c2c_hist_entry { > > > * because of its callchain dynamic entry > > > */ > > > struct hist_entry he; > > > + struct evsel *evsel; > > > > I'm not sure if it's needed. It seems c2c command doesn't collect > > samples per evsel. It uses c2c.hists.hists for all evsels. Then it > > might not be worth keeping an evsel in a c2c_hist_entry and > > just use a random evsel in the evlist. > > > > Right, but annotation browser does use it for line usage percentage > calculation. So does this mean it won't be showing correct percentages > whatever evsel is chosen and that's why it is possible to just select a > random one?
Right, annotation can be correct but c2c hist entry is not tied to an evsel so there's no need to save the evsel for each hist entry.
> > As far as I can tell evlist is not currently available in > perf_c2c__browse_cacheline(), but it can be added to struct perf_c2c.
Sure, please do.
> > > > }; > > snip > > > > + " a Annotate current symbol\n" > > > " n Toggle Node details info \n" > > > " s Toggle full length of symbol and source line columns \n" > > > " q Return back to cacheline list \n"; > > > @@ -2650,6 +2670,44 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he) > > > key = hist_browser__run(browser, "? - help", true, 0); > > > > > > switch (key) { > > > + case 'a': > > > > I think it's better to factor this code out to a function. > > Ok, will do. > > > > + if (!browser->selection || > > > + !browser->selection->map || > > > + !browser->selection->map->dso || > > > + browser->selection->map->dso->annotate_warned) { > > > + continue; > > > + } > > > + > > > + ms.map = browser->selection->map; > > > + > > > + if (!browser->selection->sym) { > > > + if (!browser->he_selection) > > > + continue; > > > + > > > + ms.sym = symbol__new_unresolved(browser->he_selection->ip, > > > + browser->selection->map); > > > + > > > + if (!ms.sym) > > > + continue; > > > + } else { > > > + if (symbol__annotation(browser->selection->sym)->src == NULL) { > > > + ui_browser__warning(&browser->b, 0, > > > + "No samples for the \"%s\" symbol.\n\n", > > > + browser->selection->sym->name); > > > + continue; > > > + } > > > + ms.sym = browser->selection->sym; > > > + } > > > + > > > + err = map_symbol__tui_annotate(&ms, c2c_he->evsel, browser->hbt, &c2c.annotation_opts); > > > + > > > + ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries); > > > > c2c_browser__update_nr_entries() ? > > Will it change from the previous call before the while loop? This part > was mostly copied over from do_annotate() in ui/browsers/hists.c so I am > a bit hazy here. My understanding is that update_nr_entries and handle > resize are called here mostly for refresh/reset of the ui and > recalculation of number of entries is not needed
Hmm.. right. I thought you should check the number of entries again but c2c doesn't have filtering or hierarchy output so you can use hists->nr_entries always.
But now I wonder if it's really needed as the annotate will use its own browser so the c2c browser won't be affected.
> > > > > > + if ((err == 'q' || err == CTRL('c')) && browser->he_selection->branch_info) > > > > Why check branch_info? > > This was copied over as well and the comment in hists.c states "offer > option to annotate the other branch source or target (if they exists) > when returning from annotate". So I now think this might not be needed > here at all?
Yeah, I don't think we need it.
Thanks, Namhyung
> > > > + continue; > > > + if (err) > > > + ui_browser__handle_resize(&browser->b); > > > + > > > + continue; > > > > It'd be natural to use 'break' instead of 'continue' here. > > Yes, will change this. > > > > case 's': > > > c2c.symbol_full = !c2c.symbol_full; > > snip > > > > perf_session__delete(session); > > > out: > > > + annotation_options__init(&c2c.annotation_opts); > > > > __exit() ? > > Ouch, thanks for noticing! > > -- > Artem >
| |