Messages in this thread Patch in this message | | | Date | Wed, 30 Mar 2016 21:02:45 +0200 | From | Andres Freund <> | Subject | Re: "perf hists browser: Support flat callchains" appears to have broken parent reporting |
| |
Hi!
On 2016-03-31 01:00:10 +0900, Namhyung Kim wrote: > On Wed, Mar 30, 2016 at 04:19:26PM +0200, Andres Freund wrote: > > On 2016-03-30 10:46:34 -0300, Arnaldo Carvalho de Melo wrote: > > > Em Wed, Mar 30, 2016 at 02:34:18PM +0200, Andres Freund escreveu: > > > > Hi, > > > > > > > > 4b3a3212233a - "perf hists browser: Support flat callchains" seems to > > > > have broken callchain display in tui mode when using !flat mode, or at > > > > least changed it in an unintended manner. > > > > > > humm, at first I thought this would be related to --percent-limit... > > > > I'm not using --percent-limit. Just to be sure, I did explicitly set it > > to various values, and it looks unrelated. > > > > > What tree/branch are you using? Can you try pressing 'L' to play with > > > the percent limit? > > > > I'm primarily using linus' tree, and bisected the behavioural down to > > that individual commit. > > Thanks for reporting and finding this!
No problem. I'm somewhat surprised to be the first to report this, the behavioural change confused me quite a bit. Maybe it took others about as long as me to figure out it's actually a perf report problem ;)
> > I don't know the perf code at all, but skimming through the commit, the > > following hunk looks suspicious: > > > > @@ -263,7 +295,7 @@ static void callchain_node__init_have_children(struct callchain_node *node, > > chain = list_entry(node->val.next, struct callchain_list, list); > > chain->has_children = has_sibling; > > > > - if (!list_empty(&node->val)) { > > + if (node->val.next != node->val.prev) { > > chain = list_entry(node->val.prev, struct callchain_list, list); > > chain->has_children = !RB_EMPTY_ROOT(&node->rb_root); > > } > > > > Reverting that individual change fixes things. I'm not actually sure > > what the post 4b3a3212233a version actually tests for? > > Yeah, this is it. It's my fault that I thought if the first chain > (node->val.next) was set by has_sibling, no need to go to the body > of the "if" statement when next == prev case. But it's not...
> Do you mind resending the fix as a formal patch with my ack ?
Done.
I couldn't really explain the problem, so I handwaved the actual problem away: I've not really grasped how the rbtree, callchain_lists and callchain_node are intertwined.
My symptom description probably is inaccurate as well.
From 017d80e3c98c7237183ac20bb1cfc81bd5e9a474 Mon Sep 17 00:00:00 2001 From: Andres Freund <andres@anarazel.de> Date: Wed, 30 Mar 2016 19:39:28 +0200 Subject: [PATCH] perf hists: Fix determination of a callchain node's childlessness.
4b3a3212233a ("perf hists browser: Support flat callchains") over-aggressively tried to optimize callchain_node__init_have_children().
That lead to --tui mode not allowing to expand call chain elements if a call chain element had only one parent. That's why --inverted callgraphs looked halfway sane, but plain ones didn't.
Revert that individual optimization, it wasn't really related to the rest of the commit.
Fixes: 4b3a3212233a042f48b7b8fedc64933e1ccd8643
Acked-by: Namhyung Kim <namhyung@kernel.org> Signed-off-by: Andres Freund <andres@anarazel.de> --- tools/perf/ui/browsers/hists.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index 4b98165..2a83414 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -337,7 +337,7 @@ static void callchain_node__init_have_children(struct callchain_node *node, chain = list_entry(node->val.next, struct callchain_list, list); chain->has_children = has_sibling; - if (node->val.next != node->val.prev) { + if (!list_empty(&node->val)) { chain = list_entry(node->val.prev, struct callchain_list, list); chain->has_children = !RB_EMPTY_ROOT(&node->rb_root); } -- 2.7.0.229.g701fa7f.dirty
| |