Make ListBuffer's iterator fail when the buffer is mutated

[NthPortal]

Make ListBuffer's iterator fail-fast when the buffer is mutated after the iterator's creation.

Fix appending a ListBuffer to itself and subtracting a ListBuffer from itself.

Fix appending a Growable that uses the default addAll implementation to itself.

Fixes scala/bug#3088
Partially addresses scala/bug#12121
This PR is the beginning of a strategy to address scala/bug#12009

[NthPortal]

I think this is definitely a community build candidate; I'm hoping this doesn't break a lot of things (if it does, that probably means a lot of bugs), but I think it's definitely something to watch out for since this will hopefully make it into a 2.13 release

[NthPortal]

I'm getting a failure with the following test, and having trouble diagnosing

scala.tools.nsc.scaladoc.HtmlFactoryTest.(It is not a test it is a sbt.testing.SuiteSelector)

anyone know where to look?

[NthPortal]

benchmarks:

- before
+ after

  Benchmark                            (size)  Mode  Cnt         Score        Error  Units
- ListBufferBenchmark.filterInPlace        10  avgt   40       209.552 ±      3.828  ns/op
+ ListBufferBenchmark.filterInPlace        10  avgt   40       196.638 ±      4.577  ns/op
- ListBufferBenchmark.filterInPlace       100  avgt   40      1819.224 ±     22.649  ns/op
+ ListBufferBenchmark.filterInPlace       100  avgt   40      1756.866 ±     76.259  ns/op
- ListBufferBenchmark.filterInPlace      1000  avgt   40     13115.598 ±   2540.419  ns/op
+ ListBufferBenchmark.filterInPlace      1000  avgt   40     16077.728 ±    292.931  ns/op
- ListBufferBenchmark.filterInPlace     10000  avgt   40     88563.923 ±   1244.332  ns/op
+ ListBufferBenchmark.filterInPlace     10000  avgt   40     92484.924 ±   1050.074  ns/op

- ListBufferBenchmark.flatMapInPlace1      10  avgt   40       512.107 ±      9.471  ns/op
+ ListBufferBenchmark.flatMapInPlace1      10  avgt   40       485.547 ±      8.411  ns/op
- ListBufferBenchmark.flatMapInPlace1     100  avgt   40      3106.082 ±     38.434  ns/op
+ ListBufferBenchmark.flatMapInPlace1     100  avgt   40      3864.339 ±     41.703  ns/op
- ListBufferBenchmark.flatMapInPlace1    1000  avgt   40     40203.206 ±    741.022  ns/op
+ ListBufferBenchmark.flatMapInPlace1    1000  avgt   40     37358.350 ±    610.488  ns/op
- ListBufferBenchmark.flatMapInPlace1   10000  avgt   40    372594.646 ±  11931.681  ns/op
+ ListBufferBenchmark.flatMapInPlace1   10000  avgt   40    320438.086 ±   4169.805  ns/op

- ListBufferBenchmark.insert               10  avgt   40       233.837 ±      2.661  ns/op
+ ListBufferBenchmark.insert               10  avgt   40       208.389 ±     10.885  ns/op
- ListBufferBenchmark.insert              100  avgt   40      4695.472 ±     38.096  ns/op
+ ListBufferBenchmark.insert              100  avgt   40      5547.858 ±     70.386  ns/op
- ListBufferBenchmark.insert             1000  avgt   40    476238.273 ±   2797.168  ns/op
+ ListBufferBenchmark.insert             1000  avgt   40    472445.513 ±   3559.258  ns/op
- ListBufferBenchmark.insert            10000  avgt   40  48167162.543 ± 373388.643  ns/op
+ ListBufferBenchmark.insert            10000  avgt   40  47403816.905 ± 409465.703  ns/op

- ListBufferBenchmark.insertAll            10  avgt   40       313.262 ±      7.922  ns/op
+ ListBufferBenchmark.insertAll            10  avgt   40       283.284 ±      6.188  ns/op
- ListBufferBenchmark.insertAll           100  avgt   40      1435.928 ±     20.963  ns/op
+ ListBufferBenchmark.insertAll           100  avgt   40      2108.278 ±     25.333  ns/op
- ListBufferBenchmark.insertAll          1000  avgt   40     73795.740 ±    626.632  ns/op
+ ListBufferBenchmark.insertAll          1000  avgt   40     73774.687 ±    750.869  ns/op
- ListBufferBenchmark.insertAll         10000  avgt   40   6567366.514 ±  29627.485  ns/op
+ ListBufferBenchmark.insertAll         10000  avgt   40   6443110.161 ±  57711.887  ns/op

