search

KDAB / hotspot

The Linux perf GUI for performance analysis.
4.16k stars 257 forks source link

Disable disassembly of inline functions #568

Closed lievenhey closed 10 months ago

lievenhey commented 11 months ago

Currently this doesn't work correctly. In the flamegraph the top entry doesn't has the inline flag set. grafik

lievenhey commented 11 months ago

~The second commit is just for debugging. I will remove it once everything works.~ Done

lievenhey commented 11 months ago

I also added a color scheme to the flamegraph to show which functions are inlined. grafik

milianw commented 11 months ago

Cool, can you also add the combined mode, that shows system code / user code / inlined code? I think that is going to be very useful

milianw commented 11 months ago

OK, you can rebase, I fixed the issue with the duplicate inline frame now

lievenhey commented 11 months ago

I updated the brushed so that they always show if a frame got inlined grafik

lievenhey commented 11 months ago

I also had to remove the costBrush since:

lievenhey commented 11 months ago

~the ci not working is expected, since I updated it to kddw 2.0 this morning. It will work once #570 is landed~

GitMensch commented 11 months ago

@lievenhey This seems to still need an automated test of some sort, right? Apart from that this seems ready to merge and looks very useful.

milianw commented 10 months ago

Hey @lievenhey - I revamped the colorization of inline frames in the flamegraph, and include the info now also in tooltips. Can you give it another shot and see if that works for you too?

From my side, this is otherwise fine now.

GitMensch commented 10 months ago

This sounds good to me, but it looks like adjustment to README / doc pngs are "missing". Of course I'm fine with both handling those here as well as a follow-up PR.

GitMensch commented 10 months ago

From my side, this is otherwise fine now.

Note @milianw: at least from the PR state it still has "change requested" from you.

GitMensch commented 10 months ago

I've tinkered with the appimage, and found the flame-graph to be quite reasonable now (much better than the previous one). ... but: I'm not sure about the inline function costs and the disassembly.

grafik grafik

Of course, there is no option any more to use the "Disassembly" context on the inlined functions, which is fine, but I can't see neither the costs in the Disassembly of the caller nor the Disassembly for it either:

grafik

Shouldn't I see those 25.8% in line 418?

Note: when I use the caller view, I can "get into" the function, this shows: grafik

but currently I have to manually "overlay" the cpu cycles % over the source view and seem to have no option to reach the actual disassembly details (the disassembly is there, of course). grafik

milianw commented 10 months ago

@GitMensch please open a new issue for whatever you are seeing there, and reproduce the issue in a way we can replicate it ourselves. As is there's not enough information for me to give you an answer to your question.

GitMensch commented 10 months ago

Hm, that likely takes a while... short summary: the disassembly of inline functions has NO cost at all with this version, while it should have the cycles incl. set to the one that is visible in the Caller/Callee tab.

Note: also stumbled over #577 again (a relative new, but really bad bug).

milianw commented 10 months ago

How do you get disassembly of an inline function to begin with? that shouldn't be possible anymore after all?!

GitMensch commented 10 months ago

How do you get disassembly of an inline function to begin with? that shouldn't be possible anymore after all?!

as noted: the disassembly of one function included the disassembly for a good part of the source, including (one of the generations of) the inline function. Showing the disassembly or not here isn't the main point I have with the effect of this PR, it is that the inlined function has no cost shown in the source code window anymore (you only see that by the caller/callee tab)...

milianw commented 10 months ago

the inline symbol shouldn't even show up in the disassembly view at all, thus it not showing any cost there is to be expected, no? after all, we cannot open the disassembly for that anymore... I really don't follow what the issue is here, there seems to be a fundamental misunderstanding between us on how things should be working...

lievenhey commented 10 months ago

Hey @lievenhey - I revamped the colorization of inline frames in the flamegraph, and include the info now also in tooltips. Can you give it another shot and see if that works for you too?

From my side, this is otherwise fine now.

thanks, I suck at choosing color schemes.

GitMensch commented 10 months ago

the inline symbol shouldn't even show up in the disassembly view at all, thus it not showing any cost there is to be expected, no? after all, we cannot open the disassembly for that anymore... I really don't follow what the issue is here, there seems to be a fundamental misunderstanding between us on how things should be working...

I try to explain what I've thought to understand and what I think should be the UI for that.

Running objdump shows the disassembly for the "outer" function, therefore the inlined code is part of the disassembly, possibly multiple times (= we can "see" the specific generation for this specific place). Code-wise we see a function call.

If we can deduce the place of that disassembly to be part of the "calling" place (where it is inlined) then we can directly attribute the costs there.

If we can't do it, then we still have the "original" place (source reference and/or function name) and on the other hand we have the costs in the function of "calling" it (this is something that can be seen in the caller/callee tab, also with this PR applied). Ideally we'd then be able to attribute the "summed" costs for the disassembly to the calling place -but I do recognize that in this case we'd have to match that in the code, which is not possible to always get correct because of the preprocessor work (for example macros) - we shouldn't start that path.

This leaves the question: Is there a way to deduce the "calling" source reference for functions that are inlined? If we yes, then we should attribute the total costs to that place.

lievenhey commented 10 months ago

I created an issue for this #586