Splom zoom perf #2527

etpinard · 2018-04-05T20:25:14Z

to be merged into #2505

This PR features performance improvements on zoom interactions geared towards splom traces, but will benefit all cartesian subplots especially on multi-subplot graphs.

I opened a separate PR because I'm hoping to improve a few more things before the splom PR is ready to be merged (cc @dy )

Things improved so far

relayout(gd, 'xaxis.range', []) is now using a special (much) faster relayout pathway
relayout(gd, 'dragmode', '') now only has to update one DOM node per graph!
subplot updates during panning are now more granular (and hence faster for splom-only, scattergl-only cases and graphs w/o points that need to be scaled on pan)

Things I'll like to improve in the coming days

double-click (i.e auto/reset range) performance. This thing currently goes through editType: 'calc' which is slow as a 🐢. I'll try to cache the autorange value somewhere (and clearing it when new data comes in) and use the axrange pathway to speed thing up. This will benefit big data gl and svg graphs too!
speed up defaults (Plots.supplyDefaults takes > 100ms at 50x50 subplots), Getting rid of indexOf into axis lists would be a good start.
cache a few (sometimes) large selections during the plot step to 🔪 selectAll on pan
speed up axrange pathway even more by targeting minimal set of axes to update (via Axes.doTicks) instead of redraw ticks and labels for all axes.

cc @alexcjohnson

... (i.e. out of plot_api.js), and move Plots.addLinks out of drawData into its own step in Plotly.plot.

- this represents the minimal sequence for '(x|y)axis.range' relayout calls which are pretty common (e.g. on zoom/pan mouseup). - by bypassing drawFramework, lsInner and initInteraction, this can save ~1000ms on 50x50 subplot grids.

- split minimal updateFx part out of initInteractions - set maindrag cursor class (which depends only on layout.dragmode) on <g .draglayer> instead of inner <rect> to update it for all subplots in < 1ms (that's a > 600ms improvement on 50x50 grids) - use gd._fullLayout instead of scoped fullLayout in initInteractions and makeDragBox to ensure correct reference after doModeBar()

- allow fullLayout._has(/*trace type*/) to work - use registry category hash object (instead of categories.indexOf) to find categories in fullLayout._modules

- replace indexOf with hash objects lookups - add 'svg' and 'draggedPts' trace module categories - speed up updateSubplots (called on pan and scroll) by splitting it into splom, scattergl, svg and draggedPts blocks, (draggedPts is very slow, svg can be slow at 50x50) - ... some scope variable clean up

etpinard · 2018-04-05T20:32:21Z

src/plots/plots.js

-            return true;
-        }
+        var name = modules[i].name;
+        if(name === category) return true;


so calls like fullLayout._has('scattergl') just works w/o having to set a scattergl category in the trace module.

alexcjohnson · 2018-04-05T20:43:24Z

src/plots/cartesian/graph_interact.js

