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

Reduce the file size of flame graphs #314

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

godlygeek
Copy link
Contributor

These two changes combined result in a flame graph that's 85% smaller for the nbody example program in --trace-python-allocators and --native mode.

@godlygeek godlygeek self-assigned this Feb 6, 2023
@godlygeek godlygeek force-pushed the shrink_flamegraphs branch 2 times, most recently from dced1e8 to bb345cb Compare February 7, 2023 06:08
@godlygeek godlygeek changed the title Shrink flamegraphs Reduce file size for flame graphs Feb 7, 2023
@godlygeek godlygeek changed the title Reduce file size for flame graphs Reduce the file size of flame graphs Feb 7, 2023
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

LGTM

This is fantastic!

I left two nits

Rather than turning our entire deeply nested tree into JSON, encode the
data that it contains in a more compressed form. Replace all references
to strings with lookups in a table of strings. Pass all node attributes
as arrays, where the Nth element represents the value of that attribute
for the Nth node, and represent relationships to other nodes as indices
into this array. Reconstruct the tree in the flame graph JavaScript, to
scope the change to just the boundary between Python and JavaScript.

Additionally, consistently represent line numbers as integers rather
than strings. Previously we represented the root node's line number as
an integer and all other nodes' line numbers as strings.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Jinja uses the default `json.dumps` separators by default, which put an
unnecessary space after every `:` and `,` added in the JSON
representation for readability. Since we don't intend this data to be
human readable, ask Jinja to encode it more compactly.

Support for this functionality was added in Jinja2 2.9, so declare
a dependency on at least that version.

Also, stop depending on the `types-Jinja2` package for type checking, as
Jinja2 itself now has (more complete) type annotations.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
@godlygeek godlygeek enabled auto-merge (rebase) February 7, 2023 19:54
@godlygeek godlygeek merged commit aeb3bf5 into bloomberg:main Feb 7, 2023
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.

None yet

2 participants