From 9f3f4c33d9c8f781a98e81131023da03bee89b11 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Wed, 11 Aug 2021 18:29:39 -0700 Subject: [PATCH 1/4] Update and reorganize benchmarks readme to be less confusing We have exactly one custom runner, so the emphasis that the old structure put on custom runners was misleading. It's better to foreground the normal case. --- test/benchmarks/README.md | 77 ++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/test/benchmarks/README.md b/test/benchmarks/README.md index 45f8e142be9e..71d0462889d4 100644 --- a/test/benchmarks/README.md +++ b/test/benchmarks/README.md @@ -3,50 +3,62 @@ This directory is used by the `bench` subproject of the Scala sbt build. It makes use of the [sbt plugin](https://github.com/ktoso/sbt-jmh) for [JMH](https://openjdk.java.net/projects/code-tools/jmh/). -## Running a benchmark +## About the benchmarks -Benchmarks are built with the bootstrap compiler ("starr") using the library built from the `library` project ("quick"). -If you want to test compiler changes you need to bootstrap with the new compiler. +Benchmarks are built with the reference compiler ("starr") using the library built from the `library` project ("quick"). +If you want to test compiler changes you need to bootstrap a new compiler. -You'll then need to know the fully-qualified name of the benchmark runner class. -The benchmarking classes are organized under `src/main/scala`, +The benchmarking classes are organized under `test/benchmarks/src/main/scala`, in the same package hierarchy as the classes that they test. -Assuming that we're benchmarking `scala.collection.mutable.OpenHashMap`, -the benchmark runner would likely be named `scala.collection.mutable.OpenHashMapRunner`. -Using this example, one would simply run - bench/jmh:runMain scala.collection.mutable.OpenHashMapRunner +The benchmarking classes use the same package hierarchy as the classes that they test +in order to make it easy to expose members of the class under test in package-private scope, +should that be necessary for benchmarking. -in the Scala sbt build. +There are two types of classes in the source directory: +those suffixed `Benchmark`, and a few that are suffixed `Runner`. +(The latter are described below, under "Custom runners".) -The JMH results can be found under `../../target/jmh-results/` (i.e. the main Scala build's `target`, -not the one that contains the benchmark class files). `jmh-results` gets deleted on an sbt `bench/clean`, -so you should copy these files out of `target` if you wish to preserve them. +## Running a normal benchmark -## Creating a benchmark and runner +Use `bench/Jmh/run` and provide the fully qualified name of the benchmark +class: -The benchmarking classes use the same package hierarchy as the classes that they test -in order to make it easy to expose, in package scope, members of the class under test, -should that be necessary for benchmarking. + bench/Jmh/run scala.collection.mutable.ListBufferBenchmark -There are two types of classes in the source directory: -those suffixed `Benchmark` and those suffixed `Runner`. -The former are benchmarks that can be run directly using `bench/jmh:run`; -however, they are normally run from a corresponding class of the latter type, -which is run using `bench/jmh:runMain` (as described above). -This …`Runner` class is useful for setting appropriate JMH command options, +Results are printed to standard output. + +## Custom runners + +Some benchmarks have custom runners. A custom runner +can be useful for setting appropriate JMH command options, and for processing the JMH results into files that can be read by other tools, such as Gnuplot. -The `benchmark.JmhRunner` trait should be woven into any runner class, for the standard behavior that it provides. +Assuming that we're benchmarking `scala.collection.mutable.OpenHashMap`, +the custom runner (if there is one) would likely be named +`scala.collection.mutable.OpenHashMapRunner`. +Using this example, one would run + + bench/Jmh/runMain scala.collection.mutable.OpenHashMapRunner + +in the Scala sbt build. + +Custom runner results are written to `../../target/jmh-results/` (i.e. the main Scala build's `target`, +not the one that contains the benchmark class files). `jmh-results` gets deleted on an sbt `bench/clean`, +so you should copy these files out of `target` if you wish to preserve them. + +If you want to make your own custom runner, extend the `benchmark.JmhRunner` trait, for the standard behavior that it provides. This includes creating output files in a subdirectory of `target/jmh-results` derived from the fully-qualified package name of the `Runner` class. ## Some useful HotSpot options -Adding these to the `jmh:run` or `jmh:runMain` command line may help if you're using the HotSpot (Oracle, OpenJDK) compiler. + +Adding these to the `Jmh/run` or `Jmh/runMain` command line may help if you're using the HotSpot (Oracle, OpenJDK) compiler. They require prefixing with `-jvmArgs`. -See [the Java documentation](https://docs.oracle.com/javase/8/docs/technotes/tools/unix/java.html) for more options. +See [the Java documentation](https://docs.oracle.com/javase/8/docs/technotes/tools/unix/java.html) for more options. ### Viewing JIT compilation events + Adding `-XX:+PrintCompilation` shows when Java methods are being compiled or deoptimized. At the most basic level, these messages will tell you whether the code that you're measuring is still being tuned, @@ -54,16 +66,20 @@ so that you know whether you're running enough warm-up iterations. See [Kris Mok's notes](https://gist.github.com/rednaxelafx/1165804#file-notes-md) to interpret the output in detail. ### Consider GC events + If you're not explicitly performing `System.gc()` calls outside of your benchmarking code, you should add the JVM option `-verbose:gc` to understand the effect that GCs may be having on your tests. ### "Diagnostic" options + These require the `-XX:+UnlockDiagnosticVMOptions` JVM option. #### Viewing inlining events + Add `-XX:+PrintInlining`. #### Viewing the disassembled code + If you're running OpenJDK or Oracle JVM, you may need to install the disassembler library (`hsdis-amd64.so` for the `amd64` architecture). In Debian, this is available in @@ -84,16 +100,16 @@ To show it for _all_ methods, add `-XX:+PrintAssembly`. ### Using JITWatch -[JITWatch](https://github.com/AdoptOpenJDK/jitwatch) is useful to understand how the JVM has JIT compiled +[JITWatch](https://github.com/AdoptOpenJDK/jitwatch) is useful to understand how the JVM has JIT-compiled code. If you install `hsdis`, as described above, machine code disassembly is also created. You can generate the `hotspot.log` file for a benchmark run by adding the [required JVM options](https://github.com/AdoptOpenJDK/jitwatch/wiki/Building-hsdis) -to JMH benchmark execution: +to JMH benchmark execution: ``` -sbt:root> bench/jmh:run scala.collection.mutable.ArrayOpsBenchmark.insertInteger -psize=1000 -f1 -jvmArgs -XX:+UnlockDiagnosticVMOptions -jvmArgs -XX:+TraceClassLoading -jvmArgs -XX:+LogCompilation -jvmArgs -XX:LogFile=target/hotspot.log -jvmArgs -XX:+PrintAssembly +sbt:root> bench/Jmh/run scala.collection.mutable.ArrayOpsBenchmark.insertInteger -psize=1000 -f1 -jvmArgs -XX:+UnlockDiagnosticVMOptions -jvmArgs -XX:+TraceClassLoading -jvmArgs -XX:+LogCompilation -jvmArgs -XX:LogFile=target/hotspot.log -jvmArgs -XX:+PrintAssembly ... [info] Loaded disassembler from /Users/jz/.jabba/jdk/1.8.172/Contents/Home/jre/lib/hsdis-amd64.dylib [info] Decoding compiled method 0x0000000113f60bd0: @@ -114,7 +130,7 @@ sbt:root> bench/jmh:run scala.collection.mutable.ArrayOpsBenchmark.insertInteger JITWatch requires configuration of the class and source path. We generate that with a custom task in our build: ``` -sbt> bench/jmh:jitwatchConfigFile +sbt> bench/Jmh/jitwatchConfigFile ... jmh ... @@ -128,6 +144,7 @@ sbt> ^C Follow instructions in the output above and start gleaning insights! ## Useful reading + * [OpenJDK advice on microbenchmarks](https://wiki.openjdk.java.net/display/HotSpot/MicroBenchmarks) * Brian Goetz's "Java theory and practice" articles: * "[Dynamic compilation and performance measurement](https://www.ibm.com/developerworks/java/library/j-jtp12214/)" From 882b1b02e24af31641ed1926e5b9bd46cc72ccd0 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Wed, 11 Aug 2021 19:50:17 -0700 Subject: [PATCH 2/4] Add ArrayBufferBenchmark Copy-pasted from `ListBufferBenchmark`, but based on the specifics of the code changes in this PR, I added benchmarks for `addAll` and `reverseIterator`, and modified the `insertAll` benchmark so it's measuring `ArrayBuffer`-into-`ArrayBuffer` insertion (rather than `Seq`-into-`ArrayBuffer`). --- .../mutable/ArrayBufferBenchmark.scala | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 test/benchmarks/src/main/scala/scala/collection/mutable/ArrayBufferBenchmark.scala diff --git a/test/benchmarks/src/main/scala/scala/collection/mutable/ArrayBufferBenchmark.scala b/test/benchmarks/src/main/scala/scala/collection/mutable/ArrayBufferBenchmark.scala new file mode 100644 index 000000000000..aafa899e3442 --- /dev/null +++ b/test/benchmarks/src/main/scala/scala/collection/mutable/ArrayBufferBenchmark.scala @@ -0,0 +1,97 @@ +/* + * Scala (https://www.scala-lang.org) + * + * Copyright EPFL and Lightbend, Inc. + * + * Licensed under Apache License 2.0 + * (http://www.apache.org/licenses/LICENSE-2.0). + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +package scala.collection.mutable + +import java.util.concurrent.TimeUnit + +import org.openjdk.jmh.annotations._ +import org.openjdk.jmh.infra._ + +@BenchmarkMode(Array(Mode.AverageTime)) +@Fork(2) +@Threads(1) +@Warmup(iterations = 15) +@Measurement(iterations = 15) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Benchmark) +class ArrayBufferBenchmark { + @Param(Array(/*"0", "1",*/ "10", "100", "1000", "10000")) + var size: Int = _ + + var ref: ArrayBuffer[Int] = _ + + @Setup(Level.Trial) def init: Unit = { + ref = new ArrayBuffer + for(i <- 0 until size) ref += i + } + + @Benchmark def filterInPlace(bh: Blackhole): Unit = { + val b = ref.clone() + b.filterInPlace(_ % 2 == 0) + bh.consume(b) + } + + @Benchmark def update(bh: Blackhole): Unit = { + val b = ref.clone() + var i = 0 + while(i < size) { + b.update(i, -1) + i += 2 + } + bh.consume(b) + } + + @Benchmark def addAll(bh: Blackhole): Unit = { + val b1 = ref.clone() + val b2 = ref.clone() + var i = 0 + b1.addAll(b2) + bh.consume(b1) + } + + @Benchmark def flatMapInPlace1(bh: Blackhole): Unit = { + val b = ref.clone() + val seq = Seq(0,0) + b.flatMapInPlace { _ => seq } + bh.consume(b) + } + + @Benchmark def iteratorA(bh: Blackhole): Unit = { + val b = ref.clone() + var n = 0 + for (x <- b.iterator) n += x + bh.consume(n) + bh.consume(b) + } + + @Benchmark def iteratorB(bh: Blackhole): Unit = { + val b = ref.clone() + bh.consume(b.iterator.toVector) + bh.consume(b) + } + + @Benchmark def reverseIteratorA(bh: Blackhole): Unit = { + val b = ref.clone() + var n = 0 + for (x <- b.reverseIterator) n += x + bh.consume(n) + bh.consume(b) + } + + @Benchmark def reverseIteratorB(bh: Blackhole): Unit = { + val b = ref.clone() + bh.consume(b.reverseIterator.toVector) + bh.consume(b) + } + +} From 7f14a79f260f6cd230174bb2e9b7db52cecadef0 Mon Sep 17 00:00:00 2001 From: NthPortal Date: Mon, 21 Sep 2020 01:22:15 -0400 Subject: [PATCH 3/4] [bug#12121] Add test for inserting an ArrayBuffer into itself --- src/library/scala/collection/mutable/ArrayBuffer.scala | 4 ++++ .../scala/collection/mutable/ArrayBufferTest.scala | 10 +++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/library/scala/collection/mutable/ArrayBuffer.scala b/src/library/scala/collection/mutable/ArrayBuffer.scala index 269d564c4c37..e9c19ff11392 100644 --- a/src/library/scala/collection/mutable/ArrayBuffer.scala +++ b/src/library/scala/collection/mutable/ArrayBuffer.scala @@ -167,6 +167,10 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) size0 = size0 + elemsLength elems match { case elems: ArrayBuffer[_] => + // if `elems eq this`, this works because `elems.array eq this.array`, + // we didn't overwrite the values being inserted after moving them in + // the previous copy a few lines up, and `System.arraycopy` will + // effectively "read" all the values before overwriting any of them. Array.copy(elems.array, 0, array, index, elemsLength) case _ => var i = 0 diff --git a/test/junit/scala/collection/mutable/ArrayBufferTest.scala b/test/junit/scala/collection/mutable/ArrayBufferTest.scala index 8f7ae6fe1fb2..fcdd04cc3875 100644 --- a/test/junit/scala/collection/mutable/ArrayBufferTest.scala +++ b/test/junit/scala/collection/mutable/ArrayBufferTest.scala @@ -4,7 +4,7 @@ import org.junit.Test import org.junit.Assert.{assertEquals, assertTrue} import scala.annotation.nowarn -import scala.tools.testkit.AssertUtil.{assertThrows, fail} +import scala.tools.testkit.AssertUtil.{assertSameElements, assertThrows, fail} import scala.tools.testkit.ReflectUtil.{getMethodAccessible, _} class ArrayBufferTest { @@ -447,4 +447,12 @@ class ArrayBufferTest { assertEquals(32, resizeDown(64, 30)) assertEquals(21, resizeDown(42, 17)) } + + // scala/bug#12121 + @Test + def insertAll_self(): Unit = { + val buf = ArrayBuffer(1, 2, 3) + buf.insertAll(1, buf) + assertSameElements(List(1, 1, 2, 3, 2, 3), buf) + } } From 3e1aad3a6310f3b305fc0c17422d2ae0392742bb Mon Sep 17 00:00:00 2001 From: NthPortal Date: Wed, 2 Sep 2020 14:52:29 -0400 Subject: [PATCH 4/4] [bug#12009] Make ArrayBuffer's iterator fail-fast Make `ArrayBuffer`'s iterator fail-fast when the buffer is mutated after the iterator's creation. --- project/MimaFilters.scala | 25 +++- src/library/scala/collection/IndexedSeq.scala | 10 +- .../scala/collection/IndexedSeqView.scala | 26 ++-- .../collection/mutable/ArrayBuffer.scala | 100 +++++++++++---- .../mutable/CheckedIndexedSeqView.scala | 117 ++++++++++++++++++ .../mutable/MutationTrackingTest.scala | 66 ++++++++-- 6 files changed, 281 insertions(+), 63 deletions(-) create mode 100644 src/library/scala/collection/mutable/CheckedIndexedSeqView.scala diff --git a/project/MimaFilters.scala b/project/MimaFilters.scala index 4b13a302e298..fa4443c37009 100644 --- a/project/MimaFilters.scala +++ b/project/MimaFilters.scala @@ -31,6 +31,29 @@ object MimaFilters extends AutoPlugin { ProblemFilters.exclude[DirectMissingMethodProblem]("scala.Predef#ArrayCharSequence.isEmpty"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.ArrayCharSequence.isEmpty"), + // #9425 Node is private[collection] + ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.HashMap#Node.foreachEntry"), + + // Fixes for scala/bug#12009 + ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.ArrayBufferView.this"), // private[mutable] + ProblemFilters.exclude[FinalClassProblem]("scala.collection.IndexedSeqView$IndexedSeqViewIterator"), // private[collection] + ProblemFilters.exclude[FinalClassProblem]("scala.collection.IndexedSeqView$IndexedSeqViewReverseIterator"), // private[collection] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$CheckedIterator"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$CheckedReverseIterator"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$Id"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$Appended"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$Prepended"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$Concat"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$Take"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$TakeRight"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$Drop"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$DropRight"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem](s"scala.collection.mutable.CheckedIndexedSeqView$$Map"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$Reverse"), // private[mutable] + ProblemFilters.exclude[MissingClassProblem]("scala.collection.mutable.CheckedIndexedSeqView$Slice"), // private[mutable] + // #8835 ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.reflect.runtime.SynchronizedOps#SynchronizedBaseTypeSeq.scala$reflect$runtime$SynchronizedOps$SynchronizedBaseTypeSeq$$super$maxDepthOfElems"), @@ -42,7 +65,7 @@ object MimaFilters extends AutoPlugin { ProblemFilters.exclude[IncompatibleMethTypeProblem]("scala.reflect.io.FileZipArchive#LeakyEntry.this"), ProblemFilters.exclude[MissingClassProblem]("scala.reflect.io.FileZipArchive$zipFilePool$"), - ) + ) override val buildSettings = Seq( mimaFailOnNoPrevious := false, // we opt everything out, knowing we only check library/reflect diff --git a/src/library/scala/collection/IndexedSeq.scala b/src/library/scala/collection/IndexedSeq.scala index 18b66b710b07..65a30efe4030 100644 --- a/src/library/scala/collection/IndexedSeq.scala +++ b/src/library/scala/collection/IndexedSeq.scala @@ -47,15 +47,7 @@ trait IndexedSeqOps[+A, +CC[_], +C] extends Any with SeqOps[A, CC, C] { self => s.asInstanceOf[S with EfficientSplit] } - override def reverseIterator: Iterator[A] = new AbstractIterator[A] { - private[this] var i = self.length - def hasNext: Boolean = 0 < i - def next(): A = - if (0 < i) { - i -= 1 - self(i) - } else Iterator.empty.next() - } + override def reverseIterator: Iterator[A] = view.reverseIterator override def foldRight[B](z: B)(op: (A, B) => B): B = { val it = reverseIterator diff --git a/src/library/scala/collection/IndexedSeqView.scala b/src/library/scala/collection/IndexedSeqView.scala index a1b3d4d5e32b..692486b1e088 100644 --- a/src/library/scala/collection/IndexedSeqView.scala +++ b/src/library/scala/collection/IndexedSeqView.scala @@ -49,14 +49,15 @@ trait IndexedSeqView[+A] extends IndexedSeqOps[A, View, View[A]] with SeqView[A] object IndexedSeqView { @SerialVersionUID(3L) - private final class IndexedSeqViewIterator[A](self: IndexedSeqView[A]) extends AbstractIterator[A] with Serializable { + private[collection] class IndexedSeqViewIterator[A](self: IndexedSeqView[A]) extends AbstractIterator[A] with Serializable { private[this] var current = 0 - private[this] var remainder = self.size + private[this] var remainder = self.length override def knownSize: Int = remainder - def hasNext = remainder > 0 + @inline private[this] def _hasNext: Boolean = remainder > 0 + def hasNext: Boolean = _hasNext def next(): A = - if (hasNext) { - val r = self.apply(current) + if (_hasNext) { + val r = self(current) current += 1 remainder -= 1 r @@ -82,18 +83,18 @@ object IndexedSeqView { } } @SerialVersionUID(3L) - private final class IndexedSeqViewReverseIterator[A](self: IndexedSeqView[A]) extends AbstractIterator[A] with Serializable { - private[this] var pos = self.size - 1 - private[this] var remainder = self.size - def hasNext: Boolean = remainder > 0 + private[collection] class IndexedSeqViewReverseIterator[A](self: IndexedSeqView[A]) extends AbstractIterator[A] with Serializable { + private[this] var pos = self.length - 1 + private[this] var remainder = self.length + @inline private[this] def _hasNext: Boolean = remainder > 0 + def hasNext: Boolean = _hasNext def next(): A = - if (pos < 0) throw new NoSuchElementException - else { + if (_hasNext) { val r = self(pos) pos -= 1 remainder -= 1 r - } + } else Iterator.empty.next() override def drop(n: Int): Iterator[A] = { if (n > 0) { @@ -103,7 +104,6 @@ object IndexedSeqView { this } - override def sliceIterator(from: Int, until: Int): Iterator[A] = { val startCutoff = pos val untilCutoff = startCutoff - remainder + 1 diff --git a/src/library/scala/collection/mutable/ArrayBuffer.scala b/src/library/scala/collection/mutable/ArrayBuffer.scala index e9c19ff11392..e60f50587fa9 100644 --- a/src/library/scala/collection/mutable/ArrayBuffer.scala +++ b/src/library/scala/collection/mutable/ArrayBuffer.scala @@ -39,6 +39,7 @@ import scala.util.chaining._ * @define mayNotTerminateInf * @define willNotTerminateInf */ +@SerialVersionUID(-1582447879429021880L) class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) extends AbstractBuffer[A] with IndexedBuffer[A] @@ -51,6 +52,8 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) def this(initialSize: Int) = this(new Array[AnyRef](initialSize max 1), 0) + @transient private[this] var mutationCount: Int = 0 + protected[collection] var array: Array[AnyRef] = initialElements protected var size0 = initialSize @@ -62,14 +65,17 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) override def knownSize: Int = super[IndexedSeqOps].knownSize /** Ensure that the internal array has at least `n` cells. */ - protected def ensureSize(n: Int): Unit = + protected def ensureSize(n: Int): Unit = { + mutationCount += 1 array = ArrayBuffer.ensureSize(array, size0, n) + } def sizeHint(size: Int): Unit = if(size > length && size >= 1) ensureSize(size) /** Reduce length to `n`, nulling out all dropped elements */ private def reduceToSize(n: Int): Unit = { + mutationCount += 1 Arrays.fill(array, n, size0, null) size0 = n } @@ -79,7 +85,10 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) * which may replace the array by a shorter one. * This allows releasing some unused memory. */ - def trimToSize(): Unit = resize(length) + def trimToSize(): Unit = { + mutationCount += 1 + resize(length) + } /** Trims the `array` buffer size down to either a power of 2 * or Int.MaxValue while keeping first `requiredLength` elements. @@ -99,12 +108,13 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) def update(@deprecatedName("n", "2.13.0") index: Int, elem: A): Unit = { checkWithinBounds(index, index + 1) + mutationCount += 1 array(index) = elem.asInstanceOf[AnyRef] } def length = size0 - override def view: ArrayBufferView[A] = new ArrayBufferView(array, size0) + override def view: ArrayBufferView[A] = new ArrayBufferView(array, size0, () => mutationCount) override def iterableFactory: SeqFactory[ArrayBuffer] = ArrayBuffer @@ -136,9 +146,12 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) override def addAll(elems: IterableOnce[A]): this.type = { elems match { case elems: ArrayBuffer[_] => - ensureSize(length + elems.length) - Array.copy(elems.array, 0, array, length, elems.length) - size0 = length + elems.length + val elemsLength = elems.size0 + if (elemsLength > 0) { + ensureSize(length + elemsLength) + Array.copy(elems.array, 0, array, length, elemsLength) + size0 = length + elemsLength + } case _ => super.addAll(elems) } this @@ -162,23 +175,25 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) elems match { case elems: collection.Iterable[A] => val elemsLength = elems.size - ensureSize(length + elemsLength) - Array.copy(array, index, array, index + elemsLength, size0 - index) - size0 = size0 + elemsLength - elems match { - case elems: ArrayBuffer[_] => - // if `elems eq this`, this works because `elems.array eq this.array`, - // we didn't overwrite the values being inserted after moving them in - // the previous copy a few lines up, and `System.arraycopy` will - // effectively "read" all the values before overwriting any of them. - Array.copy(elems.array, 0, array, index, elemsLength) - case _ => - var i = 0 - val it = elems.iterator - while (i < elemsLength) { - this(index + i) = it.next() - i += 1 - } + if (elemsLength > 0) { + ensureSize(length + elemsLength) + Array.copy(array, index, array, index + elemsLength, size0 - index) + size0 = size0 + elemsLength + elems match { + case elems: ArrayBuffer[_] => + // if `elems eq this`, this works because `elems.array eq this.array`, + // we didn't overwrite the values being inserted after moving them in + // the previous copy a few lines up, and `System.arraycopy` will + // effectively "read" all the values before overwriting any of them. + Array.copy(elems.array, 0, array, index, elemsLength) + case _ => + var i = 0 + val it = elems.iterator + while (i < elemsLength) { + this(index + i) = it.next() + i += 1 + } + } } case _ => insertAll(index, ArrayBuffer.from(elems)) @@ -234,7 +249,10 @@ class ArrayBuffer[A] private (initialElements: Array[AnyRef], initialSize: Int) * @return modified input $coll sorted according to the ordering `ord`. */ override def sortInPlace[B >: A]()(implicit ord: Ordering[B]): this.type = { - if (length > 1) scala.util.Sorting.stableSort(array.asInstanceOf[Array[B]], 0, length) + if (length > 1) { + mutationCount += 1 + scala.util.Sorting.stableSort(array.asInstanceOf[Array[B]], 0, length) + } this } } @@ -299,8 +317,36 @@ object ArrayBuffer extends StrictOptimizedSeqFactory[ArrayBuffer] { } } -final class ArrayBufferView[A](val array: Array[AnyRef], val length: Int) extends AbstractIndexedSeqView[A] { - @throws[ArrayIndexOutOfBoundsException] - def apply(n: Int) = if (n < length) array(n).asInstanceOf[A] else throw new IndexOutOfBoundsException(s"$n is out of bounds (min 0, max ${length - 1})") +final class ArrayBufferView[A] private[mutable](val array: Array[AnyRef], val length: Int, mutationCount: () => Int) + extends AbstractIndexedSeqView[A] { + @deprecated("never intended to be public; call ArrayBuffer#view instead", since = "2.13.6") + def this(array: Array[AnyRef], length: Int) = { + // this won't actually track mutation, but it would be a pain to have the implementation + // check if we have a method to get the current mutation count or not on every method and + // change what it does based on that. hopefully no one ever calls this. + this(array, length, () => 0) + } + + @throws[IndexOutOfBoundsException] + def apply(n: Int): A = if (n < length) array(n).asInstanceOf[A] else throw new IndexOutOfBoundsException(s"$n is out of bounds (min 0, max ${length - 1})") override protected[this] def className = "ArrayBufferView" + + // we could inherit all these from `CheckedIndexedSeqView`, except this class is public + override def iterator: Iterator[A] = new CheckedIndexedSeqView.CheckedIterator(this, mutationCount()) + override def reverseIterator: Iterator[A] = new CheckedIndexedSeqView.CheckedReverseIterator(this, mutationCount()) + + override def appended[B >: A](elem: B): IndexedSeqView[B] = new CheckedIndexedSeqView.Appended(this, elem)(mutationCount) + override def prepended[B >: A](elem: B): IndexedSeqView[B] = new CheckedIndexedSeqView.Prepended(elem, this)(mutationCount) + override def take(n: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.Take(this, n)(mutationCount) + override def takeRight(n: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.TakeRight(this, n)(mutationCount) + override def drop(n: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.Drop(this, n)(mutationCount) + override def dropRight(n: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.DropRight(this, n)(mutationCount) + override def map[B](f: A => B): IndexedSeqView[B] = new CheckedIndexedSeqView.Map(this, f)(mutationCount) + override def reverse: IndexedSeqView[A] = new CheckedIndexedSeqView.Reverse(this)(mutationCount) + override def slice(from: Int, until: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.Slice(this, from, until)(mutationCount) + override def tapEach[U](f: A => U): IndexedSeqView[A] = new CheckedIndexedSeqView.Map(this, { (a: A) => f(a); a})(mutationCount) + + override def concat[B >: A](suffix: IndexedSeqView.SomeIndexedSeqOps[B]): IndexedSeqView[B] = new CheckedIndexedSeqView.Concat(this, suffix)(mutationCount) + override def appendedAll[B >: A](suffix: IndexedSeqView.SomeIndexedSeqOps[B]): IndexedSeqView[B] = new CheckedIndexedSeqView.Concat(this, suffix)(mutationCount) + override def prependedAll[B >: A](prefix: IndexedSeqView.SomeIndexedSeqOps[B]): IndexedSeqView[B] = new CheckedIndexedSeqView.Concat(prefix, this)(mutationCount) } diff --git a/src/library/scala/collection/mutable/CheckedIndexedSeqView.scala b/src/library/scala/collection/mutable/CheckedIndexedSeqView.scala new file mode 100644 index 000000000000..b9598904375d --- /dev/null +++ b/src/library/scala/collection/mutable/CheckedIndexedSeqView.scala @@ -0,0 +1,117 @@ +/* + * Scala (https://www.scala-lang.org) + * + * Copyright EPFL and Lightbend, Inc. + * + * Licensed under Apache License 2.0 + * (http://www.apache.org/licenses/LICENSE-2.0). + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +package scala +package collection +package mutable + +private[mutable] trait CheckedIndexedSeqView[+A] extends IndexedSeqView[A] { + protected val mutationCount: () => Int + + override def iterator: Iterator[A] = new CheckedIndexedSeqView.CheckedIterator(this, mutationCount()) + override def reverseIterator: Iterator[A] = new CheckedIndexedSeqView.CheckedReverseIterator(this, mutationCount()) + + override def appended[B >: A](elem: B): IndexedSeqView[B] = new CheckedIndexedSeqView.Appended(this, elem)(mutationCount) + override def prepended[B >: A](elem: B): IndexedSeqView[B] = new CheckedIndexedSeqView.Prepended(elem, this)(mutationCount) + override def take(n: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.Take(this, n)(mutationCount) + override def takeRight(n: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.TakeRight(this, n)(mutationCount) + override def drop(n: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.Drop(this, n)(mutationCount) + override def dropRight(n: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.DropRight(this, n)(mutationCount) + override def map[B](f: A => B): IndexedSeqView[B] = new CheckedIndexedSeqView.Map(this, f)(mutationCount) + override def reverse: IndexedSeqView[A] = new CheckedIndexedSeqView.Reverse(this)(mutationCount) + override def slice(from: Int, until: Int): IndexedSeqView[A] = new CheckedIndexedSeqView.Slice(this, from, until)(mutationCount) + override def tapEach[U](f: A => U): IndexedSeqView[A] = new CheckedIndexedSeqView.Map(this, { (a: A) => f(a); a})(mutationCount) + + override def concat[B >: A](suffix: IndexedSeqView.SomeIndexedSeqOps[B]): IndexedSeqView[B] = new CheckedIndexedSeqView.Concat(this, suffix)(mutationCount) + override def appendedAll[B >: A](suffix: IndexedSeqView.SomeIndexedSeqOps[B]): IndexedSeqView[B] = new CheckedIndexedSeqView.Concat(this, suffix)(mutationCount) + override def prependedAll[B >: A](prefix: IndexedSeqView.SomeIndexedSeqOps[B]): IndexedSeqView[B] = new CheckedIndexedSeqView.Concat(prefix, this)(mutationCount) +} + +private[mutable] object CheckedIndexedSeqView { + import IndexedSeqView.SomeIndexedSeqOps + + @SerialVersionUID(3L) + private[mutable] class CheckedIterator[A](self: IndexedSeqView[A], mutationCount: => Int) + extends IndexedSeqView.IndexedSeqViewIterator[A](self) { + private[this] val expectedCount = mutationCount + override def hasNext: Boolean = { + MutationTracker.checkMutationsForIteration(expectedCount, mutationCount) + super.hasNext + } + } + + @SerialVersionUID(3L) + private[mutable] class CheckedReverseIterator[A](self: IndexedSeqView[A], mutationCount: => Int) + extends IndexedSeqView.IndexedSeqViewReverseIterator[A](self) { + private[this] val expectedCount = mutationCount + override def hasNext: Boolean = { + MutationTracker.checkMutationsForIteration(expectedCount, mutationCount) + super.hasNext + } + } + + @SerialVersionUID(3L) + class Id[+A](underlying: SomeIndexedSeqOps[A])(protected val mutationCount: () => Int) + extends IndexedSeqView.Id(underlying) with CheckedIndexedSeqView[A] + + @SerialVersionUID(3L) + class Appended[+A](underlying: SomeIndexedSeqOps[A], elem: A)(protected val mutationCount: () => Int) + extends IndexedSeqView.Appended(underlying, elem) with CheckedIndexedSeqView[A] + + @SerialVersionUID(3L) + class Prepended[+A](elem: A, underlying: SomeIndexedSeqOps[A])(protected val mutationCount: () => Int) + extends IndexedSeqView.Prepended(elem, underlying) with CheckedIndexedSeqView[A] + + @SerialVersionUID(3L) + class Concat[A](prefix: SomeIndexedSeqOps[A], suffix: SomeIndexedSeqOps[A])(protected val mutationCount: () => Int) + extends IndexedSeqView.Concat[A](prefix, suffix) with CheckedIndexedSeqView[A] + + @SerialVersionUID(3L) + class Take[A](underlying: SomeIndexedSeqOps[A], n: Int)(protected val mutationCount: () => Int) + extends IndexedSeqView.Take(underlying, n) with CheckedIndexedSeqView[A] + + @SerialVersionUID(3L) + class TakeRight[A](underlying: SomeIndexedSeqOps[A], n: Int)(protected val mutationCount: () => Int) + extends IndexedSeqView.TakeRight(underlying, n) with CheckedIndexedSeqView[A] + + @SerialVersionUID(3L) + class Drop[A](underlying: SomeIndexedSeqOps[A], n: Int)(protected val mutationCount: () => Int) + extends IndexedSeqView.Drop[A](underlying, n) with CheckedIndexedSeqView[A] + + @SerialVersionUID(3L) + class DropRight[A](underlying: SomeIndexedSeqOps[A], n: Int)(protected val mutationCount: () => Int) + extends IndexedSeqView.DropRight[A](underlying, n) with CheckedIndexedSeqView[A] + + @SerialVersionUID(3L) + class Map[A, B](underlying: SomeIndexedSeqOps[A], f: A => B)(protected val mutationCount: () => Int) + extends IndexedSeqView.Map(underlying, f) with CheckedIndexedSeqView[B] + + @SerialVersionUID(3L) + class Reverse[A](underlying: SomeIndexedSeqOps[A])(protected val mutationCount: () => Int) + extends IndexedSeqView.Reverse[A](underlying) with CheckedIndexedSeqView[A] { + override def reverse: IndexedSeqView[A] = underlying match { + case x: IndexedSeqView[A] => x + case _ => super.reverse + } + } + + @SerialVersionUID(3L) + class Slice[A](underlying: SomeIndexedSeqOps[A], from: Int, until: Int)(protected val mutationCount: () => Int) + extends AbstractIndexedSeqView[A] with CheckedIndexedSeqView[A] { + protected val lo = from max 0 + protected val hi = (until max 0) min underlying.length + protected val len = (hi - lo) max 0 + @throws[IndexOutOfBoundsException] + def apply(i: Int): A = underlying(lo + i) + def length: Int = len + } +} diff --git a/test/junit/scala/collection/mutable/MutationTrackingTest.scala b/test/junit/scala/collection/mutable/MutationTrackingTest.scala index 9ff9511320e3..c5a03270f01a 100644 --- a/test/junit/scala/collection/mutable/MutationTrackingTest.scala +++ b/test/junit/scala/collection/mutable/MutationTrackingTest.scala @@ -18,34 +18,40 @@ import java.util.ConcurrentModificationException import org.junit.Test import scala.annotation.nowarn +import scala.annotation.unchecked.{uncheckedVariance => uV} import scala.tools.testkit.AssertUtil.assertThrows abstract class MutationTrackingTest[+C <: Iterable[_]](factory: Factory[Int, C]) { - private def runOp(op: C => Any, viewOrIterator: C => IterableOnceOps[_, AnyConstr, _]): Unit = { - val coll = (factory.newBuilder += 1 += 2 += 3 += 4).result() + private[this] type VoI = C => IterableOnceOps[_, AnyConstr, _] + // if you do bad things with this by returning a different builder, it WILL bite you + protected[this] type BuildSequence = Builder[Int, C @uV] => Builder[Int, C @uV] + protected[this] val defaultBuildSequence: BuildSequence = _ += 1 += 2 += 3 += 4 + + private[this] def runOp(op: C => Any, bs: BuildSequence, viewOrIterator: VoI): Unit = { + val coll = bs(factory.newBuilder).result() val it = viewOrIterator(coll) op(coll) it.foreach(_ => ()) } - private def runOpMaybeThrowing(op: C => Any, - throws: Boolean, - viewOrIterator: C => IterableOnceOps[_, AnyConstr, _]): Unit = { - if (throws) assertThrows[ConcurrentModificationException](runOp(op, viewOrIterator), _ contains "iteration") - else runOp(op, viewOrIterator) + private[this] def runOpMaybeThrowing(op: C => Any, bs: BuildSequence, throws: Boolean, viewOrIterator: VoI): Unit = { + if (throws) assertThrows[ConcurrentModificationException](runOp(op, bs, viewOrIterator), _ contains "iteration") + else runOp(op, bs, viewOrIterator) } - private def runOpForViewAndIterator(op: C => Any, throws: Boolean): Unit = { - runOp(op, _.view) // never throws - runOpMaybeThrowing(op, throws, _.iterator) - runOpMaybeThrowing(op, throws, _.view.iterator) + private[this] def runOpForViewAndIterator(op: C => Any, bs: BuildSequence, throws: Boolean): Unit = { + runOp(op, bs, _.view) // never throws + runOpMaybeThrowing(op, bs, throws, _.iterator) + runOpMaybeThrowing(op, bs, throws, _.view.iterator) } /** Checks that no exception is thrown by an operation. */ - def checkFine(op: C => Any): Unit = runOpForViewAndIterator(op, throws = false) + protected[this] def checkFine(op: C => Any, buildSequence: BuildSequence = defaultBuildSequence): Unit = + runOpForViewAndIterator(op, buildSequence, throws = false) /** Checks that an exception is thrown by an operation. */ - def checkThrows(op: C => Any): Unit = runOpForViewAndIterator(op, throws = true) + protected[this] def checkThrows(op: C => Any, buildSequence: BuildSequence = defaultBuildSequence): Unit = + runOpForViewAndIterator(op, buildSequence, throws = true) @Test def nop(): Unit = checkFine { _ => () } @@ -94,6 +100,29 @@ object MutationTrackingTest { def transform(): Unit = checkThrows { _.transform(_ + 1) } } + trait IndexedSeqTest { self: MutationTrackingTest[IndexedSeq[Int]] => + @Test + def mapInPlace(): Unit = checkThrows { _.mapInPlace(_ + 1) } + + @Test + def sortInPlace(): Unit = { + checkThrows { _.sortInPlace() } + checkFine (_.sortInPlace(), _ += 1) + } + + @Test + def sortInPlaceWith(): Unit = { + checkThrows { _.sortInPlaceWith(_ > _) } + checkFine (_.sortInPlaceWith(_ > _), _ += 1) + } + + @Test + def sortInPlaceBy(): Unit = { + checkThrows { _.sortInPlaceBy(_ * -1) } + checkFine (_.sortInPlaceBy(_ * -1), _ += 1) + } + } + trait BufferTest extends GrowableTest with ShrinkableTest with SeqTest { self: MutationTrackingTest[Buffer[Int]] => @Test def insert(): Unit = checkThrows { _.insert(0, 5) } @@ -210,4 +239,15 @@ package MutationTrackingTestImpl { @Test def filterInPlace(): Unit = checkThrows { _.filterInPlace(_ => true) } } + + class ArrayBufferTest extends MutationTrackingTest(ArrayBuffer) with BufferTest with IndexedSeqTest { + @Test + def clearAndShrink(): Unit = checkThrows { _ clearAndShrink 2 } + + @Test + def trimToSize(): Unit = checkThrows { _.trimToSize() } + + @Test + def sizeHint(): Unit = checkThrows { _ sizeHint 16 } + } }