[bug#12506] Type-test-override IndexedSeqOps#foreach
#9820
Conversation
Effectively override `IndexedSeqOps#foreach` to loop over indices instead of using an `Iterator` by adding a type test to `IterableOnceOps#foreach`.
NthPortal
added
performance
|
I'll try to get to benchmarking. unfortunately, my server is hosting increasingly many things, making it increasingly unsuited to benchmarking, but hey—it's probably still fine |
|
I think we have other such "overrides" in place where a real override would be binary incompatible. At least in 2.12 we accumulated a bunch of them over time. |
|
The linked commit from strawman overrides all the similar methods such as I think testing is important because it's easy to accidentally deoptimize on the jvm. |
| } | ||
| case _ => | ||
| val it = iterator | ||
| while(it.hasNext) f(it.next()) |
There was a problem hiding this comment.
I see the inconsistent whitespace was inherited. Dr Odersky just told someone "no spaceless if(" [indirect quote] on Scala 3.
There was a problem hiding this comment.
I'll fix that, and also make the "override" its own method that gets called, for clarity
|
I just ran the following benchmark on my machine: package scala.collection
import java.util.concurrent.TimeUnit
import org.openjdk.jmh.annotations._
import org.openjdk.jmh.infra.Blackhole
import scala.collection.immutable.ArraySeq
@BenchmarkMode(Array(Mode.AverageTime))
@Fork(1)
@Threads(1)
@Warmup(iterations = 5)
@Measurement(iterations = 8)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Benchmark)
class ForEachBenchmark {
@Param(Array("0", "1", "10", "1000", "10000", "100000"))
var size: Int = _
var array: Array[String] = _
var vector: Vector[String] = _
var arraySeq: ArraySeq[String] = _
var arrayBuffer: mutable.ArrayBuffer[String] = _
@Setup(Level.Trial) def initNumbers: Unit = {
array = Array.fill(size)("foo")
vector = Vector.fill(size)("foo")
arraySeq = ArraySeq.fill(size)("foo")
arrayBuffer = mutable.ArrayBuffer.fill(size)("foo")
}
@Benchmark def array_foreach(bh: Blackhole): Unit =
bh.consume(array.foreach(bh.consume))
@Benchmark def vector_foreach(bh: Blackhole): Unit =
bh.consume(vector.foreach(bh.consume))
@Benchmark def arraySeq_foreach(bh: Blackhole): Unit =
bh.consume(arraySeq.foreach(bh.consume))
@Benchmark def arrayBuffer_foreach(bh: Blackhole): Unit =
bh.consume(arrayBuffer.foreach(bh.consume))
}Here are the results: Before: After: Before: After: Before: After (Arrays should not be affected by the PR): Before: After: Not sure if I did something wrong. But there seems to be no impact on the performance. |
|
If there's no difference either way for anything, is it actually worth adding to the code complexity? Nominally it avoids an object creation, which one would think was good, but it looks to me like the object creation is getting optimized out or you'd be able to see the initializer cost. Can you check GC churn before/after to verify? I'd imagine in cases with less optimization, you'd get increased memory usage and initializer costs, but then you'd get a |
|
@WojciechMazur you were the original reporter on Discord, I believe. Could you test this PR with your code, or supply your code in benchmark form? |
|
One potential issue with the benchmark is that it's monomorphic in the collection type. @Benchmark def arraySeq_foreach(bh: Blackhole): Unit =
bh.consume(arraySeq.foreach(bh.consume))In the JVM running the above, the That doesn't explain why the I assume it should be possible to pollute the profile (run |
|
Hey, the original issue was discovered in one of Scala Native old benchmarks, which are pretty deprecated and in some cases not very idiomatic. The gist with benchmark adopted to be run with Jmh can be found in this link. Unfortunately, when testing locally it looks this change would introduce regression, here are my results, the last one refers to a situation when changed change is moved to
I have no idea why these results are so different from the ones I was seeing before. Maybe I have some interference due to some background processes I'm not aware of. |
|
Marking as draft since the prospects of this progressing appear uncertain. @WojciechMazur are you interested in pursuing this further? @NthPortal, you? |
|
@SethTisue I don't particularly intend to pursue this further. if justified by benchmarks, I'd be happy to tweak the code to be more in line with other type-test-overrides, but unless someone else justifies it, it's not clear to me that it's worth working on. happy for someone else to reopen or copy if they think it's worth it |
Effectively override
IndexedSeqOps#foreachto loopover indices instead of using an
Iteratorby adding atype test to
IterableOnceOps#foreach.Fixes scala/bug#12506