-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Eq[Vector[A]] instance #880
Conversation
* check for size equality first; * avoid pattern matching on Vector via +:.unapply, which invokes Vector.tail and instantiates a new Vector instance.
Current coverage is
|
} | ||
loop(x, y) | ||
@tailrec def loop(from: Int): Boolean = | ||
if(from == x.size) true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit to being completely ignorant when it comes to JVM optimizations. Is it worth storing x.size
as a val
instead of calling it on each iteration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that my benchmarking is any rigorous, but taking the average of 20 runs in a REPL of the above current state of this PR and this new code that doesn't call x.size
repeatedly:
def eqv(x: Vector[A], y: Vector[A]): Boolean = {
@tailrec def loop(to: Int): Boolean =
if(to == -1) true
else ev.eqv(x(to), y(to)) && loop(to - 1)
(x.size == y.size) && loop(x.size - 1)
}
it appears this new version is on average faster by some 21ms (167.95ms vs 146.81ms).
Also, I haven't tried to tweak any optimization flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A added that to this PR.
Travis failure doesn't seem to be related to the commit. On my machine, all tests pass. |
Restarting build |
Thanks, now it has passed. |
👍 thanks! We'll want to make sure that this gets picked up in the #786 effort. |
👍 |
Optimize Eq[Vector[A]] instance
Is there a deliberate reason that a manual loop was used instead of def eqv(x: Vector[A], y: Vector[A]): Boolean =
(x.size == y.size) && x.corresponds(y)(ev.eqv) |
On my side, no. I was just improving on the pre-existing |
Ah cool. |
x +: xs
, which creates a new Vector instance viaVector.tail
.@tailrec
annotation to theloop
method.I used this mini-benchmark to assess the improvement:
The result is