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

Optimize arrayUnique #6871

Merged
merged 4 commits into from
Dec 31, 2019
Merged

Optimize arrayUnique #6871

merged 4 commits into from
Dec 31, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Dec 29, 2019

uPlot (time: linear / data)
arrayUnique
70ms -> 15ms

etimberg
etimberg previously approved these changes Dec 30, 2019
@leeoniya
Copy link

Set would be more appropriate than Map, and the .has() check is unnecessary (and slower).

@benmccann benmccann mentioned this pull request Dec 30, 2019
@kurkle
Copy link
Member Author

kurkle commented Dec 30, 2019

Changed to Set. I guess using has could be faster if there are a lot of duplicates. There are only 3 of each timestamp in that uPlot case. No measurable difference either way in that case.

src/scales/scale.time.js Outdated Show resolved Hide resolved
@leeoniya
Copy link

I guess using has could be faster if there are a lot of duplicates.

i dont see why that would be the case. Set must do this test internally in all cases either way. calling has() as a precondition to add() is never necessary or beneficial (i think).

@kurkle
Copy link
Member Author

kurkle commented Dec 30, 2019

i dont see why that would be the case. Set must do this test internally in all cases either way. calling has() as a precondition to add() is never necessary or beneficial (i think).

You are probably right when talking about Set.
Map OTOH has to update the value too, which is relatively expensive.
jsperf

benmccann
benmccann previously approved these changes Dec 30, 2019
@benmccann
Copy link
Contributor

Doing the has check seems slightly faster for Set as well: https://jsperf.com/chartjs-set-with-or-without-has

src/scales/scale.time.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented Dec 31, 2019

Using spread instead of Array.from. Seems to be as fast.

@etimberg etimberg merged commit ab39286 into chartjs:master Dec 31, 2019
@kurkle kurkle deleted the array-unique branch February 19, 2020 18:32
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