- ListBufferBenchmark.iteratorA            10  avgt   40       215.463 ±      3.137  ns/op
+ ListBufferBenchmark.iteratorA            10  avgt   40       201.963 ±      9.603  ns/op
- ListBufferBenchmark.iteratorA           100  avgt   40      1805.295 ±     50.502  ns/op
+ ListBufferBenchmark.iteratorA           100  avgt   40      1563.120 ±     28.937  ns/op
- ListBufferBenchmark.iteratorA          1000  avgt   40     17333.637 ±    183.418  ns/op
+ ListBufferBenchmark.iteratorA          1000  avgt   40     15574.602 ±    298.189  ns/op
- ListBufferBenchmark.iteratorA         10000  avgt   40     82736.341 ±   1317.157  ns/op
+ ListBufferBenchmark.iteratorA         10000  avgt   40     86412.182 ±   1132.029  ns/op

- ListBufferBenchmark.iteratorB            10  avgt   40       243.197 ±      4.185  ns/op
+ ListBufferBenchmark.iteratorB            10  avgt   40       225.307 ±      3.005  ns/op
- ListBufferBenchmark.iteratorB           100  avgt   40      1874.217 ±     32.496  ns/op
+ ListBufferBenchmark.iteratorB           100  avgt   40      1851.259 ±     37.221  ns/op
- ListBufferBenchmark.iteratorB          1000  avgt   40      9206.308 ±    118.528  ns/op
+ ListBufferBenchmark.iteratorB          1000  avgt   40     16971.258 ±    285.469  ns/op
- ListBufferBenchmark.iteratorB         10000  avgt   40    142303.132 ±  27946.817  ns/op
+ ListBufferBenchmark.iteratorB         10000  avgt   40    109750.241 ±   3137.235  ns/op

- ListBufferBenchmark.remove1              10  avgt   40       203.914 ±      3.968  ns/op
+ ListBufferBenchmark.remove1              10  avgt   40       172.079 ±      3.029  ns/op
- ListBufferBenchmark.remove1             100  avgt   40      1327.190 ±     23.052  ns/op
+ ListBufferBenchmark.remove1             100  avgt   40      2122.334 ±     23.976  ns/op
- ListBufferBenchmark.remove1            1000  avgt   40    125240.886 ±   1029.013  ns/op
+ ListBufferBenchmark.remove1            1000  avgt   40    127018.261 ±   1097.686  ns/op
- ListBufferBenchmark.remove1           10000  avgt   40  13557831.094 ± 137941.865  ns/op
+ ListBufferBenchmark.remove1           10000  avgt   40  13902997.558 ± 100696.734  ns/op

- ListBufferBenchmark.remove2              10  avgt   40       189.867 ±      4.883  ns/op
+ ListBufferBenchmark.remove2              10  avgt   40       164.865 ±      2.265  ns/op
- ListBufferBenchmark.remove2             100  avgt   40       754.743 ±      8.621  ns/op
+ ListBufferBenchmark.remove2             100  avgt   40      1489.532 ±     21.509  ns/op
- ListBufferBenchmark.remove2            1000  avgt   40     40683.386 ±    620.152  ns/op
+ ListBufferBenchmark.remove2            1000  avgt   40     40996.992 ±    452.266  ns/op
- ListBufferBenchmark.remove2           10000  avgt   40   3365176.547 ±  51647.179  ns/op
+ ListBufferBenchmark.remove2           10000  avgt   40   3388047.209 ±  40572.239  ns/op

