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 "uniq" to be O(n) instead of O(n^2) #2537

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Optimize "uniq" to be O(n) instead of O(n^2) #2537

wants to merge 2 commits into from

Conversation

Tzook
Copy link

@Tzook Tzook commented May 13, 2016

The uniq function does heavy operations that could cause performance issues on big arrays.
This commit utilizes the Map object if supported (95% browser support according to caniuse), to make such cases much faster.
Running a simple example in Chrome on an array with 10,000 unique elements:

  • Before - takes about 8 seconds
  • After - takes about 0.03 seconds

@coveralls
Copy link

coveralls commented May 13, 2016

Coverage Status

Coverage increased (+0.02%) to 96.887% when pulling e462042 on Tzook:master into 53086b0 on jashkenas:master.

@jdalton
Copy link
Contributor

jdalton commented May 13, 2016

Keep in mind this toggles the matching behavior from strict equality to same value zero depending on environment support.

@Tzook
Copy link
Author

Tzook commented May 14, 2016

Good point @jdalton.
The only differences according to this table are that -0 is not equal to +0, and that NaN equals to NaN.
In my opinion it makes even more sense for the function to use same-value-zero comparison. Having an array with multiple NaNs should result in only one NaN after using unique.

@jdalton
Copy link
Contributor

jdalton commented May 14, 2016

Yep.

@forty
Copy link

forty commented May 4, 2017

Hello everyone,
is this PR ever going to be merged, or does it need additional work?

Thanks

@jgonggrijp
Copy link
Collaborator

  • It would have to consistently use SameValueZero, even in environments that don't have Map.
  • There would have to be a new major release of Underscore in which we actually decided to use SameValueZero instead of strict equality everywhere. See Change to SameValueZero for comparisons #2453. It is not yet decided whether we will ever do that.
  • It shouldn't do if/else all over the place. Move the supportsMap out of the function and switch over its value only once instead of repeatedly in every loop iteration.

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.

5 participants