Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVG trace on selection perf #2583

Merged
merged 12 commits into from May 1, 2018

Conversation

Projects
None yet
2 participants
@etpinard
Copy link
Member

etpinard commented Apr 27, 2018

resolves #2459

There's still plenty of room for improvements (for example via #2548), but this PR gets scatter selection perf back to a level comparable to before the persistent selection PR (in #2135). Unfortunately, this means adding yet another method on the trace module objects. I hope @alexcjohnson won't mind. In brief, the new styleOnSelect methods only update DOM styles corresponding the selected and unselected attributes and thus by-passes much of the regular style DOM updates.

In numbers from the start to end of the selecting throttle using this codepen from #2459, before this PR we had ~300-400ms: and after: ~40-60ms

In gifs:

before

peek 2018-04-27 10-19

after

peek 2018-04-27 10-20

etpinard added some commits Apr 19, 2018

add 'styleOnSelect' method to Scatter* module
... that bypasses DOM style things selection doesn't affect.
    This speeds up on-selection perf and brings in back
    to a level comparably to before the persistent selection PR.
@@ -404,18 +404,20 @@ function updateSelectedState(gd, searchTraces, eventData) {
var len = items.length;
var item0 = items[0];
var trace0 = item0.cd[0].trace;
var _module = item0._module;
var fn = _module.styleOnSelect || _module.style;

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 27, 2018

Contributor

I like styleOnSelect, very nice solution!

But please call it something more descriptive than fn here - styleSelection?

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 27, 2018

Author Member

sure thing 😉

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 30, 2018

Author Member

done in 0e91c6e

Drawing.selectedPointStyle(s.selectAll('path'), trace, gd);
Drawing.selectedPointStyle(s.selectAll('text'), trace, gd);
} else {
stylePoints(s, trace, gd);

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 27, 2018

Contributor

Why do we need this else? I don't see a corresponding case in the previous code...

BTW, I'm hesitant to suggest this after the .select().select() data-mangling mess, but does it work to do Drawing.selectedPointStyle(s.selectAll('path,text'), trace, gd)? Is that any faster?

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 27, 2018

Author Member

Ah oops, this should be Drawing.selectedTextStyle(s.selectAll('text'), trace, gd). I'll add a test.

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 27, 2018

Author Member

Why do we need this else? I

To get back to the original state after double-click. Drawing.selectedPointStyle and Drawing.selectedTextStyle only handle selected / unselected styling off a "base" state given by stylePoints.

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 30, 2018

Author Member

I'll add a test.

added in 01df2d2

etpinard added some commits Apr 27, 2018

refactor selected points drawing methods
- make Drawing.pointStyle know how to apply [un]selected styles,
  so that we don't need to update DOM styles twice when we have
  set `selectedpoints`.
- make Drawing.selected(Point|Text)Style no longer need a base state
  (coming from Drawing.pointStyle) to apply correct styles.
add styleOnSelect test
... as on-selection styling now goes a different pathway than
    restyle().
@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Apr 30, 2018

Commit 3776e8f is a little messy, so I'll explain it here:

Previously (i.e. since the persistent selection PR #2135), [un]selected styles were applied to the DOM nodes assuming that the base style had been already applied to DOM nodes. Meaning that to get [un]selected style right, we needed to call Drawing.pointStyles first and then Drawing.selectedPointStyle() + Drawing.selectedTextStyle(). This turned out to be slow especially during selection (i.e. on drag). In other words, before commit 3776e8f, calling Drawing.selected(Point|Text)Style without Drawing.pointStyle lead to bugs.

Now post commit 3776e8f, Drawing.pointStyles knows how to apply [un]selected styles. This means DOM nodes style attributes are updated only once per Plots.style call. This should improve first-render perf for traces with [un]selected attributes.

Moreover, Drawing.selected(Point|Text)Style don't assume a base state anymore. They know how to handle cases where certain [un]selected keys are unset. This means that styleOnSelect (which bypasses Drawing.pointStyle for speed) now does the right thingTM.

};

drawing.singlePointStyle = function(d, sel, trace, markerScale, lineScale, gd) {
drawing.makePointStyleFns = function(trace) {

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 30, 2018

Author Member

Define trace-wide per-pt functions w/o knowledge of d3 or DOM methods so that non-svg modules (e.g. scattermapbox) can reuse them ♻️

s.each(function(d) {
for(var i = 0; i < seq.length; i++) {
seq[i](d3.select(this), d);
}

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 30, 2018

Contributor

interesting approach with seq - it's not obvious to me that this is meaningfully different from 3 s.each calls inside if(functionExists) blocks, or 3 if(functionExists) blocks inside one s.each, did it make a big difference to do it this way?

This comment has been minimized.

Copy link
@etpinard

etpinard May 1, 2018

Author Member

d3's each are a little slower than regular forEach and for-loops. Nothing worth losing sleep over, but we do gains a few ms using this sequence thing.

This comment has been minimized.

Copy link
@etpinard

etpinard May 1, 2018

Author Member

... by gain, I mean lose a few ms and gain performance. 😄

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

alexcjohnson commented May 1, 2018

Looks great! Thanks for the help understanding 3776e8f

💃

@etpinard etpinard merged commit 2b1713d into master May 1, 2018

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@etpinard etpinard deleted the scatter-select-perf branch May 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.