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

Repeated x value calculations in updateDatasets #3211

Closed
desowin opened this issue Aug 25, 2016 · 4 comments
Closed

Repeated x value calculations in updateDatasets #3211

desowin opened this issue Aug 25, 2016 · 4 comments

Comments

@desowin
Copy link
Contributor

desowin commented Aug 25, 2016

updateDatasets function calls update() on every dataset controller which in turn loops on all points and calls updateElement(). Update element calculates x using xScale.getPixelForValue().

If there is a chart with M (example: 12) datasets (N points each, example: 4000) sharing the same label then exactly the same value x gets callculated M times for every updateDatasets() call. This results in O(N*M) calls to getPixelForValue(). As the x values are the same for all datasets it should be possible to optimize this to O(N) calls to getPixelForValue().

Performance improvement would be especially visible when time scale is used, as it calls moment.js diff() function. In example (M=12, N=4000) built-in Chrome profiler shows over 40% of cpu usage for diff() function.

This optimization would require code refactoring. What would be the best way to achieve the goal?

@etimberg
Copy link
Member

Hmm, as you noticed this would require code refactoring. But, there's also no guarantee that the x values are always the same for every dataset. For a scatter chart, they can be different.

You could add a cache to the time scale so that each value is only calculated once

desowin added a commit to desowin/Chart.js that referenced this issue Sep 3, 2016
This reduces number of calls to momentjs diff() if there are multiple
datasets sharing the same labels (chartjs#3211).
desowin added a commit to desowin/Chart.js that referenced this issue Sep 3, 2016
This reduces number of calls to momentjs diff() if there are multiple
datasets sharing the same labels (chartjs#3211).
@desowin
Copy link
Contributor Author

desowin commented Sep 3, 2016

020ac35 improves the performance around 3 times for my use case (update() takes now 1 second instead of 3 seconds for M=12, N=4000).

I slightly modified the behavior of getPixelForValue() when called with all three parameters. Is such change acceptable?

@desowin
Copy link
Contributor Author

desowin commented Sep 3, 2016

Test script that I use is available at https://jsfiddle.net/desowin/j2s46z3o/
Default JSFiddle N is 200 because it takes quite some time to init for bigger N.

desowin added a commit to desowin/Chart.js that referenced this issue Sep 4, 2016
Previously buildLabelDiffs() was called at the end of buildTicks() and
hence labelDiffs cache was calculated twice (in getMinimumBoxSize() and
then in fitBox()).
@etimberg
Copy link
Member

Done in #3261

exwm pushed a commit to exwm/Chart.js that referenced this issue Apr 30, 2021
This reduces number of calls to momentjs diff() if there are multiple
datasets sharing the same labels (chartjs#3211).
exwm pushed a commit to exwm/Chart.js that referenced this issue Apr 30, 2021
Previously buildLabelDiffs() was called at the end of buildTicks() and
hence labelDiffs cache was calculated twice (in getMinimumBoxSize() and
then in fitBox()).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants