Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable branches #518

Merged
merged 7 commits into from
Oct 21, 2023
Merged

Disable branches #518

merged 7 commits into from
Oct 21, 2023

Conversation

lievenhey
Copy link
Contributor

No description provided.

@lievenhey lievenhey linked an issue Oct 10, 2023 that may be closed by this pull request
Disassembly view: allow to disable branches / jumps #516
Closed
@GitMensch
Copy link
Contributor

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
Copy link
Contributor Author

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.

@lievenhey lievenhey force-pushed the disable-branches branch 4 times, most recently from 41bda57 to b2494e1 Compare October 11, 2023 13:12
@GitMensch
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

milianw
milianw requested changes Oct 11, 2023
View reviewed changes
src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved
src/resultsdisassemblypage.cpp Show resolved Hide resolved
src/resultsdisassemblypage.cpp Outdated Show resolved Hide resolved
@GitMensch
Copy link
Contributor

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 lievenhey force-pushed the disable-branches branch 2 times, most recently from 692e5b7 to 1e74256 Compare October 11, 2023 16:14
milianw
milianw requested changes Oct 12, 2023
View reviewed changes
src/models/disassemblyoutput.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the disable-branches branch 3 times, most recently from c2a4a84 to e7c1411 Compare October 12, 2023 12:39
GitMensch
GitMensch reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new logic of keeping the branches in a separate column, I think we could simplify the split.

Reasoning (please recheck if that's correct):

  • we have one big disassembly from objdump
  • at least currently the start of the assembly is always at the same line position (that would change if we have colored input because of terminal sequences)

In this case we can check the start of the assembly exactly one time on the first line of the assembly, then use that offset for each record.

That would drop the distance-call from each line, calculating it only once. preferably outside.

src/models/disassemblyoutput.cpp Show resolved Hide resolved
@lievenhey
Copy link
Contributor Author

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

@GitMensch
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

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.

@GitMensch GitMensch mentioned this pull request Oct 12, 2023
Draft
milianw
milianw approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this as an incremental step, but I dislike that toggling the visibility of the branch column directly is detached from the settings - can you please synchronize that to make it more obvious?

furthermore, I'm surprised to see that this patch has no test changes. That means this feature is currently completely untested? Can you please work on adding test coverage @lievenhey ?

If you prefer, you can merge this right away and work on both of the above in separate merge requests.

Thanks!

@lievenhey
Copy link
Contributor Author

I will add a test and then merge this

@lievenhey lievenhey force-pushed the disable-branches branch 3 times, most recently from a6dd6ba to 5eb7a57 Compare October 19, 2023 12:14
@lievenhey
rename source path settings to disassembly settings
3dfe071 
I originaly named these seetings source path settings since they only
influenced the source code search path so it didn't feel right to call
them disassembly settings. But since this patch series introduces an
option to turn of branches it now feels right to correct the name
@lievenhey
fix invalid memory acces in timelinedelegate
d50fe2b 
If only one event was recorded it points to end. In this case end->time
will be access which is undefined behavior
@lievenhey
fix disassemble called twice
39229ae 
Since the introduction of the stack in the disasembler there is no
longer a need to call showDisassembly since it will be called whenever
the stack changes.
@lievenhey
remove unused includes
ab06e32
milianw
milianw requested changes Oct 19, 2023
View reviewed changes
tests/modeltests/tst_disassemblyoutput.cpp Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the disable-branches branch 3 times, most recently from 99d0f1a to 825d48b Compare October 20, 2023 10:43
milianw
milianw requested changes Oct 21, 2023
View reviewed changes
Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix the small nits myself now

tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_disassemblyoutput.cpp Outdated Show resolved Hide resolved
lievenhey and others added 3 commits October 21, 2023 14:05
@lievenhey @milianw
add setting to disable branches in disassembler
c682645 
sometimes there are too many branches, this patch allows the user to
disable them

fixes: #516
@milianw
Centralize finding objdump binary
0adb8bf 
Skip whole test if it isn't found
@milianw
Don't use Q_ASSERT in tests
78f0642 
Prefer VERIFY_OR_THROW to ensure the checks are also done in
release builds without assertions enabled.
@milianw milianw merged commit deb59a5 into master Oct 21, 2023
23 checks passed
@milianw milianw deleted the disable-branches branch October 21, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GitMensch GitMensch GitMensch left review comments

@milianw milianw milianw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disassembly view: allow to disable branches / jumps
3 participants
@lievenhey @GitMensch @milianw