search

KDAB / hotspot

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

Disable branches #518

Closed lievenhey closed 1 year ago

GitMensch commented 1 year ago

LGTM in general.

What do you think about the idea to have the jumps in a separate "column"? In this case the column would be auto-hidden/disabled if CanVisualize() == false (or the visualization was turned of in the settings) and otherwise the user would be able to show/hide that column at any time.

If you don't want to follow that route, then I think it would be useful to (additionally?) have a toggle to be able to switch this directly on the disassembly page to re-read with visualization and update.

lievenhey commented 1 year ago

What do you think about the idea to have the jumps in a separate "column"?

The more I think about it, the better it sounds. In the current version I completely reload the model which will reset the view if I would add a checkbox to toggle this feature. I originally didn't want to go that route since it involves a lot of dark magic (regexes) but it seems to be the better solution in the long run.

GitMensch commented 1 year ago

The current resulting AppImage is much faster, instead of 24 seconds it only needs 8-9 seconds for opening. If I globally disable the jumps in the settings, then it gets down to 5-6 seconds.

Disabling the branch column "on demand" works fine. Note: the visible columns are not saved, if you disable it, then go back to the callee view, then use Disassemble on the same function the column is enabled again; same if the setting is of and the column is manually enabled. I think the setting should be primarily about "is --visualize-jumps to be used or not" (with the nice check and note like in #520), not about the visibility of that column.

GitMensch commented 1 year ago

Just to recheck as it wasn't obvious from inspecting only the changes: is the new branches column "monospace text" only, or is it "syntax-highlighted", too? I think it may also work with non-monospace (then looking better and saving space - but I'm not sure...)

lievenhey commented 1 year ago

The current resulting AppImage is much faster, instead of 24 seconds it only needs 8-9 seconds for opening. If I globally disable the jumps in the settings, then it gets down to 5-6 seconds.

Interesting seem like qt has a problem with layouting if there are a lots of whitespaces

Disabling the branch column "on demand" works fine. Note: the visible columns are not saved, if you disable it, then go back to the callee view, then use Disassemble on the same function the column is enabled again; same if the setting is of and the column is manually enabled.

I see this as an on demand feature for quick stuff. It is only saved, if you change it in the settings.

Just to recheck as it wasn't obvious from inspecting only the changes: is the new branches column "monospace text" only, or is it "syntax-highlighted", too? I think it may also work with non-monospace (then looking better and saving space - but I'm not sure...)

it is monospace only

GitMensch commented 1 year ago

Just to recheck as it wasn't obvious from inspecting only the changes: is the new branches column "monospace text" only, or is it "syntax-highlighted", too? I think it may also work with non-monospace (then looking better and saving space - but I'm not sure...)

it is monospace only

Can you give it a short try to see how it looks with the default font? If that looks better (or at least not worse and saves space), then this would be possibly a reasonable switch.

lievenhey commented 1 year ago

The current resulting AppImage is much faster, instead of 24 seconds it only needs 8-9 seconds for opening. If I globally disable the jumps in the settings, then it gets down to 5-6 seconds.

That is way too long. It should only take a few milliseconds. I will ask some KDE People about this, since they wrote their own Layout Engine which they use in Kate. Kate can layout 2 million lines in like 10ms.

lievenhey commented 1 year ago

Can you give it a short try to see how it looks with the default font? If that looks better (or at least not worse and saves space), then this would be possibly a reasonable switch.

looks terrible since a space has a different width than a dash grafik

GitMensch commented 1 year ago

The current resulting AppImage is much faster, instead of 24 seconds it only needs 8-9 seconds for opening. If I globally disable the jumps in the settings, then it gets down to 5-6 seconds.

That is way too long. It should only take a few milliseconds. I will ask some KDE People about this, since they wrote their own Layout Engine which they use in Kate. Kate can layout 2 million lines in like 10ms.

Thank you. Note that these functions are quite huge, so I wouldn't blame it all to the syntax-highlighting / layouting... but then the layouting seems to be an issue, let's go on with this at #505.

lievenhey commented 1 year ago

I will have a look at coloured output soonish, so I would keep it as is. Especially since it is a cheap calculation.

GitMensch commented 1 year ago

This is now much better than before, I hope this gets pulled in and afterwards we may get a finished #494 (looks like 98% done) pulled in, too.

Thank you for the work on this!

GitMensch commented 1 year ago

I will have a look at coloured output soonish, so I would keep it as is. Especially since it is a cheap calculation.

Note that escape sequences would again match the hex-check :-) But I totally agree that this can be revisited later.

lievenhey commented 1 year ago

This is now much better than before, I hope this gets pulled in and afterwards we may get a finished https://github.com/KDAB/hotspot/pull/494 (looks like 98% done) pulled in, too.

This will still take some time. We want to move the logic to the perfparser since it has all the logic for finding binaries etc.

lievenhey commented 1 year ago

I will add a test and then merge this