+// should trigger a full initInteractions.
+exports.updateFx = function(fullLayout) {
+    var cursor = fullLayout.dragmode === 'pan' ? 'move' : 'crosshair';
+    setCursor(fullLayout._draggers, cursor);


Ah, great solution - much simpler than what I was thinking, and at least as fast 🐎

etpinard · 2018-04-05T20:44:20Z

src/traces/scatter/index.js

@@ -34,7 +34,7 @@ Scatter.animatable = true;
 Scatter.moduleType = 'trace';
 Scatter.name = 'scatter';
 Scatter.basePlotModule = require('../../plots/cartesian');
-Scatter.categories = ['cartesian', 'symbols', 'markerColorscale', 'errorBarsOK', 'showLegend', 'scatter-like'];
+Scatter.categories = ['cartesian', 'svg', 'symbols', 'markerColorscale', 'errorBarsOK', 'showLegend', 'scatter-like', 'draggedPts'];


IMPORTANT (and possibly worth refactoring)

scattergl and splom traces are currently declared with a 'cartesian' category so that cartesian axis logic that they share just works. But this means that not all 'cartesian' traces are svg or more precisely not all 'cartesian' traces are plotted in <g .cartersianlayer>. So to avoid looping over all subplots on drag to shift plotinfo.plot and its clip <rect> for scattergl and splom traces, I made an 'svg' category. This is a slight misnomer though, as heatmaps and histogram2d are here considered svg even though they are built using a <canvas>. Oh well. The 'svg' category mirrors the 'gl' and 'regl' category listed in scattergl and splom trace modules. Any objections?

Moreover, I added a 'draggedPts' category to avoid those costly selectAll('.point') on pan when we don't need them. For example, graphs with contour lines feel faster on pan as we no longer traverse the <g .cartesianlayer> looking for points to scaled.

I think this is fine - 'cartesian' means "data relates to cartesian axes" and 'svg' means "data is drawn into an SVG container" (heatmaps use a canvas to generate the image but then put it into an svg <image> element)

'draggedPts' is still potentially overkill, as the trace type doesn't necessarily tell you if the traces contain these elements - all of these trace types can be drawn with no points or text, only lines and fills...

etpinard · 2018-04-05T20:47:00Z

src/plots/cartesian/axes.js

@@ -2188,7 +2188,7 @@ axes.doTicks = function(gd, axid, skipTitle) {
            }
            drawTicks(mainPlotinfo[axLetter + 'axislayer'], tickpath);

-            tickSubplots = Object.keys(ax._linepositions);
+            tickSubplots = Object.keys(ax._linepositions || {});


Unfortunate, but important as relinkPrivateKeys does relink empty objects.

... does not relink

does not relink empty objects.

alexcjohnson · 2018-04-05T20:52:22Z

src/plots/plots.js

+        var name = modules[i].name;
+        if(name === category) return true;
+        // N.B. this is modules[i] along with 'categories' as a hash object
+        var _module = Registry.modules[name];


TBH I've forgotten why we wrapped modules like this in the registry... not a big deal, but would it be better to attach the hash directly to the module, or even make categories as a hash from the beginning? Is it ever important that it's a list?

Referencing #2174 where we like to make building custom trace modules more user friendly:

we should try to not mutate the input module object, so that means not replacing the categories list with a hash object nor augmenting the input module object with e.g. a categoryHash key.

listing categories as an array feels more natural than {cat1: 1, cat2: 1} hash object

So, I'm voting for the status quo.

alexcjohnson · 2018-04-05T21:36:45Z

src/plot_api/plot_api.js

+                // TODO
+                // no test fail when commenting out doAutoRangeAndConstraints,
+                // but I think we do need this (maybe just the enforce part?)
+                // Am I right?


Huh, I'd have thought so... perhaps worth spending a little time looking for something that does fail if this is removed, so it can be 🔒 down.

@alexcjohnson Tests are currently passing even when subroutines.doAutoRangeAndConstraints is commented out, because of this block:

plotly.js/src/plot_api/plot_api.js

Lines 2088 to 2107 in 2a25820

// figure out if we need to recalculate axis constraints

var constraints = fullLayout._axisConstraintGroups || [];

for(axId in rangesAltered) {

for(i = 0; i < constraints.length; i++) {

var group = constraints[i];

if(group[axId]) {

// Always recalc if we're changing constrained ranges.

// Otherwise it's possible to violate the constraints by

// specifying arbitrary ranges for all axes in the group.

// this way some ranges may expand beyond what's specified,

// as they do at first draw, to satisfy the constraints.

flags.calc = true;

for(var groupAxId in group) {

if(!rangesAltered[groupAxId]) {

Axes.getFromId(gd, groupAxId)._constraintShrinkable = true;

}

}

}

}

}

which makes axis range relayout calls altering constrained axes go though the calc: true edit pathway which (of course) calls doAutoRangeAndConstraints itself.

Interestingly, commenting out that block which sets flags.calc = true does not make any test fails when subroutines.doAutoRangeAndConstraints is called during axrange edits. Perhaps we don't need?

It doesn't look to me as though I wrote all that great test coverage for this section... the two key points of it are:

You can relayout a constrained axis, particularly to a smaller range, and have the other axes in the group update (expanding or shrinking) to continue to meet the constraints (note that this won't work with react, there you'd have to manually update all axes in the constraint group)

If you relayout (or react) two or more axes in a group in such a way as to explicitly violate the constraints, one or more of those axes will expand (never shrink) its range to fit the constraints.

So if you want to clean this up we should first add a few tests explicitly for ^^. I see some gui tests and one restyle test that uses constraints, but unfortunately no relayout or react tests of these edge cases.

Ok if I open up an issue about this and defer this to a later PR?

Ok if I open up an issue about this and defer this to a later PR?

Yep for sure - that was the "if you want to clean this up" clause

Moved to -> #2540

alexcjohnson · 2018-04-05T22:01:40Z

double-click (i.e auto/reset range) performance. This thing currently goes through editType: 'calc' which is slow as a 🐢

Axes.expand (which happens during calc does nothing if the axis isn't autoranged - which can speed up initial drawing but means if you turn on autorange later you don't know whether ax._min/_max are populated. We could either calculate min/max no matter what during calc, or at least stash the arrays & options used to generate them so that when autorange is enabled we can pull it back out and proceed with the calculation without needing to go all the way through calc again.

There would also be some advantages to keeping separate track of each trace / object that called Axes.expand - ie keep a separate _min/_max for each trace/shape/annotation etc. and only combine them when finally calculating the range. That would avoid calc for visibility changes, for example (you'd just have to know to include or not include that object's _min/_max) and for moving annotations/shapes, and down the road would allow calc per trace.

alexcjohnson · 2018-04-10T21:44:45Z

src/components/shapes/draw.js

+
+    for(var k in fullLayout._plots) {
+        var shapelayer = fullLayout._plots[k].shapelayer;
+        if(shapelayer) shapelayer.selectAll('path').remove();


Interesting - why is this better? Wasn't fullLayout._shapeSubplotLayers essentially just an array of all the shapelayer nodes? Or is the win actually below in subroutines.js that you don't have to do a selectAll to find these?

Or is the win actually below in subroutines.js that you don't have to do a selectAll to find these?

That. selectAll are bad.

alexcjohnson · 2018-04-10T21:46:05Z

src/plots/cartesian/dragbox.js

@@ -1105,7 +1105,7 @@ function calcLinks(gd, xaHash, yaHash) {
    var yaHashLinked = {};
    var yaxesLinked = [];
    for(yLinkID in yLinks) {
-        var ya = getFromId(gd, xLinkID);
+        var ya = getFromId(gd, yLinkID);


yikes, good catch!

etpinard · 2018-04-11T14:41:44Z

Unfinished work has been ✏️ in:

faster axis autorange relayout: Faster axis autorange relayout #2546
per-axis 'axrange' relayout: Per axis axrange edit type #2547
stash pts selections for pan: Stash SVG points selections for faster pan #2548
speed up cartesian axis defaults: Speed up cartesian axes supplyDefaults #2549

(to get tests to pass)

alexcjohnson · 2018-04-11T19:55:26Z

@etpinard really impressive - and counterintuitive - work here!
💃 💃 🐎 🚀

etpinard added 7 commits April 5, 2018 14:25

move doAutoRangeAndConstraints, drawData and finalDraw to subroutines

02ed2eb

8000
... (i.e. out of plot_api.js), and move Plots.addLinks out of drawData into its own step in Plotly.plot.

add 'axrange' editType

3ad1eaa

- this represents the minimal sequence for '(x|y)axis.range' relayout calls which are pretty common (e.g. on zoom/pan mouseup). - by bypassing drawFramework, lsInner and initInteraction, this can save ~1000ms on 50x50 subplot grids.

improve fullLayout._has()

d07ae70

- allow fullLayout._has(/*trace type*/) to work - use registry category hash object (instead of categories.indexOf) to find categories in fullLayout._modules

clean splom drag logic

0979272

lint in plot_api

ebb35ce

etpinard added status: in progress labels Apr 5, 2018

etpinard commented Apr 5, 2018

View reviewed changes

alexcjohnson reviewed Apr 5, 2018

View reviewed changes

etpinard commented Apr 5, 2018

View reviewed changes

alexcjohnson reviewed Apr 5, 2018

View reviewed changes

etpinard added 4 commits April 6, 2018 11:13

(fixup) add draggedPts to scattercarpet categories

f9090b7

Merge branch 'splom-feature' into splom-zoom-perf

b2ee736

Merge branch 'splom-feature' into splom-zoom-perf

a0b2574

replace selectAll with for(k in _plots) loop

d3fe40d

etpinard added this to the v1.36.0 milestone Apr 10, 2018

fixup and 🔒 dragbox with linked y axes

cc1b3de

alexcjohnson reviewed Apr 10, 2018

View reviewed changes

etpinard added 2 commits April 11, 2018 10:42

Merge branch 'splom-feature' into splom-zoom-perf

980855c

(to get tests to pass)

📚 link TODOs to gh issues

f7d637d

etpinard added status: reviewable and removed status: in progress labels Apr 11, 2018

etpinard merged commit 978a70e into splom-feature Apr 11, 2018

etpinard deleted the splom-zoom-perf branch April 11, 2018 20:44

etpinard mentioned this pull request Apr 13, 2018

Splom open items #2555

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Splom zoom perf #2527

Splom zoom perf #2527

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	// figure out if we need to recalculate axis constraints
	var constraints = fullLayout._axisConstraintGroups \|\| [];
	for(axId in rangesAltered) {
	for(i = 0; i < constraints.length; i++) {
	var group = constraints[i];
	if(group[axId]) {
	// Always recalc if we're changing constrained ranges.
	// Otherwise it's possible to violate the constraints by
	// specifying arbitrary ranges for all axes in the group.
	// this way some ranges may expand beyond what's specified,
	// as they do at first draw, to satisfy the constraints.
	flags.calc = true;
	for(var groupAxId in group) {
	if(!rangesAltered[groupAxId]) {
	Axes.getFromId(gd, groupAxId)._constraintShrinkable = true;
	}
	}
	}
	}
	}

Uh oh!

Splom zoom perf #2527

Splom zoom perf #2527

Uh oh!

Conversation

Things improved so far

Things I'll like to improve in the coming days

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!