Skip to content

Optimize the performance of scatter trace sorting comparator with map - #1555

Merged
etpinard merged 1 commit into
plotly:masterfrom
hy9be:perf-scatterlayersort
Apr 7, 2017
Merged

Optimize the performance of scatter trace sorting comparator with map#1555
etpinard merged 1 commit into
plotly:masterfrom
hy9be:perf-scatterlayersort

Conversation

@hy9be

@hy9behy9be commented Apr 5, 2017

Copy link
Copy Markdown
Contributor

Construct a map rather than an array to improve the performance of index search. Jasmine tests passed.

Construct a map rather than an array to improve the performance of index search. Jasmine tests passed.
@hy9behy9be changed the title Optimize the scatter trace sorting comparator with mapOptimize the performance of scatter trace sorting comparator with mapApr 5, 2017
@etpinard

Copy link
Copy Markdown
Contributor

Looks good to me.

@rreusser can you think of any side effects (e.g. for animations)?

// and recreating on every update.
for(i = 0, uids = []; i < cdscatter.length; i++) {
uids[i] = cdscatter[i][0].trace.uid;
for(i = 0, uids = {}; i < cdscatter.length; i++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems entirely equivalent to me 👍

I didn't anticipate this would be more than like 10-15 or so, which would make the overhead of indexOf negligible. I'll keep this in mind when contemplating orders of magnitude!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, actually there is almost no difference on map and array here if the amount of trace is small. My cases might be a bit crazy (with 5000+ traces). But it seems worthwhile to use map instead here since there is almost no side effect.

@etpinard

etpinard commented Apr 7, 2017

Copy link
Copy Markdown
Contributor

@hy9be before merging this, would you sharing a few before / after benchmark numbers?

(Can't wait to wrap up #1511)

@rreusser

rreusser commented Apr 7, 2017

Copy link
Copy Markdown
Contributor

Suggested benchmark strategy for this case (though end-to-end is relevant too):

vart0=performance.now()// if your browser supports it, otherwise Date.now()...theabovelinesofcodebefore,thenaftervart1=performance.now()console.log('Time taken to sort:',t0-t1)

The fix seems appropriate either way (I should have known better in the first place 🙄 ), it's just very useful to have a sense for the cost and gains of optimizations like this.

@hy9be

hy9be commented Apr 7, 2017

Copy link
Copy Markdown
ContributorAuthor

Here is my script.

And some data points collected on my computer (time in ms):
image

@etpinard

Copy link
Copy Markdown
Contributor

@hy9be thanks for that!

@etpinardetpinard added this to the v1.26.0 milestone Apr 7, 2017
@etpinard
etpinard merged commit 10acee0 into plotly:masterApr 7, 2017
@etpinard

Copy link
Copy Markdown
Contributor

By the way, @hy9be thanks for all your contributions this week!

You get my vote for contributor of the week 🏆

@hy9be

hy9be commented Apr 7, 2017

Copy link
Copy Markdown
ContributorAuthor

Wow...feel flattered. Thanks!

I had some exposure to the idea of grammar of graphics. Plotly is not quite an implementation of that but the design is much more orthogonal and abstract than most other charting libraries. I like the project.

Will try to help more.

@rreusser

Copy link
Copy Markdown
Contributor

Thanks for the benchmark! Honestly I didn't think that single call would make a big enough difference to have that obvious of an effect in an end-to-end test. 🐢

@hy9behy9be mentioned this pull request Apr 11, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@hy9be@etpinard@rreusser