Re: [PATCH v3 10/10] perf tools: Move TUI-specific fields out of map_symbol

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Jiri Olsa <jolsa@redhat.com>, LKML <linux-kernel@vger.kernel.org>,
	David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH v3 10/10] perf tools: Move TUI-specific fields out of map_symbol
Date: Tue, 5 May 2015 13:00:54 -0300	[thread overview]
Message-ID: <20150505160054.GX10475@kernel.org> (raw)
In-Reply-To: <1430837746-5439-1-git-send-email-namhyung@kernel.org>

Em Tue, May 05, 2015 at 11:55:46PM +0900, Namhyung Kim escreveu:
> The has_children and unfolded fields don't belong to struct map_symbol
> since they're used by TUI only.  Move those fields out of map_symbol
> since the struct is also used by other places.
> 
> This will also help to compact the sizeof struct hist_entry.

So this fixes that problem, but to test this patchkit a bit more before
commiting it, I tried the following:

1) perf record -a -g sleep 2

2) perf report

3) press E to expand all callchains

4) press P to save a perf.hist.0 "screenshot" of all the hist entries
   and its callchains, in their current state, i.e. expanded


Ok I did this with and without your patchset, and there are differences
in the result, that I don't know if are unintended fixes or how to
account them for, could you try reproducing these steps to see if you
notice this difference too?

Ok, now I understand, its a bug, when collecting callchains we need to
process them all, marking the DSOs that had hits _in the callchains_ and
save the build-id for those in the perf.data header, in addition to
saving the DSO in the buildid cache.

What happened was that the different entries I noticed where all in the
callchains, and the perf tool got rebuilt, and all the samples, in my
case, were in kernel functions, that were called by the perf binary....

So for something like:

--- perf.hist.0.WITHOUT 2015-05-05 12:49:06.557802910 -0300
+++ perf.hist.0.WITH_NAMHYUNG_PATCHSET  2015-05-05 12:48:13.871216253 -0300
@@ -167,26 +167,26 @@
 -    0.07%     0.00%  perf            perf                        [.] main
      main
      __libc_start_main
--    0.07%     0.00%  perf            perf                        [.] browser_command_config
-     browser_command_config
+-    0.07%     0.00%  perf            perf                        [.] run_builtin

And I collected the perf.data with your patchset applied, the correct is to
appear "run_builtin", not like here:

-    0.07%     0.00%  perf            perf                        [.] browser_command_config
     browser_command_config
     main

This is all because the perf binary used got lost, replaced by the new one, as it
wasn't saved in the perf.data file header/~/.debug/ cache:

[root@ssdandy linux]# perf buildid-list
cd1d2cf9f473d0cac668e3afee32866da4540bd4 /lib/modules/4.0.0-rc6+/build/vmlinux
2c07e0dc941be12d3db663ea617c08cf10dbbfa1 /lib/modules/4.0.0-rc6+/kernel/drivers/net/wireless/iwlwifi/iwlwifi.ko
[root@ssdandy linux]#

The fix probably will make the buildid collection at the end of a 'perf record'
session more expensive, will try to fix this...

But your patch is not responsible for this, so thanks for the fixes and
I'm applying it.

- Arnaldo
