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.

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

Fix cost attribution of inline frames in disassembly view #672

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

milianw
Copy link
Member

@milianw milianw commented Sep 2, 2024

Previously, we put the cost of an inline frame into a container indexed by the inline symbol. But during disassembly, we never get to query that data again, since we cannot disassemble an inline frame. Instead, we need to be able to query the cost for arbitrary binary offsets, independent of their originating symbol.

The patch here achieves this by lifting the OffsetLocationCostMap out of the CallerCalleeEntryMap into CallerCalleeResults, but mapped by the binary name. This way we can efficiently store and lookup the data of a given offset within a specific binary during disassembly, which allows us to show the cost for inlined code in the disassembly view.

Fixes: #671

Previously, we put the cost of an inline frame into a container
indexed by the inline symbol. But during disassembly, we never get
to query that data again, since we cannot disassemble an inline frame.
Instead, we need to be able to query the cost for arbitrary binary
offsets, independent of their originating symbol.

The patch here achieves this by lifting the OffsetLocationCostMap out
of the CallerCalleeEntryMap into CallerCalleeResults, but mapped by
the binary name. This way we can efficiently store and lookup the data
of a given offset within a specific binary during disassembly, which
allows us to show the cost for inlined code in the disassembly view.

Fixes: #671
@milianw
Copy link
Member Author

milianw commented Sep 2, 2024

@arvidfm @lievenhey @GitMensch care to try out the appimage in here for inline cost attribution in the disassembly addr view? I think that should be fixed now

@arvidfm
Copy link

arvidfm commented Sep 2, 2024

@milianw Is this the right artifact to test? I can't seem to find any artifacts for the latest commit (c3e1682), only the pre-force push one (8e20e25).

@GitMensch
Copy link
Contributor

@arvidfm you find the right artifact here: https://github.com/KDAB/hotspot/actions/runs/10662946666/artifacts/1881066271 (go to "build appimage, then to its summary, then click on "skipped by" until you get to actual binaries)

@arvidfm
Copy link

arvidfm commented Sep 3, 2024

Giving it a try on one of my projects, the inlined instructions seem to be getting annotated properly now, but the corresponding source lines on the left are still missing the costs, is that expected?
image

@GitMensch
Copy link
Contributor

GitMensch commented Sep 4, 2024

but the corresponding source lines on the left are still missing the costs

I think this is what #586 is about.
Interesting in this area would be #523 as well - but that's an enhancement ;-)

@milianw
Copy link
Member Author

milianw commented Sep 5, 2024

ok cool, I'll merge this as-is for now and then look at the source file/line problem next when I find the time. the solution to that is probably going to look similar, but we need to make sure that we don't show confusing numbers when the same function is inlined into multiple ones

@milianw milianw merged commit 36bedef into master Sep 5, 2024
25 checks passed
@milianw milianw deleted the work/inline-assembly-cost branch September 5, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disassembly: per-instruction cost is not shown for inlined code
3 participants