- ListBufferBenchmark.update               10  avgt   40       227.541 ±      1.935  ns/op
+ ListBufferBenchmark.update               10  avgt   40       218.951 ±      8.176  ns/op
- ListBufferBenchmark.update              100  avgt   40      4621.739 ±     30.248  ns/op
+ ListBufferBenchmark.update              100  avgt   40      5525.563 ±     42.342  ns/op
- ListBufferBenchmark.update             1000  avgt   40    480379.841 ±   4256.451  ns/op
+ ListBufferBenchmark.update             1000  avgt   40    490791.195 ±   3660.844  ns/op
- ListBufferBenchmark.update            10000  avgt   40  52779922.013 ± 687751.997  ns/op
+ ListBufferBenchmark.update            10000  avgt   40  53137869.474 ± 541782.921  ns/op

[NthPortal]

I'm not sure what to make of the results. neither is clearly more efficient than the other, and each have some cases with significantly better performance. I can only assume that, because my computer was doing other things in the background (even though I was not actively using it), the results are not perfectly accurate. It would certainly be valuable for someone with a dedicated benchmarkinig machine (i.e. that isn't used for anything else) to repeat the benchmarks (the addition of which is a single commit (b313b88) that can be cherry-picked to 2.13.x). Nevertheless, my initial conclusion is that there is not a significant enough performance hit to the addition of fast-failing to the iterators to be of any concern, if there is any performance cost at all. I am happy to hear others' analysis, and to have myself disproven by further benchmarking.

[Ichoran]

Could you add a singletons benchmark that just does xs.map(x => ListBuffer(x))? When adding new work to a constructor, it's good to test it on small loads. But a single object creation is a bit fast for even JMH to measure accurately.

[NthPortal]

Could you add a singletons benchmark that just does xs.map(x => ListBuffer(x))?

@Ichoran what's xs there? if it's a ListBuffer, map doesn't modify anything, so it doesn't log a mutation and there's not any performance impact of calling it; I wouldn't expect it to make any difference.

[Ichoran]

@NthPortal - xs can be a ListBuffer. It doesn't really matter--but since you're set up to build ListBuffer anyway, why not use it? The point is that the map repeatedly calls the ListBuffer constructor, which now has to build a MutationChecker. So it's sensitive to performance changes.

[NthPortal]

But a single object creation is a bit fast for even JMH to measure accurately.

I have no reason to believe that's correct. JMH generates a whole bunch of logic to do things that are less than the granularity of calls to System.nanoTime() in loops.

If you just want to benchmark the constructor, it doesn't really need sizes particularly (so it might need a separate benchmark) - I don't think there's any need to map it.

[NthPortal]

- before
+ after

  Benchmark                         (size)  Mode  Cnt    Score    Error  Units
- ConstructionBenchmark.listBuffer       0  avgt   40   18.419 ±  0.293  ns/op
+ ConstructionBenchmark.listBuffer       0  avgt   40   21.373 ±  0.177  ns/op
- ConstructionBenchmark.listBuffer       1  avgt   40   25.768 ±  0.456  ns/op
+ ConstructionBenchmark.listBuffer       1  avgt   40   28.390 ±  0.273  ns/op
- ConstructionBenchmark.listBuffer      10  avgt   40  114.286 ±  1.931  ns/op
+ ConstructionBenchmark.listBuffer      10  avgt   40  113.392 ±  1.512  ns/op
- ConstructionBenchmark.listBuffer     100  avgt   40  963.965 ± 17.102  ns/op
+ ConstructionBenchmark.listBuffer     100  avgt   40  931.931 ± 17.372  ns/op

it doesn't seem like a very significant cost to me

quick analysis: there's a very small cost (~2 or 3 ns) to the tracker, but it only really manifests at extremely small sizes. at even a moderately small number of elements (~10), the cost is dwarfed by the cost of adding elements, and lost in the noise.

[SethTisue]

I'm getting a failure with the following test, and having trouble diagnosing

@NthPortal when I run this locally I get:

[error] Uncaught exception when running scala.tools.nsc.scaladoc.HtmlFactoryTest: java.util.ConcurrentModificationException: mutation occurred after iterator creation
[error] stack trace is suppressed; run last scalacheck / Test / testOnly for the full output

which is not a test failure per se, but an exception occurring in the test framework:

