Misc performance optimization #1585
Conversation
|
The way I cache those objects is not ideal. Please suggest the desired way to cache objects, during the review. Thanks! |
|
I skipped the colorbar layout step when colorbar is not enabled for a trace: f4196f9. But the change breaks 4 unit test cases so I decide to roll it back for now. Maybe I misunderstood the necessity of having colorbar layouted during calling |
| @@ -302,7 +303,7 @@ Plotly.plot = function(gd, data, layout, config) { | |||
| .selectAll(query) | |||
| .remove(); | |||
|
|
|||
| fullLayout._infolayer.selectAll('g.rangeslider-container') | |||
| rangesliderContainers | |||
There was a problem hiding this comment.
Interesting. How does this result in a perf improvement?
There was a problem hiding this comment.
Did not tested separately. Just checked and my tests shows 200ms to 400ms improvement on 200 traces.
When I profile the 200-trace case, the select and selectAll are among top 5 of hot functions. So my thought was caching all the constant selected elements in data loops.
There was a problem hiding this comment.
Right. I understand the value of caching selections, but here you're just assigning the selection to a variable: .selectAll is still called inside a loop.
There was a problem hiding this comment.
.selectAll('g.rangeslider-container') is moved out of the loop, maybe that's where the save comes from.
There was a problem hiding this comment.
Ha. Now I see it (it isn't part of the GH diff view). Thanks 👍
| @@ -512,32 +513,36 @@ drawing.bBox = function(node) { | |||
| return Lib.extendFlat({}, savedBBoxes[saveNum.value]); | |||
| } | |||
|
|
|||
| var test3 = d3.select('#js-plotly-tester'), | |||
There was a problem hiding this comment.
Nicely done. There's only one tester div per page, so might as well cache in the Drawing module object. 👍
There was a problem hiding this comment.
Not sure if I understand correctly. I did cache the tester div in line 517 and 518 already. Is that what you are referring to?
There was a problem hiding this comment.
I'm saying this is a great commit.
There was a problem hiding this comment.
Got used to expect modification suggestions in a code review lol. Thanks!
src/plot_api/plot_api.js
Outdated
| @@ -205,7 +205,7 @@ Plotly.plot = function(gd, data, layout, config) { | |||
| if(trace.visible !== true || !trace._module.colorbar) { | |||
| Plots.autoMargin(gd, 'cb' + trace.uid); | |||
| } | |||
| else trace._module.colorbar(gd, cd); | |||
| else if(trace.marker.colorbar) trace._module.colorbar(gd, cd); | |||
There was a problem hiding this comment.
This new if() makes traces that have root-level colorbar settings (e.g. heatmap, contour, surface) bypass the colorbar draw step.
Something like
else if(Plots.traceIs(trace, 'markerColorscale') && Colorbar.hasColorbar(trace)) {
trace._module.colorbar(gd, cd);
}might makes things work.
There was a problem hiding this comment.
I made the changes and run test locally. It still fails in the contour/heatmap traces assertion on colorbar, https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/plot_interact_test.js#L223 and https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/plot_interact_test.js#L283. Both countColorBars function returns 0.
I will take a look. And it would be great if you could shed some lights here.
There was a problem hiding this comment.
I did not find any problems and tried again today, the local test passed this time. Not sure why it failed last time. So I pushed the commit you suggested.
|
Thanks (again) @hy9be for these perf improvements 👍 |
|
I fetch and merged the latest master and all tests passed locally, but CI still has couple of cases failed. Not sure why. |
|
@etpinard I fixed all jasmine test cases but the CI failed at some other commands which I do not know how to debug. Could you please give me some hints here? Basically I do not really understand the error reported by CI...Thanks! |
|
I think adding Or perhaps, replacing else if(Plots.traceIs(trace, 'markerColorscale') || Colorbar.hasColorbar(trace)) {}with else if(Plots.traceIs(trace, 'markerColorscale') || Plots.traceIs(trace, 'parcoords') || Colorbar.hasColorbar(trace)) {} |
|
@etpinard I tried both ways you suggested and did some further investigation, still cannot figure out a way to pass all the image passed. |
|
@etpinard I removed the optimization for colorbar. Now all tests passed. Just let you know. |
|
@hy9be looks like this PR is ready to be merged. 🎉 But before, since you chose to not include that colorbar optimisation, can you rebase and remove all the colorbar-related commits locally and then |
|
@etpinard I removed all colorbar related commits. Please see if that looks good. Thanks! |
Following my attempts in #1544, #1555, and #1557, I did some work to further improve the performance of Plotly. I scan through timeline results I captured on slow charts, which has 200+ traces, and did some caching. I also tried to skip processing on some components (e.g. colorbar) when it is enabled for a graph.
By doing all these, I observed about 15% improvements on bar chart and marginal improvements on scatter plot.