Adds a presenting benchmark and optimizes it #427
Conversation
| def size = inArray.size | ||
| private[algebird] def getArray = inArray | ||
| // Need to do underlying array equality sensibly | ||
| override def equals(o: Any) = { |
There was a problem hiding this comment.
can we do:
o match {
case ReadOnlyBoxedArray(that) => java.util.Arrays.equals(inArray, that)
case _ => false
}
|
y'all should consider switching to jmh now that there's decent sbt support. |
|
Will take another look at it when we've the chance, since oscar got the sbt plugin in here, these work reasonably well. |
| def estimatedSize = approximateSize.estimate.toDouble | ||
| def estimatedSize: Double = initialEstimate | ||
|
|
||
| lazy val initialEstimate: Double = { |
|
In #399 (CMS improvements, scheduled to be merged into Algebird v0.10.0) we have a Bytes class that looks quite similar to Would it make sense to collapse these two classes? |
|
+1 to reuse Bytes class. I forgot about that. What do you think @ianoc |
|
No real objection, these are lighter api changes than most -- but alas since DenseHLL is public it will break the binary compat too. we can combine the two classes sure. |
|
I think @johnynek wants to not introduce #399 until 0.10.0 to minimize risk of breaking changes. I am not sure when you want to release 0.10.0, hence I can't really tell whether merging to develop now would be good timing. The answer to the stacking question may similarly depend on the release plan -- would you be ok with postponing this PR until 0.10.0 / until #399 eventual merge date? |
|
I guess 0.10 is coming up. What do you all think? /cc @isnotinvain We will need to republish scalding + summingbird as well. |
|
I think we should merge and go, i guess there was a target date in Feb for 0.10 -- if its not heavily breaking the upgrades down the line shouldn't be too bad? |
|
@johnynek rebased and moved over to the bytes class. -- Also just fixes an issue crept in between when the aggregator PR was written and tests for properties changing on develop. |
| val newContents: Array[Byte] = oldV.array.clone | ||
| val siz = newContents.size | ||
|
|
||
| maxRhow.foreach { |
There was a problem hiding this comment.
should we while loop it here to be consistent? We could get the iterator, and do the hasNext, next unrolling?
Man, we really need some macros for this stuff (glanced at ScalaBlitz, does not seem to be exactly what we want here).
There was a problem hiding this comment.
Yeah sounds good, just running tests on the change locally.
|
minor suggestion about using one more while loop rather than a foreach while we are optimizing. |
Adds a presenting benchmark and optimizes it
Benchmark results :
Old code:
https://gist.github.com/ianoc/980b369dd6d5354ce896
New code:
https://gist.github.com/ianoc/41afcafd7099948dbc84
Speedup: a little over 2.4x average ignoring outliers that would bias towards new code more(so if pathological cases new code still does better).