-
Notifications
You must be signed in to change notification settings - Fork 129
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
Speed up reflow when resizing flamegraphs with a monospace font #262
Conversation
Codecov ReportBase: 90.33% // Head: 90.33% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
=======================================
Coverage 90.33% 90.33%
=======================================
Files 19 19
Lines 4241 4241
=======================================
Hits 3831 3831
Misses 410 410
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
See attached files to get a sense of the impact, specifically you should resize the window and notice how responsive refreshes are. |
Added |
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.
This generally looks good to me, just had a couple of questions
Given this only addresses monospace fonts, perhaps the default font should be monospace for now? |
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.
Could you post a screenshot of a decently-busy flamegraph with and without monospace fonts? I'd like to see how much of a detriment it is to the appearance.
Also, would you mind updating the changelog?
Yeah, looks reasonable to me — let's update the default! |
… of faster drawing for case where monospace is used.
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.
Thanks! Will cut a release once I get a chance.
Released as 0.11.12 🎉 |
This addresses #256 partially; variable-width fonts aren't addressed.
By having two passes, one for reading attributes and one for writing attributes, we avoid refreshing the screen a bazillion times, as recommended by https://web.dev/avoid-large-complex-layouts-and-layout-thrashing/.
I tested this by rendering the input file attached in one of the comments on #256 and resizing the window, in both Chrome and Firefox, and with both variations of text truncation. On my very fast desktop, with a monospace font the resize is in real-time, and with variable-width font there is a noticeable pause before refresh happens.
Before merging I should probably at least: