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

Temporal inverted flame graph #452

Merged

Conversation

ivonastojanovic
Copy link

Issue number of the reported bug or feature request: #

Describe your changes
A clear and concise description of the changes you have made.

Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.

Additional context
Add any other context about your contribution here.

@@ -144,6 +144,7 @@ <h5 class="modal-title" id="helpModalLabel">How to interpret {{ kind }} reports<
const merge_threads = {{ merge_threads|tojson }};
const memory_records = {{ memory_records|tojson }};
const inverted = {{ inverted|tojson }};
const temporal = packed_data["high_water_mark_by_snapshot"] != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't fully correct - --temporal --leaks flamegraphs don't have high_water_mark_by_snapshot. Let's instead check:

Suggested change
const temporal = packed_data["high_water_mark_by_snapshot"] != null;
const temporal = packed_data.intervals != null;

That condition is true both with and without --leaks

const { strings, nodes, unique_threads } = packedData;

function generateNodeObjects(packedData) {
const { strings, nodes } = packedData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only need 2 things, let's just have the caller pass two parameters, instead of forcing them to pass an object that we can unpack here

Comment on lines 89 to 92
if (!node_objects) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this check is new - why do we need it?

@ivonastojanovic ivonastojanovic force-pushed the temporal_inverted_flame_graph branch from e7d924d to e65c1e9 Compare September 8, 2023 14:36
Allow `memray flamegraph` to support `--temporal` and `--inverted`
at the same time.

Signed-off-by: Ivona Stojanovic <[email protected]>
@godlygeek godlygeek force-pushed the temporal_inverted_flame_graph branch from e65c1e9 to 48dad0c Compare September 8, 2023 22:19
@godlygeek godlygeek marked this pull request as ready for review September 8, 2023 22:19
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

LGTM! I've squashed the fixup and slightly reworded the commit message, but otherwise I'm happy to land this.

@godlygeek godlygeek enabled auto-merge (rebase) September 8, 2023 22:20
@godlygeek godlygeek merged commit 514af3e into bloomberg:main Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants