jonhoo / inferno

A Rust port of FlameGraph
Other
1.68k stars 125 forks source link

Speed up reflow when resizing flamegraphs with a monospace font #262

Closed itamarst closed 2 years ago

itamarst commented 2 years ago

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:

codecov[bot] commented 2 years ago

Codecov Report

Base: 90.33% // Head: 90.33% // No change to project coverage :thumbsup:

Coverage data is based on head (604c7fe) compared to base (9676e33). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #262 +/- ## ======================================= Coverage 90.33% 90.33% ======================================= Files 19 19 Lines 4241 4241 ======================================= Hits 3831 3831 Misses 410 410 ``` | [Impacted Files](https://codecov.io/gh/jonhoo/inferno/pull/262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset) | Coverage Δ | | |---|---|---| | [src/flamegraph/mod.rs](https://codecov.io/gh/jonhoo/inferno/pull/262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL2ZsYW1lZ3JhcGgvbW9kLnJz) | `97.94% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

itamarst commented 2 years ago

See attached files to get a sense of the impact, specifically you should resize the window and notice how responsive refreshes are. result.zip

itamarst commented 2 years ago

Added zoom() support too, so now the refresh on startup is faster. result.zip

itamarst commented 2 years ago

Given this only addresses monospace fonts, perhaps the default font should be monospace for now?

itamarst commented 2 years ago

Screenshot from 2022-10-23 17-44-41

I will update changelog once you decide whether you want to change to monospace by default or not.

jonhoo commented 2 years ago

Yeah, looks reasonable to me — let's update the default!

jonhoo commented 2 years ago

Released as 0.11.12 :tada: