PMSI-AlignAlytics / dimple

An object-oriented API for business analytics
Other
2.73k stars 557 forks source link

Bar Chart Performance Bottleneck: dimple._parentWidth/Height #178

Open rpaskowitz opened 9 years ago

rpaskowitz commented 9 years ago

Looking into some possible performance bottlenecks, we've come across dimple._parentWidth. It's not so much that the function is itself slow, but rather that with bar charts, hundreds of style recalculations and and forced synchronous layouts seem to occur because of it.

From the following devtool timelines:

image

Hundreds of calls to parentWidth/Height are triggering the style recalcs and relayouts over a period of over 500ms resulting a slow render time and lots of blocking of the UI (with many charts rendering a once, the tab becomes unresponsive and Chrome warns about killing it).

We'll continue digging, but wanted to raise this here in case others have seen it, or have thoughts on how it might be solved.

rpaskowitz commented 9 years ago

A somewhat more complete timeline of a more basic graph: image

Here's the graph itself, which has 20 bars: image

rpaskowitz commented 9 years ago

The issue here seems to be the parent read of offsetHeight and offsetWidth, which are then used to position other elements. Since this happens in a loop, once one parent is read, an attribute change is made to another element, and then when the next parent is read, the DOM is dirty and so a relayout must occur.

See: Layout Thrashing at https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing

I'm starting to poke around to see if there's any decent central place to read the parent offsets ahead of time, along with potentially using fastdom (level of effort seems about equal so far). I managed to use fastdom in one area (incorrectly, but it did have an impact of cutting down the number of forced layouts)

rpaskowitz commented 9 years ago

This is turning out to be a bit more of a refactor than I'd hoped, but still probably less than it could have been. I've been focusing solely on bar charts to start, but have a pattern down should be replicable across all the relevant properties, including how they are used across all chart types.

Here are some early results that involve using fastdom to cache 'y' and 'height' from _helpers, and then retrieving from those caches in the bar draw(). There are other properties to cache (x, width, opacity, fill, ...), but bar seems to use y and height extensively, so they made a good place to start.

In the first chart below, you can see that the first bulk off draw() took ~70-80ms, and the second ~60ms. The second chart shows a much less deep call-stack with fewer forced relayouts, and the first big bulk consuming ~20-25ms, and the second ~30ms.

Caching the rest of the properties should cut the second part of draw() for bar down, where the deeper callstack can still be seen.

screen shot 2015-08-24 at 11 31 02 pm

screen shot 2015-08-24 at 11 29 20 pm

rpaskowitz commented 9 years ago

Full implementation of the caching can be viewed on my branch at https://github.com/rpaskowitz/dimple/tree/avoid-force-layout

Work remains to ensure that all places that use the _helpers ensure that they initialize the cache first. Still just bars for now.

A note about the previous graphs. The second half of the second graph which shows a deeper callstack is actually outside bar.draw() and is some code custom to our application. That section was cropped out of the first image. Recognizing in hindsight that the 2 halves are of different things, you can now notice that bar.draw() in the first pre-caching image is ~150ms, and in the second only ~20-25ms.

With that in mind, here is what it looks like with all properties cached:

screen shot 2015-08-25 at 12 40 31 am

screen shot 2015-08-25 at 12 39 25 am

In the first case, we can see bar.draw() measured at just under ~150ms, and in the second, it is ~4ms. It's cut off in the diagram, but there is now an exceptionally long stack of calls, the majority of them being the fastdom calls, which all stack up and run in parallel, providing for the significant time savings.

In the overall profile now, axis drawing takes quite some time, and the rendering appears to happen in 2 stages, with the axes showing up, a pause, and then the bars.

Within our application, there does seem to be some odd interaction when there are a lot of charts on one page. The axes all show up across the charts pretty much at once, but the bars are all staggered. They seem to take perhaps as long as they did before, although we gain from the UI not being blocked as it was before (with chrome warning of unresponsive tabs), but more investigation is needed.

Please have a look at the fork/branch to review the overall approach. We'll probably focus next on the odd multi-chart interaction, and if it can be solved, roll the changes out to the other places using _helpers.

rpaskowitz commented 9 years ago

Shaving off milliseconds is fun, but saving 10s of seconds is the main goal here. Here's one of our worst performing charts before the improvements:

screen shot 2015-08-25 at 9 03 00 am

And here, after. Two shots included as there is the work that was deferred through fastdom.

screen shot 2015-08-25 at 9 03 25 am screen shot 2015-08-25 at 9 05 38 am

Before: ~47s After: 1.5s + 0.8s = 2.3s

johnkiernander commented 9 years ago

Hi, I've been away and am just catching up with things. This looks really interesting. I'll give it a proper read and get back to you.

jpmunz commented 9 years ago

We were able to improve performance by adding a cache method and then calling it before draw: https://github.com/PMSI-AlignAlytics/dimple/compare/master...upsight:performance-fix?expand=1

Doesn't give as much of a performance boost as the fastdom stuff but it's orthogonal so can be done on top of it.