Add sumOption to Short,Int,Long,Float,Double #538
Conversation
|
mima crashed on this. Trying to bump to the latest and see if that helps. |
|
LGTM -- did you manage to get any benchmark/profile to illustrate? |
| } | ||
| override def sum(t: TraversableOnce[Float]): Float = { | ||
| var sum = 0.0f | ||
| t.foreach(sum += _) |
There was a problem hiding this comment.
did we find out in the CMS PR that foreach doesn't compile down to a while?
should this be a while loop? I could imagine dynamic dispatch on a closure costingmore than boxing, but who knows
There was a problem hiding this comment.
Closure usage is far far cheaper than boxing. I guess its to do with inlining or new gen allocations. But boxing will show up orders of magnitude higher than it. But seems reasonable to eliminate it here
There was a problem hiding this comment.
OK thanks, didn't really have an intuition for what the cost of method dispatch vs boxing (object allocation?) is. But yeah, seems like here we might as well use a while anyway.
There was a problem hiding this comment.
The only catch is TraversableOnce has no way to use while without toIterator. In that case, building an iterator can be more expensive that foreach. Consider an array. If you need to write foreach for an array, you can do:
def foreach(f: T => Unit): Unit = {
var idx = 0
while(idx < arr.length) {
f(arr(idx))
idx += 1
}
}Which can be cheaper than wrapping with an Iterator.
I think we should assume foreach is fine until we really want to geek out on different data structure testing.
There was a problem hiding this comment.
that's a good point, didn't realize that TraversableOnce only has a conversion to Iterator.
|
+1, 2 minor comments if you think they are worth addressing (but without benchmarks, not really worth too much thought probably) |
|
+1 |
This can avoid some cases of double boxing. When we use for instance,
Semigroup[T]andThappens to beLong, then we can still box despite the specialized call becauseTwas generic where theSemigroupwas used (I believe that is true).Doing
sumwith plus means, unboxing the accumulator and the next item, adding them, boxing the result and starting again. This gives aboutNboxing operations and2Nunboxing operations forNitems to sum. If we do sum internally, we know the type, so we only have to unbox from theTraverableOnce, which givesNunboxing operations.