[error] Uncaught exception when running scala.tools.nsc.scaladoc.HtmlFactoryTest: java.util.ConcurrentModificationException: mutation occurred after iterator creation
[error] sbt.ForkMain$ForkError: java.util.ConcurrentModificationException: mutation occurred after iterator creation
[error] 	at scala.collection.MutationTracker$Checker.checkMutations(MutationTracker.scala:52)
[error] 	at scala.collection.MutationTracker$Checker.checkMutationsForIterator(MutationTracker.scala:67)
[error] 	at scala.collection.MutationTracker$CheckedIterator.hasNext(MutationTracker.scala:74)
[error] 	at scala.collection.IterableOnceOps.foreach(IterableOnce.scala:553)
[error] 	at scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:551)
[error] 	at scala.collection.AbstractIterable.foreach(Iterable.scala:919)
[error] 	at org.scalacheck.ScalaCheckRunner$$anon$3.execute(ScalaCheckFramework.scala:118)
[error] 	at sbt.ForkMain$Run.lambda$runTest$1(ForkMain.java:304)
[error] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
...

it appears to me that the ListBuffer in question is the one in https://github.com/typelevel/scalacheck/blob/master/src/main/scala/org/scalacheck/Properties.scala — perhaps ScalaCheckRunner and/or Properties ought to be doing some synchronization? I don't see any doc or comments in https://github.com/sbt/test-interface about threading (except one which is about Logger rather than Runner)

[SethTisue]

I think this is definitely a community build candidate

queued: https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/1748/

[NthPortal]

@SethTisue it's not a synchronization/thread-safety issue; rather an issue of using an iterator after mutating the underlying collection, likely in the same thread.

[Ichoran]

Thanks!

The difference of 2-3 ns is about what I'd expect. Object allocations tend to take about that long.

I had forgotten what a huge penalty there was for using from (or the varargs companion method).

val x = new ListBuffer; x += thingy; x is fast, but if nobody does that, then it doesn't matter if it takes ~50% longer (3 object creations instead of 2).

I think if you write the test that way, you'll show a much bigger difference. I don't know whether it's relevant to usage of ListBuffer in the wild--maybe not.

[NthPortal]

I don't know whether it's relevant to usage of ListBuffer in the wild--maybe not.

at the end of the day, people tend to use collections to put things in, so I'm not overly concerned with the empty collection case being slightly more expensive

[Ichoran]

It's not "slightly more" if you do it with new and element addition. You didn't test that. Many lists are small, also.

But small lists tend to be created with :: so maybe it's okay.

[som-snytt]

ListBuffer.empty[Int].tap(_ += 42) just to get ichoran's goat.

[NthPortal]

  Benchmark                         (size)  Mode  Cnt    Score    Error  Units
- ConstructionBenchmark.listBuffer       0  avgt   40    6.125 ±  0.119  ns/op
+ ConstructionBenchmark.listBuffer       0  avgt   40    8.174 ±  0.140  ns/op
- ConstructionBenchmark.listBuffer       1  avgt   40   10.803 ±  0.216  ns/op
+ ConstructionBenchmark.listBuffer       1  avgt   40   14.082 ±  0.238  ns/op
- ConstructionBenchmark.listBuffer      10  avgt   40   60.687 ±  0.634  ns/op
+ ConstructionBenchmark.listBuffer      10  avgt   40   64.039 ±  2.277  ns/op
- ConstructionBenchmark.listBuffer     100  avgt   40  576.759 ± 10.223  ns/op
+ ConstructionBenchmark.listBuffer     100  avgt   40  586.238 ±  9.666  ns/op

[Ichoran]

👍 Thank you! I'm not sure whether this is relevant for anyone's performance, but it's good to have documented so we can think about it more precisely.

[NthPortal]

I was going to post a benchmark showing how whether you use new, from or to has a much more significant effect on performance, but then I got these results:

