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: use subtraction instead of multiple compares in compareUint8Arrays #8

Merged
merged 4 commits into from
Jan 6, 2024

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Jan 5, 2024

This looks to have an almost 20% performance win on my machine, mostly by removing one comparison in the tight loop.

  • Used Math.sign to clamp the outputs so that the tests still pass.
  • Added missing benchmark for compareUint8Arrays

Comparison of benchmark outputs on my machine:
Before: compareUint8Arrays x 479 ops/sec ±0.10% (95 runs sampled)
After: compareUint8Arrays x 574 ops/sec ±3.75% (96 runs sampled)

index.js Outdated
if (a[index] > b[index]) {
return 1;
const diff = a[index] - b[index];
if (diff) {
Copy link
Owner

Choose a reason for hiding this comment

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

Use an explicit check

@sindresorhus
Copy link
Owner

Interesting finding. I would never have guessed that this would be faster. I would have assumed that even two comparisons operators would be faster than a method call.

@hildjj
Copy link
Contributor Author

hildjj commented Jan 6, 2024

The method call only happens when the arrays are different, and the benchmark is rigged so that never happens in the inner loop. A more fair benchmark would probably compare lots of smaller random arrays.

@hildjj
Copy link
Contributor Author

hildjj commented Jan 6, 2024

If we are worried about the overhead of the Math.sign call, you could relax the outputs to be [negative, 0, positive], move the Math.sign call to the tests, change the docs, and this function would still work great as a sort comparison function.

@sindresorhus
Copy link
Owner

If we are worried about the overhead of the Math.sign call, you could relax the outputs to be [negative, 0, positive], move the Math.sign call to the tests, change the docs, and this function would still work great as a sort comparison function.

Nah, it's fine.

@sindresorhus sindresorhus merged commit b196c07 into sindresorhus:main Jan 6, 2024
2 checks passed
@sindresorhus
Copy link
Owner

Thanks :)

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

Successfully merging this pull request may close these issues.

2 participants