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

Perf improvement for ticks.source:'labels' #6354

Merged
merged 3 commits into from
Jul 18, 2019

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jun 23, 2019

We only need to add the labels to timestamps once - not once per dataset

We also don't need to call arrayUnique on the labels because duplicate labels aren't supported. If you pass duplicate labels and then we call arrayUnique then we end up with fewer labels than data points, so this code never worked. arrayUnique is quite expensive for large datasets, so it's very helpful to remove. And then we no longer need to call sort which is also expensive. That was done only because arrayUnique may not return items in the order they were passed in

@etimberg etimberg requested review from simonbrunel and kurkle and removed request for simonbrunel June 23, 2019 22:05
etimberg
etimberg previously approved these changes Jun 23, 2019
@kurkle
Copy link
Member

kurkle commented Jun 24, 2019

What about ticks.source: 'auto', labels: '2015', '2016', data: '2015-01' .. '2015-12'. Labels are going to be first in array, months last.

Another thing to consider is the way labels are implemented. Those might as well be intended for a category scale in a time/category chart. So we need the check if its object data or not.

Then we could have a mixed set of datasets with object data and plain data. So would need to check if any of those datasets have plain data and add labels then.

I'd maybe add a labelsAdded flag, for not adding them many times. Maybe another flag for objectData and sort only if both are set.

There is also a bug (in master), we are not checking if dataset is any of current scales business.
Examples

@benmccann
Copy link
Contributor Author

Thanks for the great review @kurkle. I've made the changes you suggested

src/scales/scale.time.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
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.

4 participants