optimize Generated{Abstract,Product}Algebra with benchmarking #591
Conversation
| @@ -166,6 +166,7 @@ lazy val algebird = Project( | |||
| base = file("."), | |||
| settings = sharedSettings) | |||
| .settings(noPublishSettings) | |||
| .settings(coverageExcludedPackages := "<empty>;.*\\.benchmark\\..*") | |||
There was a problem hiding this comment.
gotta increase the coverage!
sritchie
force-pushed
the
sritchie/tuple_performance
branch
from
1122d43 to
00a457d
Compare
Current coverage is 81.03% (diff: 100%)@@ develop #591 diff @@
==========================================
Files 122 113 -9
Lines 4694 4903 +209
Methods 4268 4517 +249
Messages 0 0
Branches 387 385 -2
==========================================
+ Hits 3482 3973 +491
+ Misses 1212 930 -282
Partials 0 0
|
| @@ -216,6 +216,7 @@ lazy val algebirdTest = module("test").settings( | |||
| ).dependsOn(algebirdCore) | |||
|
|
|||
| lazy val algebirdBenchmark = module("benchmark").settings(JmhPlugin.projectSettings:_*).settings( | |||
| coverageExcludedPackages := "com\\.twitter\\.algebird\\.benchmark.*", | |||
There was a problem hiding this comment.
this is hurting our coverage!
| * the `sumOption` implementation of the supplied Semigroup[T] | ||
| */ | ||
| def fromSumOption[T](size: Int)(implicit sg: Semigroup[T]) = | ||
| new ArrayBufferedOperation[T, T](1000) with BufferedReduce[T] { |
| * Returns an ArrayBufferedOperation instance that internally uses | ||
| * the `sumOption` implementation of the supplied Semigroup[T] | ||
| */ | ||
| def fromSumOption[T](size: Int)(implicit sg: Semigroup[T]) = |
There was a problem hiding this comment.
can we put a narrow return type on this? Maybe BufferedReduce[T]?
|
@johnynek ah, nice catches. Was letting the coffee get to me. All fixed up. |
|
LGTM Any idea why ignoring Does this now always use sumOption, even when plus would normally be called? I wonder if any |
|
@isnotinvain, my mistake was that I left a value of All this PR does is add a more efficient implementation for anything that explicitly calls If you call Thanks for the LGTM! Tests timed out, so waiting for the build to complete again and then I'll merge and continue with the averaged value implementation. |
|
so, I'm worried about this. On a deep tuple are we exponentially growing buffers? Previously we were not (I don't think). You had a linear number of buffers. Can we stop and document why this is not exponential here? |
|
Sorry, looks like I pulled the trigger too soon. Did this go into the release? |
|
hey, sorry @isnotinvain for not updating the ticket! @johnynek and I talked offline when releasing and decided that this is NOT an issue. Buffers are going to grow linearly with the depth if you have nested tuples, which is fine. If I have a tuple2 of tuple2s, The other tuple 2 will create 2 buffers, one for each inner tuple - each inner tuple will then create 2 buffers as well. I think this is totally fine, and how it has to work. Does that sound right? Glad we thought this through! |
|
Yeah SGTM! |
This PR
Tuple4(seemed like a nice one to pick :)sumOptionimplementations that this benchmark uncovered.Run the benchmark with:
Benchmark Results
Before the changes:
After the changes:
so ~6x improvement for the ProductN semigroups, 1.5x improvement for the tupleN semigroups. AND, more importantly, they're all more efficient than just using
plus.Note that benchmarks only adds longs, so it measures the improvement of
sumOptionon tuples when the tuple elements don't have an efficientsumOptionimplementation. When they do, thesesumOptionimplementations will be that much faster.