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

Slow rendering of flamegraphs #256

Open
itamarst opened this issue Jul 21, 2022 · 12 comments
Open

Slow rendering of flamegraphs #256

itamarst opened this issue Jul 21, 2022 · 12 comments

Comments

@itamarst
Copy link
Contributor

A user complained about slow rendering of flamegraphs, and showed me a video; it was quite slow. Loading the same data into speedscope (https://www.speedscope.app/) was not slow, so the problem was with the way Inferno rendered it.

I tried doing a little performance profiling in Chrome; for context, I'm using 12th-gen i7 desktop, so it's probably got almost double the single-core speed of many (most?) computers.

  • Looking at rendering time for a complex SVG (35K lines in input), there's 90ms of time spent in JavaScript.
  • If I make update_text() a no-op, that goes down to 10ms.

This suggests that coming up with a faster alternative to update_text() would improve rendering time for everyone, and hopefully fix rendering problems for people whose computers are extra slow.

Chrome's profiler suggested as a hint that maybe the problem had something to do with https://web.dev/avoid-large-complex-layouts-and-layout-thrashing/

@itamarst
Copy link
Contributor Author

itamarst commented Jul 21, 2022

Reading the implementation it also seems not ideal, e.g. shrinking the string character by character until it's OK.

@itamarst
Copy link
Contributor Author

If it is layout thrashing, fastdom is proposed by the linked article and does seem like it would allow fixing things without a massive refactoring.

@itamarst
Copy link
Contributor Author

itamarst commented Jul 22, 2022

From an example (https://github.com/wilsonpage/fastdom/blob/master/examples/aspect-ratio.html):

    function reset(done) {
      n = input.value;
      divs = [];

      fastdom.measure(function() {
        var winWidth = window.innerWidth;

        fastdom.mutate(function() {
          container.innerHTML = '';

          for (var i = 0; i < n; i++) {
            var div = document.createElement('div');
            div.style.width = Math.round(Math.random() * winWidth) + 'px';
            container.appendChild(div);
            divs.push(div);
          }

          if (done) done();
        });
      });
    }

@itamarst
Copy link
Contributor Author

Coincidentally I just found an example input file that triggers visible slowness on my computer with only 2000 lines of input.
memory.zip

@itamarst
Copy link
Contributor Author

itamarst commented Jul 22, 2022

Increasing flamegraph::Options::min_width helps a lot for the example above. So my guess: long text + tall stacks + extremely thin columns means a whole lot of looping to shrink the text until it fits.

Perhaps then one improvement would be increasing text width until it is too big, instead of shrinking it until it fits? By their nature you'd expect many more thin columns than large columns, so that should reduce useless spinning. And/or maybe use parent column's text width as starting point, since child by its nature is going to be narrower?

@jonhoo
Copy link
Owner

jonhoo commented Jul 26, 2022

Alternatively it sounds like maybe we could do a binary search — that should speed it up pretty significantly.

@itamarst
Copy link
Contributor Author

Oh yeah computer science.

@itamarst
Copy link
Contributor Author

Another thought: divide frame width by font size to get approximate starting point, then do linear search up or down depending on how that fits.

@SergejIsbrecht
Copy link

Maybe switch from svg to d3 (https://d3js.org/) just like async-profiler did (https://github.com/jvm-profiling-tools/async-profiler)?

Following html FlameGraphs were created with async-profiler: https://gist.github.com/SergejIsbrecht/dda8abca7c3c1004f03f1507bc1e9240 . Just open with any browser.

@itamarst
Copy link
Contributor Author

Also occurs to me that for monospace fonts the value could be calculated directly, instead of empirically, so I might as a first pass do a fast path for that case (which would suffice for my use case).

@mrob95
Copy link
Contributor

mrob95 commented Oct 9, 2022

I've also noticed slow rendering with large graphs. Thanks for looking into it @itamarst.

I haven't done any detailed profiling of it but another potentially unnecessary thing that update_text is doing is running a regex every time to snip the sample count from the end of the title to get the function name. The function name (or just its length) could be stored somewhere to avoid this, e.g.

mrob95@7fa333d

@jonhoo
Copy link
Owner

jonhoo commented Oct 15, 2022

@mrob95 You'll want to keep an eye on #262 — looks like that will bring some pretty major improvements!

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

No branches or pull requests

4 participants