Messages in this thread | | | From | Milian Wolff <> | Subject | Re: [PATCH 0/7] generate full callchain cursor entries for inlined frames | Date | Wed, 24 May 2017 15:42:59 +0200 |
| |
On Wednesday, May 24, 2017 1:46:04 PM CEST Milian Wolff wrote: > On Monday, May 22, 2017 11:06:43 AM CEST Namhyung Kim wrote: > > Hi Milian, > > > > On Thu, May 18, 2017 at 10:05:36PM +0200, Milian Wolff wrote: > > > On Donnerstag, 18. Mai 2017 21:34:04 CEST Milian Wolff wrote: > > > > This series of patches completely reworks the way inline frames are > > > > handled. Instead of querying for the inline nodes on-demand in the > > > > individual tools, we now create proper callchain nodes for inlined > > > > frames. The advantages this approach brings are numerous: > > > > > > > > - less duplicated code in the individual browser > > > > - aggregated cost for inlined frames for the --children top-down list > > > > - various bug fixes that arose from querying for a srcline/symbol > > > > based > > > > on > > > > > > > > the IP of a sample, which will always point to the last inlined > > > > frame > > > > instead of the corresponding non-inlined frame > > > > > > > > - overall much better support for visualizing cost for heavily-inlined > > > > C++ > > > > > > > > code, which simply was confusing and unreliably before > > > > > > > > - srcline honors the global setting as to whether full paths or > > > > basenames > > > > > > > > should be shown > > > > > > > > For comparison, below lists the output before and after for `perf > > > > script` > > > > > > > and `perf report`. The example file I used to generate the perf data is: > > > And of course shortly after sending this patch series I notice the first > > > issues ;-) The new behavior shows confusing results for `-g function` > > > because match_chain uses sym->start. I fixed this locally to compare the > > > actual > > > > > function name if either of the two symbols is an inlined fake symbol: > > Why not making the fake symbol has start addr of the sample IP and > > length of 1. The histogram sort code also compares the sym->start > > which might confuse the output of the children mode too IMHO. > > I can try that out, thank you for the suggestion. But I think it can easily > break in different ways. I.e. when the same inline function gets used at > different IPs, it should actually be considered to be the same function when > we group/merge/aggregate. I updated the `match_chain` function accordingly, > to do a symname / srcline comparison on inlined frames, instead of relying > on the symbol start/end. I think using the IP for the fake symbols won't be > more reliable here, don't you think? > > In the end, I think we'll always have to special-case inlined fake symbols > when we aggregate data, since the sym start/end is always going to be some > arbitrary value that may or may not be what we want it to be. Doing the > explicit comparison on e.g. srcline/symname is always going to be the most > reliable option, as it also directly results in a proper aggregation based > on the strings that the user will see in the end.
I haven't yet tried it out, but I think I can come up with a way to break your approach easily. Assume the following pseudo-code:
void tail() { instr1; // IP1 instr2; // IP2 }
void mid() { tail(); }
void main() { mid(); }
Now, assume both `tail` and `mid` get inlined into `main`. If we get one sample each for both IP1 and IP2, we want the following merged structure if we merge based on symbol:
sym | incl | self main | 2 | 0 mid | 2 | 0 tail | 2 | 2
If we would give the inlined fake-symbols a start of the IP, i.e. either IP1 or IP2, then we would end up with this (unexpected) behavior instead:
sym | incl | self main | 2 | 0 mid | 1 | 0 mid | 1 | 0 tail | 1 | 1 tail | 1 | 1
The reason is that the fake symbols for the inlined frames would be considered to be different functions since their start/end are not equal. This is "wrong" in my eyes - we really have to do symbol name comparisons for inlined frames, and also include srcline if that is desired.
If you think the above is not a valid assessment, I'll try to change my patch series to use the IP + 1 trick you suggest. But I really don't think it's going to work.
Cheers -- Milian Wolff | milian.wolff@kdab.com | Software Engineer KDAB (Deutschland) GmbH&Co KG, a KDAB Group company Tel: +49-30-521325470 KDAB - The Qt Experts[unhandled content-type:application/pkcs7-signature] | |