Benchmark                              (size)  Mode  Cnt    Score    Error  Units
ConstructionBenchmark.listBuffer_from       0  avgt   40   17.970 ±  0.277  ns/op
ConstructionBenchmark.listBuffer_from       1  avgt   40   25.273 ±  0.368  ns/op
ConstructionBenchmark.listBuffer_from      10  avgt   40  119.745 ±  4.541  ns/op
ConstructionBenchmark.listBuffer_from     100  avgt   40  973.068 ± 12.593  ns/op
ConstructionBenchmark.listBuffer_new        0  avgt   40    6.104 ±  0.103  ns/op
ConstructionBenchmark.listBuffer_new        1  avgt   40   10.781 ±  0.223  ns/op
ConstructionBenchmark.listBuffer_new       10  avgt   40   60.370 ±  1.041  ns/op
ConstructionBenchmark.listBuffer_new      100  avgt   40  571.437 ±  9.147  ns/op
ConstructionBenchmark.listBuffer_to         0  avgt   40   14.757 ±  0.242  ns/op
ConstructionBenchmark.listBuffer_to         1  avgt   40   17.769 ±  0.235  ns/op
ConstructionBenchmark.listBuffer_to        10  avgt   40   58.702 ±  0.779  ns/op
ConstructionBenchmark.listBuffer_to       100  avgt   40  484.976 ±  7.548  ns/op

to, which itself calls from (after some indirection that maybe is optimised out by the compiler?), ought to be at least as expensive as the latter, but isn't. so... not sure what to make of that.

edit: personally, I would say the stats on to are the most important, as I think that's what people tend to use most, but idk

[NthPortal]

@SethTisue I've found the issue with that test - it's that it defines properties within another property. because properties are lazy, that the nested ones are created while the original ones are being iterated over and evaluated, which causes problems. I'm not sure if that should be considered a bug in that test, a bug in scalacheck for allowing it, intended behaviour in scalacheck that relied on UB, or something else.

Update: the scalacheck folks said this is not something you're supposed to do

[SethTisue]

community build result: scopt failed (https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/1748/artifact/logs/scopt-build.log), 2 downstream projects didn't run, everything else green

[NthPortal]

thank you for making my life easy Seth 💜

[NthPortal]

Do we think a single project in the community build failing represents an acceptably small fraction of the community relying on UB to move forward with this? or do we want to reject this change? or, as an alternative, we could be conservative and only increment the mutation count for removals, which are always unsafe afaict?

(fwiw, scopt made assumptions about the iterator in their code that were actually incorrect but happened to work)

[som-snytt]

Uninformed Buffering?

[NthPortal]

Undefined Behavior (popularised by C). I suppose in this case "unspecified" might be more accurate? idk

[SethTisue]

anything more than that, Java doesn't support either I don't think

As an only-somewhat-artificial example, suppose every time there is"foo" in a ListBuffer we want to remove it and the item after it.

With the Java-style API with remove you can write (modulo ambiguity in the problem statement about what happens if there are consecutive "foo"s):

val it = xs.iterator()
while (it.hasNext) {
  if (it.next() == "foo") { it.remove(); it.next(); it.remove(); }
}

This is the sort of code CS 101 students can easily write. Whereas with the Scala-style API, solving it efficiently (especially in the absence of efficient indexed access) is a brainteaser. You can do it with filterInPlace by tracking state as you go in a boolean var, but it's a lot more difficult to write (and difficult to read) then the Java-style version.

(But if we want to continue discussing this, I'd suggest a change of venue, either a new ticket or a Discourse thread. For me, it falls in a middle zone of not-exactly-opposed-but-not-actually-excited-about-it-either. Happy to relocate this comment in its entirety to a new thread, if one starts. I agree with Jason that it doesn't affect the outcome of this PR.)

UPDATE: see https://contributors.scala-lang.org/t/design-for-mutating-iterators/4552

[NthPortal]

this fails with a ConcurrentModificationException if it comes from this. might fix in a separate PR. it didn't work before this PR anyway - it failed in an unspecified way (adding/removing the wrong elements) rather than with an exception

[NthPortal]

specifically, the following code

val b = ListBuffer(1, 2, 3)
b.patchInPlace(1, b, 1)

would previously result in ListBuffer(1, 1, 3, 3) when it should result in ListBuffer(1, 1, 2, 3, 3), but now throws an exception

[NthPortal]

I think this is ready for merge?

[NthPortal]

Travis please

[NthPortal]

if there are no objections, I will merge this in a week

[som-snytt]

I am too ashamed to ever attempt to write another unit test.

For the first time in a while, I miss postfixOps in
(factory.newBuilder += 1 += 2 += 3 += 4).result()
which style I am sure to emulate.

[NthPortal]

1 down, many to go