-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Profiling] Fix UI discrepancies in frame information window #142812
Conversation
sourceLine: number; | ||
}) { | ||
const executable = fileID === '' && addressOrLine === 0 ? 'root' : exeFileName; | ||
const functionSourceLine = functionOffset > 0 ? ` (+${sourceLine - functionOffset})` : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced that the relative line number is of any use here. E.g. +66
means 66 lines inside the function - to navigate there you have to manually calculate the absolute line number.
Also: since the aggregation ignores the line number, the value is an arbitrary value.
IMO better: show the function's line number after the source file name, e.g. "xyz.py#123"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about the relative line number. prodfiler
used the relative line number in the information window, yet the function line number in the flamegraph labels. Your last suggestion would make the flamegraph label and the information window agree, which is what I decided to go with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from my comment, LGTM
This matches with the value used in the flamegraph labels.
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
#142914) * Fix UI discrepancies in frame information window * Fix comment * Use source line instead This matches with the value used in the flamegraph labels. (cherry picked from commit 39811a3) Co-authored-by: Joseph Crail <[email protected]>
…#142812) * Fix UI discrepancies in frame information window * Fix comment * Use source line instead This matches with the value used in the flamegraph labels.
…#142812) * Fix UI discrepancies in frame information window * Fix comment * Use source line instead This matches with the value used in the flamegraph labels.
Summary
This PR addresses three small missing UI features in the frame information window:
frame type should be shown when the executable name is unavailable
function offset should be shown after the function when available