Throttle selectPoints #2040

alexcjohnson · 2017-09-28T02:27:32Z

closes #2005 - adds throttling to box and lasso selection. I opted to throttle just the selectPoints portion, because It's where nearly all the expense of that drag handler lives, so this way keeps the selection region feeling snappy even if it takes some time for the selected points to catch up.

Here's what it looked like before:

and now with throttling:

Using this plot:

var x = new Array(3e5), y = new Array(x.length);
for(var i = 0; i < x.length; i++) { x[i] = Math.random(); y[i] = Math.random(); }
Plotly.newPlot(gd,
    [{x: x, y: y, type: 'scattergl', mode: 'markers', marker: {opacity: 0.1}}],
    {width: 600, height: 600})

@rreusser

turns out this leads to stack errors when used with very large arrays anyway it's redundant for the first trace. thx @rreusser!

alexcjohnson · 2017-09-28T02:30:00Z

src/components/fx/hover.js

+    Lib.throttle(
+        function() { _hover(gd, evt, subplot, noHoverEvent); },
+        constants.HOVERMINTIME,
+        gd._fullLayout._uid + constants.HOVERID


using fullLayout._uid to construct the key to what we're throttling means we can 🔪 gd._lastHoverTime and gd._hoverTimer

Loving this ❤️

alexcjohnson · 2017-09-28T02:33:46Z

src/lib/get_graph_div.js

+// Get the container div: we store all variables for this plot as
+// properties of this div
+// some callers send this in by DOM element, others by id (string)
+module.exports = function(gd) {


oops, this file was supposed to go in the next commit aa434dd

docs cleaned up in 0861736

alexcjohnson · 2017-09-28T02:38:14Z

src/plots/cartesian/select.js

@@ -186,9 +186,15 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) {
        selection = [];
        for(i = 0; i < searchTraces.length; i++) {
            searchInfo = searchTraces[i];
-            [].push.apply(selection, fillSelectionItem(


not a reported bug AFAIK, but this use of push.apply overloads the stack for large plots. Doesn't look to me as though any of the other places we use push.apply are susceptible to this issue.

Anyone interested in writing a syntax test to 🔒 this down?

we do use push.apply in a few other places - but they get max a few items in the array so it's not a problem - so if we wanted to 🔒 it, we'd have to also remove those other uses.

@alexcjohnson funny enough I stumbled upon this stack overflow in selection for regl. I replaced it with .concat here https://github.com/plotly/plotly.js/pull/1869/files#diff-8c5d8f688f2bb41a18d35397e5f58864R211, since it is being called only as many times as there is traces per subplot, which is small. (related tweet)

alexcjohnson · 2017-09-28T02:43:03Z

src/plots/cartesian/select.js

-                searchInfo = searchTraces[i];
-                searchInfo.selectPoints(searchInfo, false);
-            }
+        throttle.done(throttleID).then(function() {


we need to wait for any throttling-delayed move event before we process the mouseup event, so we have the correct data.

alexcjohnson · 2017-09-28T02:50:25Z

test/jasmine/tests/select_test.js

+        .then(function() {
+            // deselect gives an extra plotly_selected event on the first click
+            // event data is undefined
+            checkEventCounts(1, 2, 1, 'deselect');


The gd.once we were using in this test before hid this behavior - which exists on master as well as in this branch. I imagine we would prefer to not emit these events with undefined data - which happen I guess when we get here but dragged === false. @etpinard shall I remove that extra event and put this test back as it appeared to be previously?

I imagine we would prefer to not emit these events with undefined data

Hmm. I'm thinking some users might find it useful to have plotly_selected fired even when the selection polygon is smaller than the minimum allowed. In this case, plotly_selected essentially mimics plotly_deselect - which some users might not know about.

So, I'd vote for keeping the current behavior and updating the test (good catch by the way 🏅 )

But, maybe this is something we might want to change in v2, especially now that click selections are on the table for future development.

alexcjohnson · 2017-09-28T02:51:48Z

test/jasmine/tests/select_test.js

            assertPoints([{lon: 20, lat: 20}]);
            assertLassoPoints([
                [13.28, 25.97], [13.28, 14.33], [25.71, 14.33], [25.71, 25.97], [13.28, 25.97]
            ]);
+            checkEventCounts(4, 1, 0, 'lasso');


4 plotly_selecting events is real - because there are 5 points in the lasso path.

rreusser · 2017-09-28T03:17:56Z

Interestingly, I noticed today that selecting a box + heatmapgl can be very slow. I'm curious whether that's the same pathway.

etpinard · 2017-09-28T15:13:25Z

that selecting a box + heatmapgl can be very slow

I didn't know you could select heatmapgl traces 😕

etpinard

Looking good. That new lib/throttle.js module is very very nice.

Your patches in select_test.js scare me a little bit as they will conflict with e53a908.

etpinard · 2017-09-28T14:53:53Z

src/components/fx/hover.js

+    Lib.throttle(
+        function() { _hover(gd, evt, subplot, noHoverEvent); },
+        constants.HOVERMINTIME,
+        gd._fullLayout._uid + constants.HOVERID


Loving this ❤️

etpinard · 2017-09-28T14:58:28Z

src/plots/plots.js

@@ -1327,8 +1327,6 @@ plots.purge = function(gd) {
    delete gd.hmlumcount;
    delete gd.hmpixcount;
    delete gd.numboxes;
-    delete gd._hoverTimer;
-    delete gd._lastHoverTime;


etpinard · 2017-09-28T14:58:53Z

test/jasmine/tests/cartesian_interact_test.js

@@ -446,7 +446,7 @@ describe('Event data:', function() {
    function _hover(px, py) {
        return new Promise(function(resolve, reject) {
            gd.once('plotly_hover', function(d) {
-                delete gd._lastHoverTime;
+                Lib.clearThrottle();


etpinard · 2017-09-28T15:00:10Z

src/components/dragelement/unhover.js


 var hoverConstants = require('../fx/constants');

 var unhover = module.exports = {};


 unhover.wrapped = function(gd, evt, subplot) {
-    if(typeof gd === 'string') gd = document.getElementById(gd);
+    gd = getGraphDiv(gd);


etpinard · 2017-09-28T15:02:08Z

src/plots/cartesian/select.js

@@ -186,9 +186,15 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) {
        selection = [];
        for(i = 0; i < searchTraces.length; i++) {
            searchInfo = searchTraces[i];
-            [].push.apply(selection, fillSelectionItem(


Anyone interested in writing a syntax test to 🔒 this down?

etpinard · 2017-09-28T15:04:36Z

src/plots/cartesian/constants.js

+    SELECTDELAY: 100,
+
+    // cache ID suffix for throttle
+    SELECTID: '-select',


Non-blocking, but we should start thinking about moving selection things out of plots/cartesian/ and into e.g. component/fx/ or a new component/select module.

etpinard · 2017-09-28T15:05:09Z

src/plots/cartesian/select.js

-            if(selection.length) {
-                for(var j = 0; j < thisSelection.length; j++) {
-                    selection.push(thisSelection[j]);
+        throttle.throttle(


etpinard · 2017-09-28T15:08:26Z

src/plots/cartesian/select.js

-                searchInfo.selectPoints(searchInfo, false);
-            }
+        throttle.done(throttleID).then(function() {
+            throttle.clear(throttleID);


Maybe throttle.done() should call throttle.clear() before returning a promise?

Maybe throttle.done() should call throttle.clear() before returning a promise?

I don't think so. It doesn't come into play here, but you could imagine wanting to finish whatever event had been throttled when done was called, without cutting off throttling of future events in the train. I guess that's saying I didn't mean done to mean you were necessarily done forever, just done with whatever is in the queue right now.

Got it. Thanks!

etpinard · 2017-09-28T15:11:00Z

test/jasmine/tests/select_test.js

+    var selectingCnt, selectingData, selectedCnt, selectedData, deselectCnt, doubleClickData;
+    var selectedPromise, deselectPromise;
+
+    function resetEvents(gd) {


Ha. This will conflict massively with e53a908

Not sure who should get priority here.

#2030 is epic enough already, you don't need to add a big merge resolution to it. You can merge first.

etpinard · 2017-09-28T15:19:57Z

src/lib/throttle.js

+ * @param {number} minInterval: minimum time, in milliseconds, between
+ *   invocations of `callback`
+ */
+exports.throttle = function throttle(callback, minInterval, id) {


I think throttle(id, minInterval, callback) would a call signature more familiar to most (node.)js devs.

-> b6d64be

etpinard · 2017-09-28T15:22:57Z

src/lib/throttle.js

+    _clearTimeout(cache);
+
+    function exec() {
+        callback();


So callback can't itself be async - which is fine but deserves a mention in the docstring.

-> b6d64be

alexcjohnson · 2017-09-28T21:31:21Z

test/jasmine/tests/select_test.js

-                x1 = 150,
-                y1 = 250,
-                x2 = 50,
-                y2 = 50;


(x2, y2) is in the top margin of this plot, so the doubleclick didn't do anything. But since these tests didn't actually check that the handlers ever got called, it didn't cause a failure.

Nice investigation work. Thanks!

alexcjohnson · 2017-09-28T21:45:09Z

@etpinard I had to do (as expected) a fair amount of work pruning merge conflicts in select_test - would you mind giving it a look to make sure I preserved your intentions?

and beef up its docstring

etpinard · 2017-09-28T22:01:43Z

would you mind giving it a look to make sure I preserved your intentions?

select_test.js is looking very nice. That resetEvents abstraction is 💯

I'm looking forward to adding tests for bar/histogram traces tomorrow. 🍻

and increase tolerance to get all tests passing locally

etpinard · 2017-09-28T22:25:47Z

Great. Tests are ✅ ↪️ 💃

alexcjohnson added 5 commits September 26, 2017 17:45

generalized event throttling routine

a6df912

move getGraphDiv to lib so we can use it outside plot_api

aa434dd

unwrap [].push.apply

753b2c9

turns out this leads to stack errors when used with very large arrays anyway it's redundant for the first trace. thx @rreusser!

throttle just the selectPoints part of box/lasso selection

256c5c7

throttle.done method and test updates

80508e8

alexcjohnson commented Sep 28, 2017

View reviewed changes

fix docs for getGraphDiv

0861736

alexcjohnson commented Sep 28, 2017

View reviewed changes

etpinard added status: reviewable labels Sep 28, 2017

etpinard added this to the v1.31.0 milestone Sep 28, 2017

etpinard reviewed Sep 28, 2017

View reviewed changes

alexcjohnson added 2 commits September 28, 2017 16:51

Merge branch 'master' into throttle-select

1968af7

remove silently broken and obsolete select box node creation tests

6da3cfa

alexcjohnson commented Sep 28, 2017

View reviewed changes

alter argument order of throttle.throttle

b6d64be

and beef up its docstring

clean up geo_test for new throttle

811073e

and increase tolerance to get all tests passing locally

alexcjohnson merged commit e7c7149 into master Sep 28, 2017

alexcjohnson deleted the throttle-select branch September 28, 2017 22:47

Throttle selectPoints #2040

Throttle selectPoints #2040

Conversation

alexcjohnson commented Sep 28, 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

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

Choose a reason for hiding this comment

rreusser commented Sep 28, 2017

etpinard commented Sep 28, 2017 • edited Loading

etpinard left a 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

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

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

Choose a reason for hiding this comment

etpinard Sep 28, 2017 • edited Loading

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

alexcjohnson commented Sep 28, 2017

etpinard commented Sep 28, 2017

etpinard commented Sep 28, 2017

etpinard commented Sep 28, 2017 •

edited

Loading

etpinard Sep 28, 2017 •

edited

Loading