Misc perf improvements #1772

alexcjohnson · 2017-06-09T06:27:30Z

A bunch of perf improvements. The biggest one is changing how we cache Drawing.bBox results ea883e6 - instead of having an element keep track of its own caching, I changed it to just using the contents of the node (its innerHTML plus its style and text-anchor - I'm 90% sure this is sufficient to determine the bbox but needs a bit more testing). This means we're not always invalidating the cache, whether accidentally or out of an abundance of caution, and in fact even unrelated elements that have the same contents and styling can use the same bbox (such as matching labels on different axes).

Another fairly significant one is removing an unnecessary double-draw of ticks and grids during Plotly.plot b4a2f4d - no idea where that came from but it doesn't seem to cause any problems to pare that down.

Two smaller ones:

don't draw placeholder titles that we're just going to delete c02204f
remove Lib.getPlotDiv 032cf7c - this was by far the most intrusive change (I had to pass gd around to a bunch of places that didn't have it before), for the smallest benefit, but it turns out instanceof HTMLElement (which is actually in Lib.isPlotDiv which I didn't get rid of... but that one calls it once and getPlotDiv called it in a loop) could be kind of slow.

Also I added a perf tester timeit to the global scope in the test dashboard, just for convenience.

I'd like to get #1767 merged before finishing this so I can bring the test improvements in here.

means we get a lot more caching, and a lot less DOM manipulation and getBoundingClientRect

etpinard · 2017-06-09T13:36:37Z

src/lib/index.js

@@ -481,13 +481,6 @@ lib.containsAny = function(s, fragments) {
    return false;
 };

-// get the parent Plotly plot of any element. Whoo jquery-free tree climbing!
-lib.getPlotDiv = function(el) {


Beautiful commit.

Note that Lib.isPlotDiv is currently used in W2. No such problem for Lib.getPlotDiv.

etpinard · 2017-06-09T13:37:01Z

src/components/titles/index.js

@@ -83,7 +89,7 @@ Titles.draw = function(gd, titleClass, options) {
    }

    var el = group.selectAll('text')
-        .data([0]);
+        .data(elShouldExist ? [0] : []);


etpinard · 2017-06-09T13:39:16Z

devtools/test_dashboard/devtools.js

@@ -8,6 +8,8 @@ var credentials = require('../../build/credentials.json');
 var Lib = require('@src/lib');
 var d3 = Plotly.d3;

+require('./perf');


cc @rreusser would be nice to have something like that in https://github.com/rreusser/plotly-mock-viewer

Moreover, @dfcreative's brilliant ✨ https://www.npmjs.com/package/fps-indicator ✨ would be a nice addition to our dev tools.

etpinard · 2017-06-09T13:45:22Z

src/components/drawing/index.js

+//   }
+//   return h;
+// }
+function nodeHash(node) {


Interesting patch. Do you have any benchmark results comparing old and new Drawing.bBox?

If you're concerned about memory usage, you might want to consider setting maxSavedBBoxes to something less than 1e4. 10000 text nodes on one page sounds a bit much.

cc'ing @monfera @rreusser who might have experience doing stuff like this.

Do you have any benchmark results comparing old and new Drawing.bBox?

I don't have any explicit benchmarks, but putting in a few console.log statements you can see that previously during redraws (and especially interactions like dragging axis ends) we were only actually using the cache in a few cases, mostly we were redoing the cloneNode and getBoundingClientRect. Now we hardly ever need to do that. The key is that managing data-bb so we can safely use the cache is really hard and we weren't doing it well. This way there's nothing to manage, and nodeHash is very fast (as long as I don't actually hash the resulting string 🤔 ).

10000 text nodes on one page sounds a bit much.

Yes, perhaps I could drop the limit a bit, but two things to note:

I'm not storing text nodes, just a lookup object of {nodeHash: bBox} where bBox is a plain object, my main concern is that nodeHash itself can be quite a long string, though I guess a bBox object of 6 numbers can also take non-negligible memory.

A large number of text nodes is exactly when this would be most useful, and wiping the cache will kill this whole gain. Perhaps I could take advantage of Object.keys being ordered in insertion order (usually? always?) and drop the oldest 50% of cache entries or something if we hit the limit?

monfera · 2017-06-09T14:15:56Z

Looks great!

I've often seen double or worse rendering in imperative code, hard to automatically catch, I guess this double render was not intended
bounding box calc is in good part expensive because the layouting must be synchronously awaited, so I'd guess avoiding bbox calc on identical tick text is even bigger than avoiding repeat calls
caching is best if global, but risk is mem leak

alexcjohnson · 2017-06-10T23:04:39Z

test/jasmine/tests/cartesian_interact_test.js

+        return function() {
+            gd._mouseDownTime = 0; // ensure independence from any previous clicks
+            return doubleClick(getDragger(subplot, directions));
+        };


Man, this one was brutal - the key was that (locally, in chrome, when running multiple tests) it was seeing a previous click (from the same test, despite the fact that it still cared whether another test had been run before this one) too close to the first click of the doubleclick, and ended up responding as if there were two doubleclicks - which I still don't quite understand, I didn't think a triple-click would look like two doubles, but anyway this works and we can use this in the future (similar to your trick with gd._lastHoverTime @etpinard ) to cut down on unnecessary delays in the test suite.

For future reference @etpinard since we talked about potentially doing this, I went off on a dead end for a while when I noticed that calling Plotly.purge(gd) during destroyGraphDiv caused all sorts of problems... That turns out to be unrelated, that sometimes there are some things still waiting to happen on the plot after tests have finished (like rehover) which will fail (and confusingly attribute the failure to the next test that's run, since it's happening async) if we've purged the plot of attributes like emit but they still complete without failure if all we've done is remove the gd from the DOM.

Thanks for writing down the details.

🎉 nice work

The only reason we needed two initInteractions during Plotly.plot is so the second one could pick up the `onmousemove` from the first to cache as `initialMouseMove`, but it works fine now to remove that junk entirely.

alexcjohnson · 2017-06-11T15:55:21Z

Some profiling on small charts: All results from Chrome on my mac with no profiles running. Running profiles adds ~5% to these times with no screenshots, ~10% with screenshots (interestingly the profiling penalty seems bigger even fractionally before the change). All results are averaging ~10 runs of Plotly.newPlot on the same data repeatedly, throwing away the first one. This gives extra weight to the caching improvements, but as my interest is mainly in interactivity performance that's what I want.

using mock category_dtick_3 (small line chart with a linear and a category axis):

before this branch: 144ms
after this branch: 63ms

using a chart with ~30 variable-size bubbles, gradient fill, long category names:

before this branch: 444ms
after this branch: 270ms

alexcjohnson · 2017-06-11T16:07:08Z

(offline weekend 💃 from @etpinard )

etpinard · 2017-06-12T14:02:49Z

src/components/shapes/draw.js

@@ -141,6 +140,8 @@ function setupDragElement(gd, shapePath, shapeOptions, index) {

    dragElement.init(dragOptions);

+    shapePath.node().onmousemove = updateDragMode;


Referencing the ticket for annotation edits -> #679

etpinard · 2017-06-12T14:03:09Z

src/components/dragelement/index.js

@@ -57,15 +62,11 @@ dragElement.init = function init(options) {
        startY,
        newMouseDownTime,
        dragCover,
-        initialTarget,
-        initialOnMouseMove;


Amazing. Thanks for 🔪 that!

alexcjohnson added 5 commits June 9, 2017 02:09

get rid of Lib.getPlotDiv

032cf7c

short-circuit title draw if there's no title

c02204f

drop the unneeded initial draw of axes

b4a2f4d

add perf tester to global scope in test dashboard

b9c33c9

cache bounding boxes by node contents instead of by data-bb attr

ea883e6

means we get a lot more caching, and a lot less DOM manipulation and getBoundingClientRect

etpinard added status: in progress labels Jun 9, 2017

etpinard reviewed Jun 9, 2017

View reviewed changes

alexcjohnson added 8 commits June 9, 2017 12:08

Merge branch 'master' into misc-perf

11f4a0e

clear bbox cache to fix bar test

a511bd2

standardize fail logic in bar test

434dc0e

more failure info in cartesian_interact_test

d077e32

cleanup cartesian_interact_test a little

3132624

remove unused dragElement.init option 'doubleclick'

00c7ad5

cleaner delay pattern in pan tester

2da3544

robustify cartesian_interact_test doubleclicker

dc4fc32

alexcjohnson commented Jun 10, 2017

View reviewed changes

alexcjohnson merged commit 85e8770 into master Jun 11, 2017

alexcjohnson deleted the misc-perf branch June 11, 2017 16:07

alexcjohnson mentioned this pull request Jun 11, 2017

[WIP] some more performance optimizations #1776

Closed

etpinard reviewed Jun 12, 2017

View reviewed changes

etpinard added the status: reviewable label Jun 12, 2017

etpinard removed the status: in progress label Jun 12, 2017

etpinard added this to the 1.28.0 milestone Jun 12, 2017

This was referenced Jun 13, 2017

annotations in IE: bounding boxes and arrowheads broken #1782

Closed

No innerHTML and pseudo-html cleanup #1792

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc perf improvements #1772

Misc perf improvements #1772

alexcjohnson commented Jun 9, 2017

etpinard Jun 9, 2017

etpinard Jun 9, 2017

etpinard Jun 9, 2017

etpinard Jun 9, 2017

alexcjohnson Jun 9, 2017

monfera commented Jun 9, 2017 •

edited

Loading

alexcjohnson Jun 10, 2017

etpinard Jun 12, 2017

etpinard Jun 12, 2017

alexcjohnson commented Jun 11, 2017

alexcjohnson commented Jun 11, 2017

etpinard Jun 12, 2017

etpinard Jun 12, 2017

		@@ -141,6 +140,8 @@ function setupDragElement(gd, shapePath, shapeOptions, index) {

		dragElement.init(dragOptions);

		shapePath.node().onmousemove = updateDragMode;

Misc perf improvements #1772

Misc perf improvements #1772

Conversation

alexcjohnson commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfera commented Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Jun 11, 2017

alexcjohnson commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfera commented Jun 9, 2017 •

edited

Loading