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

[performance] replace helpers.each calls with for-loops #6301

Merged
merged 3 commits into from Jun 22, 2019

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented May 27, 2019

for-loop and Math.min/max is much faster than helpers.forEach and if statements: https://jsperf.com/chartjs-helpers-each

I was focused on determineDataLimits since that's a particularly costly method that iterates over each data point. There are more places where helpers.each is called in less performance-sensitive locations where there wasn't a compelling reason to change it

The scale behavior was really inconsistent. If there was no data then the tests expected the scale to start at -1. I changed it to start at 0 instead for consistency. E.g. I would expect it to behave the same when there's no data and all data points are hidden. When all data points are hidden then the tests expect the scale to start at 0. E.g.:

expect(chart.scales['x-axis-0'].min).toEqual(0);

@benmccann benmccann force-pushed the helpers-each branch 4 times, most recently from b5ba6c0 to 3b0b502 Compare May 27, 2019 04:56
etimberg
etimberg previously approved these changes Jun 16, 2019
nagix
nagix previously approved these changes Jun 17, 2019
Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

There might be some user applications that expect to have the scale range [-1, 1] with no data, but it does not seem to be the intention of the current implementation, and for better consistency, I'm fine with the changes in this PR.

kurkle
kurkle previously approved these changes Jun 19, 2019
src/scales/scale.linear.js Outdated Show resolved Hide resolved
@benmccann benmccann dismissed stale reviews from kurkle, nagix, and etimberg via e4b59fc June 19, 2019 15:05
@benmccann benmccann requested a review from kurkle June 21, 2019 17:16
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
* [performance] replace helpers.each calls with for-loops

* Use Math.min/max instead of if statement

* Rename variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants