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

Speed up equivalence comparisons #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

viluon
Copy link

@viluon viluon commented Mar 25, 2020

This PR improves performance on all the five benchmarks (and hopefully in real life too). It does so simply by taking some shortcuts when the left and right operands of eq, le, and lt point to the same object.

Disregard the above. I need to verify that this change has any (positive) performance impact at all. The previous benchmarks were not representative.

@SquidDev
Copy link
Member

SquidDev commented Apr 7, 2020

On master:

# Run complete. Total time: 00:23:40

PerformanceBenchmark.binarytrees  thrpt   15  3.977 ? 0.137  ops/s
PerformanceBenchmark.fannkuch     thrpt   15  5.974 ? 0.225  ops/s
PerformanceBenchmark.nbody        thrpt   15  0.599 ? 0.024  ops/s
PerformanceBenchmark.nsieve       thrpt   15  0.289 ? 0.018  ops/s

On feature/fast-equiv:

# Run complete. Total time: 00:23:15

Benchmark                          Mode  Cnt  Score   Error  Units
PerformanceBenchmark.binarytrees  thrpt   15  4.051 ? 0.180  ops/s
PerformanceBenchmark.fannkuch     thrpt   15  6.187 ? 0.309  ops/s
PerformanceBenchmark.nbody        thrpt   15  0.718 ? 0.016  ops/s
PerformanceBenchmark.nsieve       thrpt   15  0.300 ? 0.012  ops/s

This does end up being slightly faster. Couple of other changes it may be worth trying if I've a little more time:

  • Reverting the changes to toDouble. Shouldn't really have any impact on performance, but we should see.
  • Only applying this patch in the case of eq.

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.

2 participants