SI-9072 Vector ++ concatenation of parallel collection cause inconsisten... by mzitnik · Pull Request #4259 · scala/scala

mzitnik · 2015-01-18T16:37:47Z

...t results

…tent results

Ichoran · 2015-01-18T23:26:28Z

LGTM but we should get the IDE build error under control before merging. (Not specific to this patch; everything's been failing on it lately.)

retronym · 2015-01-20T00:44:03Z

Could you please expand the commit comment to briefly describe what was wrong with the code before? (e.g: "Should not side effect within a potentially parallel foreach call.")

I'm auditing the rest of the collections for the same mistake.

So far I've found:

// /Users/jason/code/scala2/src/library/scala/collection/TraversableViewLike.scala:184
  trait Appended[B >: A] extends Transformed[B] {
    protected[this] val rest: GenTraversable[B]
    def foreach[U](f: B => U) {
      self foreach f
      rest foreach f
    }
    final override protected[this] def viewIdentifier = "A"
  }

// /Users/jason/code/scala2/src/library/scala/collection/TraversableViewLike.scala:139
trait Forced[B] extends Transformed[B] {
    protected[this] val forced: GenSeq[B]
    def foreach[U](f: B => U) = forced foreach f
    final override protected[this] def viewIdentifier = "C"
  }

Ichoran · 2015-01-20T02:26:33Z

@retronym - Manual auditing sounds tedious. Maybe you're not doing it manually; e.g. you could deprecate foreach on GenTraversableLike and the other Gen-children; then the compiler should catch all uses. Anything outside collection.parallel is suspect.

retronym · 2015-01-20T05:10:58Z

Excellent suggestion!

I added a custom warning to flag any use of a higher order GenXxx method from scala.collection._ (other than in .parallel). Results:

diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
index d2931ff..2df03e2 100644
--- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -1291,6 +1291,13 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
       if (sym.isDeprecated && !currentOwner.ownerChain.exists(x => x.isDeprecated || x.hasBridgeAnnotation))
         currentRun.reporting.deprecationWarning(pos, sym)

+      val isHigherOrderGenTraversableMethod = {
+        sym.hasTransOwner(CollectionPackage.moduleClass) && mexists(sym.paramss)(param => definitions.isFunctionType(param.info) && !param.isImplicit) && sym.owner.name.toString.matches("Gen[A-Z].*")
+      }
+      if (isHigherOrderGenTraversableMethod && currentClass.hasTransOwner(CollectionPackage.moduleClass) && !currentClass.hasTransOwner(CollectionParallelPackage.moduleClass) && !currentClass.name.toString.matches("Gen[A-Z].*")) {
+        reporter.warning(pos, "Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel")
+      }
+
       // Similar to deprecation: check if the symbol is marked with @migration
       // indicating it has changed semantics between versions.
       if (sym.hasMigrationAnnotation && settings.Xmigration.value != NoScalaVersion) {
diff --git a/src/reflect/scala/reflect/internal/Definitions.scala b/src/reflect/scala/reflect/internal/Definitions.scala
index 9f4ec3e..7e51f09 100644
--- a/src/reflect/scala/reflect/internal/Definitions.scala
+++ b/src/reflect/scala/reflect/internal/Definitions.scala
@@ -419,6 +419,8 @@ trait Definitions extends api.StandardDefinitions {
     def elementType(container: Symbol, tp: Type): Type       = elementExtract(container, tp)

     // collections classes
+    lazy val CollectionPackage  = getPackage("scala.collection")
+    lazy val CollectionParallelPackage  = getPackage("scala.collection.parallel")
     lazy val ConsClass          = requiredClass[scala.collection.immutable.::[_]]
     lazy val IteratorClass      = requiredClass[scala.collection.Iterator[_]]
     lazy val IterableClass      = requiredClass[scala.collection.Iterable[_]]

Results:

qscalac -d /tmp $(find src/library/scala/collection -name '*.scala')
src/library/scala/collection/generic/GenericTraversableTemplate.scala:219: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
      for (x <- asTraversable(xs)) {
                             ^
src/library/scala/collection/generic/SeqForwarder.scala:38: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  override def prefixLength(p: A => Boolean) = underlying prefixLength p
                                                          ^
src/library/scala/collection/generic/SeqForwarder.scala:39: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  override def indexWhere(p: A => Boolean): Int = underlying indexWhere p
                                                             ^
src/library/scala/collection/generic/SeqForwarder.scala:45: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  override def lastIndexWhere(p: A => Boolean): Int = underlying lastIndexWhere p
                                                                 ^
src/library/scala/collection/immutable/List.scala:327: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
          f(rest.head).foreach{ b =>
                       ^
src/library/scala/collection/immutable/Vector.scala:223: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
            for (x <- again) v = v :+ x
                      ^
src/library/scala/collection/IndexedSeqOptimized.scala:50: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
    val i = prefixLength(!p(_))
            ^
src/library/scala/collection/IndexedSeqOptimized.scala:153: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  def takeWhile(p: A => Boolean): Repr = take(prefixLength(p))
                                              ^
src/library/scala/collection/IndexedSeqOptimized.scala:156: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  def dropWhile(p: A => Boolean): Repr = drop(prefixLength(p))
                                              ^
src/library/scala/collection/IndexedSeqOptimized.scala:159: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  def span(p: A => Boolean): (Repr, Repr) = splitAt(prefixLength(p))
                                                    ^
src/library/scala/collection/SeqProxyLike.scala:35: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  override def prefixLength(p: A => Boolean) = self.prefixLength(p)
                                                    ^
src/library/scala/collection/SeqProxyLike.scala:36: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  override def indexWhere(p: A => Boolean): Int = self.indexWhere(p)
                                                       ^
src/library/scala/collection/SeqProxyLike.scala:43: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
  override def lastIndexWhere(p: A => Boolean, end: Int): Int = self.lastIndexWhere(p)
                                                                     ^
src/library/scala/collection/SeqViewLike.scala:115: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
    protected[this] lazy val len = self prefixLength pred
                                        ^
src/library/scala/collection/SeqViewLike.scala:123: warning: Use of higher order methods in GenTraversable et al not advisable from outside of scala.colllection.parallel
    protected[this] lazy val start = self prefixLength pred
                                          ^

Maybe checking for foreach usages is enough here, but I wanted to start broadly. In addition to the bug in this PR and the potential bugs I found manually, these two are pretty glaring:

scala> List(0 to 10 par).transpose.flatten
warning: there was one feature warning; re-run with -feature for details
res18: List[Int] = List(0, 1, 2, 3, 4, 8, 9, 10, 5, 6, 7)

scala> List(1, 2, 3).flatMap[Int, List[Int]](x => (1 to 10).par)
res19: List[Int] = List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 1, 2, 3, 4, 5, 6, 7, 8, 10, 9, 1, 2, 3, 6, 7, 8, 4, 5, 9, 10)

adriaanm · 2015-01-28T21:23:13Z

PLS REBUILD ALL

scala-jenkins · 2015-01-28T21:28:46Z

(kitty-note-to-self: ignore 71918881)
🐱 Roger! Rebuilding pr-scala for fb04376. 🚨

Ichoran · 2015-01-30T22:52:26Z

Merging as-is; the need to .seq is, sadly, rather common in the code, so there is not much benefit in having a comment here pointing it out. @retronym - did you file a ticket for the other spots (or fix them)?

Ichoran · 2015-01-30T22:54:22Z

Argh, silly laptop touchpad. Didn't mean to close.

SI-9072 Vector ++ concatenation of parallel collection cause inconsisten...

Ichoran · 2015-01-30T23:10:37Z

@retronym - Filed an issue for this, hopefully not a duplicate: https://issues.scala-lang.org/browse/SI-9126

@adriaanm - Not sure whether you intended me to merge this or to review it again (I already had, and nothing had really changed?).

adriaanm · 2015-01-30T23:40:17Z

I meant to divide up the work of getting the four dozen or so pending PRs merged in due course.

SI-9072 Vector ++ concatenation of parallel collection cause inconsis…

fb04376

…tent results

scala-jenkins added this to the 2.11.6 milestone Jan 18, 2015

scala-jenkins added the needs-attention label Jan 18, 2015

scala-jenkins added the reviewed label Jan 18, 2015

scala-jenkins added tested and removed needs-attention labels Jan 29, 2015

adriaanm assigned Ichoran Jan 30, 2015

Ichoran closed this Jan 30, 2015

Ichoran reopened this Jan 30, 2015

Ichoran added a commit that referenced this pull request Jan 30, 2015

Merge pull request #4259 from mzitnik/2.11.x

5d7098c

SI-9072 Vector ++ concatenation of parallel collection cause inconsisten...

Ichoran merged commit 5d7098c into scala:2.11.x Jan 30, 2015

This was referenced Apr 7, 2017

Par[Vector/Range] conversion to Vector using ++ cause inconsistent results scala/bug#9072

Closed

Missing .seqs causes problems with parallel collections passed as GenXs scala/bug#9126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SI-9072 Vector ++ concatenation of parallel collection cause inconsisten...#4259

SI-9072 Vector ++ concatenation of parallel collection cause inconsisten...#4259
Ichoran merged 1 commit intoscala:2.11.xfrom
mzitnik:2.11.x

mzitnik commented Jan 18, 2015

Uh oh!

Ichoran commented Jan 18, 2015

Uh oh!

retronym commented Jan 20, 2015

Uh oh!

Ichoran commented Jan 20, 2015

Uh oh!

retronym commented Jan 20, 2015

Uh oh!

adriaanm commented Jan 28, 2015

Uh oh!

scala-jenkins commented Jan 28, 2015

Uh oh!

Ichoran commented Jan 30, 2015

Uh oh!

Ichoran commented Jan 30, 2015

Uh oh!

Ichoran commented Jan 30, 2015

Uh oh!

adriaanm commented Jan 30, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

mzitnik commented Jan 18, 2015

Uh oh!

Ichoran commented Jan 18, 2015

Uh oh!

retronym commented Jan 20, 2015

Uh oh!

Ichoran commented Jan 20, 2015

Uh oh!

retronym commented Jan 20, 2015

Uh oh!

adriaanm commented Jan 28, 2015

Uh oh!

scala-jenkins commented Jan 28, 2015

Uh oh!

Ichoran commented Jan 30, 2015

Uh oh!

Ichoran commented Jan 30, 2015

Uh oh!

Ichoran commented Jan 30, 2015

Uh oh!

adriaanm commented Jan 30, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants