inferno icon indicating copy to clipboard operation
inferno copied to clipboard

Slow rendering of flamegraphs

Open itamarst opened this issue 1 year ago • 12 comments

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 avatar Jul 21 '22 20:07 itamarst

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

itamarst avatar Jul 21 '22 20:07 itamarst

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 avatar Jul 22 '22 13:07 itamarst

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 avatar Jul 22 '22 13:07 itamarst

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

itamarst avatar Jul 22 '22 14:07 itamarst

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?

itamarst avatar Jul 22 '22 15:07 itamarst

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

jonhoo avatar Jul 26 '22 12:07 jonhoo

Oh yeah computer science.

itamarst avatar Jul 26 '22 12:07 itamarst

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.

itamarst avatar Aug 12 '22 20:08 itamarst

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.

SergejIsbrecht avatar Aug 22 '22 13:08 SergejIsbrecht

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).

itamarst avatar Sep 20 '22 13:09 itamarst

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.

https://github.com/mrob95/inferno/commit/7fa333d5a698000a4631aebff08758fdbdd81d7c

mrob95 avatar Oct 09 '22 21:10 mrob95

@mrob95 You'll want to keep an eye on https://github.com/jonhoo/inferno/pull/262 — looks like that will bring some pretty major improvements!

jonhoo avatar Oct 15 '22 19:10 jonhoo