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 Eq[Vector[A]] instance #880

Merged
merged 2 commits into from
Feb 17, 2016
Merged

Conversation

TomasMikula
Copy link
Contributor

  • Check size equality first.
  • Avoid pattern matching on Vector via x +: xs, which creates a new Vector instance via Vector.tail.
  • Added @tailrec annotation to the loop method.

I used this mini-benchmark to assess the improvement:

import cats.Eq
import cats.std.vector._
import cats.std.int._

val v = (1 to 10000000).toVector
val EqV = Eq[Vector[Int]]

{
  val t0 = System.currentTimeMillis
  EqV.eqv(v, v)
  val t1 = System.currentTimeMillis
  println(s"${t1 - t0}ms")
}

The result is

before refactoring after refactoring
6542ms 161ms

 * check for size equality first;
 * avoid pattern matching on Vector via +:.unapply, which invokes
   Vector.tail and instantiates a new Vector instance.
@codecov-io
Copy link

Current coverage is 89.38%

Merging #880 into master will increase coverage by +0.04% as of bb8181e

@@            master    #880   diff @@
======================================
  Files          169     169       
  Stmts         2337    2336     -1
  Branches        75      76     +1
  Methods          0       0       
======================================
  Hit           2088    2088       
  Partial          0       0       
+ Missed         249     248     -1

Review entire Coverage Diff as of bb8181e

Powered by Codecov. Updated on successful CI builds.

}
loop(x, y)
@tailrec def loop(from: Int): Boolean =
if(from == x.size) true
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@TomasMikula
Copy link
Contributor Author

Travis failure doesn't seem to be related to the commit. On my machine, all tests pass.

@adelbertc
Copy link
Contributor

Restarting build

@TomasMikula
Copy link
Contributor Author

Thanks, now it has passed.

@ceedubs
Copy link
Contributor

ceedubs commented Feb 17, 2016

👍 thanks!

We'll want to make sure that this gets picked up in the #786 effort.

@mpilquist
Copy link
Member

👍

mpilquist added a commit that referenced this pull request Feb 17, 2016
Optimize Eq[Vector[A]] instance
@mpilquist mpilquist merged commit ce94b7f into typelevel:master Feb 17, 2016
@japgolly
Copy link

japgolly commented May 2, 2016

Is there a deliberate reason that a manual loop was used instead of .corresponds here? This should result in the same performance:

def eqv(x: Vector[A], y: Vector[A]): Boolean =
  (x.size == y.size) && x.corresponds(y)(ev.eqv)

@TomasMikula
Copy link
Contributor Author

On my side, no. I was just improving on the pre-existing loop.

@japgolly
Copy link

japgolly commented May 8, 2016

Ah cool. .corresponds is a bit of a hidden gem. Anyway, I'd imagine this should be the slightest, slightest bit faster cos there's one index counter instead of two.

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.

6 participants