Remove parallel collection views and, with them, Gen*View

[retronym]

This removes one layer from a many-layered implementation.
Maybe that will be enough to help us fix a bug or two.

[retronym]

Review by @Ichoran @jsuereth @adriaanm @axel22

• do we believe that the intersection of the sets of of parallel collection users and view users is near enough to zero to deprecate this feature?
  • even better, as proposed in the second commit, should we drop them immediately in 2.11, with a source compatible, if less performant, fallback to sequential views.

Secondary is review of the code change itself. If we decided to go ahead with this, I'll need to go through that again with a fine toothcomb to confirm linearization order hasn't accidentally changed. We can do that with tooling.

If we don't feel comfortable with this for 2.11, we could think about shooting off a 2.12 branch (I don't think that should be master just yet.) I sort of like that idea, actually: we are pretty bad at tracking distant TODO items in JIRA/code comments ("that's not BC so needs to wait for the next release" / "oh shoot, we forgot about that, well, let's try the next major release"), that maybe it would be better to always have the next major release around on a branch.

[retronym]

In "To Kill a View" @odersky points out that the GenXxx layers were a tipping point of complexity in views:

Here's the fundamental problem with views: To do them correctly you
need to re-implement a lot of classes in every layer of your
inheritance library. Each class captures the behavior of one common
collection operation and possible a couple of variants. We used to
have 4 such layers: Traversable, Iterable, Seq, and Linear/IndexedSeq.
Views were a pain to implement then, but it was sort of manageable. We
now have almost twice the number of layers because of the Gen...
types. This seemed to have pushed views over the edge where no person
can understand the code anymore and bugs like SI-4332 creep in and are
almost impossible to fix (I am pretty sure that you could take a slice
of a slice before 2.9, the whole point of views is that this should be
effcient!)

His suggestion was to:

So, here's an alternate proposal to get back some sanity: Let's
completely separate the implementations of sequential and parallel
collections, including their views. This means we still keep GenSeq,
GenIterable as common supertypes, but these would be pure interfaces
without any implementation. And traits GenIterableLike, GenSeqLike,
etc would be removed.

This means that both parallel and sequential collections have to
implement their own view hierarchies, but at least there's no
interference between them. And we collapse the number of levels from 7
to 4 or maybe 3 (if Iterable and Traversable get merged). Hopefully
this makes views manageable again. And it would have other benefits
such as reducing the number of supertraits of common types such as
List.

This PR is a slightly different takes a different path to the same destination:
remove parallel collection views altogether.

[axel22]

Views in parallel collections are something I never really liked a lot, and I think they need a completely different hierarchy, since the operations that take can implement reasonably efficient comprise only a subset of all collection operations. Expecting to replace any occurence of a collection/parallel collection with a view and reap the benefits is in vain.

So if you want a quick ok after a quick glance on the changeset, without studying this PR carefully - LGTM.

However, isn't the standard way of doing things now going through a deprecation cycle?

[soc]

While I think this is a move into the right direction, I think it should go into 2.12 (never thought I would be one of those who doesn't want to ship everything immediately :-D), because this should be - in the best case - only the tip of the iceberg.

In my opinion, we need to have a look at the whole collection framework and rethink our strategy we have concerning performance and the things Java 8 ships.

• I'm pretty sure that we can't even remotely reach the performance of Java 8's Streams with the current design of views.
• We should reevaluate whether we want to keep our approach of let's-build-immediate-collections-after-every-collection-operation-by-default. Compared to the Iterator like design of Java 8, this is extremely wasteful.

The only advantage of views compared to Java 8's Streams as far as I see is that one can't "exhaust" them.
On the other side, even large non-view operations which should be slower by all means sometimes beat views.
Additionally, it seems we can't really make sure that we don't have dozens of missing/broken methods in views all over the place.

I mean, IF we can make views comparably fast, then we have a huge issue less (then only the question whether we should do it by default remains ... I think that would be the right thing, but even if we don't we still don't mandate more boilerplate than Java), if we can't make it work, we need to consider other options:

As far as I see, individual collection types shine when we use individual operation for which they were built, but bulk-operations can almost never use these advantages, because what matters most here is pure iteration speed (e. g. having some special data-structure with a blazingly fast operation X just can't show its advantages when we spent a considerable amount of time building and rebuilding the collection after pretty much every step ... which are probably exactly those types of operations many special purpose data-structures are not fast at).

Maybe a "common" (e. g. one type) way to facilitate these bulk-operations similar to Streams would prove to be more beneficial, because it would allow us to really tune all the operations there instead of having to deal with * plus the issue of all the overhead of the mentioned reification of the data structure after every operation.

A completely different approach would be to keep things the way they are and add some notion of purity to the compiler, so that the compiler can avoid the overhead if it isn't observable from the outside. I'm not sure this is practical though, considering that we want to have less complexity, not more and purity is a huge, complicated topic which we probably won't be able to spec, implement and ship in the next few releases.

In the end, if we really want to fix the remaining issues of the collection library, we have to think about what level of breakage is acceptable. I think it is possible to preserve source compatibility to a large extent, but I think all those people with custom collections will have to fix things manually.

Wow, this has gotten quite long ... maybe I should post to the mailing list instead.

[soc]

Idea: Maybe we can make parallel collections lazy by default (what I mean by that is to avoid building all the immediate data structures mentioned above), because there aren't any guarantees regarding the order in which things happen anyway.

[axel22]

Well, there are guarantees currently - there is an ordering between different operations in e.g. xs.par.map(f).filter(p) where all side-effects in f should happen before those in p, but given what a typical usecase is and the fact that these functions are almost pure, I agree with you that the current semantics should be changed.

soc notifications@github.com wrote:

Idea: Maybe we can make parallel collections lazy by default (what I
mean by that is to avoid building all the immediate data structures
mentioned above), because there aren't any guarantees regarding the
order in which things happen anyway.

---

Reply to this email directly or view it on GitHub:
#3191 (comment)

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[retronym]

@soc We have to tear things down, before we can build them up anew. Parallel views are the low hanging fruit that I think we should seriously consider for this release.

There is no way we can migrate to an approach were we auto-fuse List(1, 2, 3).map(f).map(g). Is that what you are suggesting?

There is some research happening at EPFL into (parallel) collections 2.0, part of which is a fusing macro, so you could write fusing { ...; List(..).map(f).map(g) }.

@axel22: My counter argument for the deprecation cycle here is that xs.par.view will still work, it will just now be deprecated and sequential.

[axel22]

In that case, LGTM!

Jason Zaugg notifications@github.com wrote:

@soc We have to tear things down, before we can build them up anew.
Parallel views are the low hanging fruit that I think we should
seriously consider for this release.

There is no way we can migrate to an approach were we auto-fuse
List(1, 2, 3).map(f).map(g). Is that what you are suggesting?

There is some research happening at EPFL into (parallel) collections
2.0, part of which is a fusing macro, so you could write fusing { ...; List(..).map(f).map(g) }.

@axel22: My counter argument for the deprecation cycle here is that
xs.par.view will still work, it will just now be deprecated and
sequential.

---

Reply to this email directly or view it on GitHub:
#3191 (comment)

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[adriaanm]

+1!

On Sunday, November 24, 2013, Aleksandar Prokopec wrote:

In that case, LGTM!

Jason Zaugg <notifications@github.com <javascript:_e({}, 'cvml',
'notifications@github.com');>> wrote:

@soc We have to tear things down, before we can build them up anew.
Parallel views are the low hanging fruit that I think we should
seriously consider for this release.

There is no way we can migrate to an approach were we auto-fuse
List(1, 2, 3).map(f).map(g). Is that what you are suggesting?

There is some research happening at EPFL into (parallel) collections
2.0, part of which is a fusing macro, so you could write fusing { ...; List(..).map(f).map(g) }.

@axel22: My counter argument for the deprecation cycle here is that
xs.par.view will still work, it will just now be deprecated and
sequential.

---

Reply to this email directly or view it on GitHub:
#3191 (comment)

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3191#issuecomment-29181318
.

[retronym]

For the sequential views that we retain, we still are faced with the "view gaps." These arise when new methods are added to the collections API and are not overriden in the view.

For example:

scala> List(1).view.distinct.force
java.lang.UnsupportedOperationException: SeqView(...).newBuilder

scala> List(1).view.inits.toList
java.lang.UnsupportedOperationException: SeqView(...).newBuilder

It turns out these are pretty easy to fix, once you spot them. (This fact was never clear to me before.)

Once just needs to add:

  override def distinct: This =
    newForced(thisSeq.distinct).asInstanceOf[This]

That will force all the prior operations over then entire collection, but, hey, that's better than a UnsupportedOperationError!

So, all that remains is a robust way to identify the gaps, and to prevent more from opening up. The following reflects on the views and finds all methods which produce a new view (read: ones that return a type containing "Repr") that are inherited from somewhere other than "*ViewLike":

scala> def checkView(viewType: Type, viewLikeType: Type) {
     |   val sep = "=" * 40
     |   println(s"\n$sep\nChecking ${viewType.typeSymbol.fullName}\n$sep")
     |   val methods = viewType.nonPrivateMembers.toList.filter(_.isMethod).map(fullyInitializeSymbol)
     |   val inheritedMethods = methods.filterNot(_.owner.name.decoded.contains("ViewLike"))
     |   val needOverride = inheritedMethods.filter(sym => viewType.memberType(sym).contains(viewType.typeSymbol))
     |   val report = needOverride.groupBy(_.owner).map{
     |     case (owner, syms) => s"\n$owner\n${"-" * 40}\n${syms.map(_.defString).mkString("\n")}"
     |   }.mkString("\n")
     |   println(report)
     | }
checkView: (viewType: $r.intp.global.Type, viewLikeType: $r.intp.global.Type)Unit

scala>

scala> def checkViews {
     |   import collection._
     |   checkView(typeOf[TraversableView[_, _]], typeOf[TraversableViewLike[_, _, _]])
     |   checkView(typeOf[IterableView[_, _]], typeOf[IterableViewLike[_, _, _]])
     |   checkView(typeOf[SeqView[_, _]], typeOf[SeqViewLike[_, _, _]])
     |   checkView(typeOf[immutable.StreamView[_, _]], typeOf[immutable.StreamViewLike[_, _, _]])
     | }
checkViews: Unit

scala>

scala> checkViews

========================================
Checking scala.collection.TraversableView
========================================

trait TraversableLike
----------------------------------------
def view(from: Int,until: Int): scala.collection.TraversableView[A,Repr]
def view: scala.collection.TraversableView[A,Repr]
def inits: Iterator[Repr]
def tails: Iterator[Repr]
private[package scala] def sliceWithKnownBound(from: Int,until: Int): Repr
private[package scala] def sliceWithKnownDelta(from: Int,until: Int,delta: Int): Repr
override def tail: Repr
def scan[B >: A, That](z: B)(op: (B, B) => B)(implicit cbf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def filterNot(p: A => Boolean): Repr
def ++:[B >: A, That](that: Traversable[B])(implicit bf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def ++:[B >: A, That](that: scala.collection.TraversableOnce[B])(implicit bf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
protected[this] def toCollection(repr: Repr): Traversable[A]
def repr: Repr

========================================
Checking scala.collection.IterableView
========================================

trait TraversableLike
----------------------------------------
def inits: Iterator[Repr]
def tails: Iterator[Repr]
private[package scala] def sliceWithKnownBound(from: Int,until: Int): Repr
private[package scala] def sliceWithKnownDelta(from: Int,until: Int,delta: Int): Repr
override def tail: Repr
def scan[B >: A, That](z: B)(op: (B, B) => B)(implicit cbf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def filterNot(p: A => Boolean): Repr
def ++:[B >: A, That](that: Traversable[B])(implicit bf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def ++:[B >: A, That](that: scala.collection.TraversableOnce[B])(implicit bf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def repr: Repr

trait IterableLike
----------------------------------------
override def view(from: Int,until: Int): scala.collection.IterableView[A,Repr]
override def view: scala.collection.IterableView[A,Repr]
def dropRight(n: Int): Repr
def takeRight(n: Int): Repr
def sliding(size: Int): Iterator[Repr]
override protected[this] def toCollection(repr: Repr): Iterable[A]

========================================
Checking scala.collection.SeqView
========================================

trait SeqLike
----------------------------------------
override def view(from: Int,until: Int): scala.collection.SeqView[A,Repr]
override def view: scala.collection.SeqView[A,Repr]
def sortBy[B](f: A => B)(implicit ord: scala.math.Ordering[B]): Repr
def sortWith(lt: (A, A) => Boolean): Repr
def distinct: Repr
def combinations(n: Int): Iterator[Repr]
def permutations: Iterator[Repr]
override protected[this] def toCollection(repr: Repr): Seq[A]

trait TraversableLike
----------------------------------------
def inits: Iterator[Repr]
def tails: Iterator[Repr]
private[package scala] def sliceWithKnownBound(from: Int,until: Int): Repr
private[package scala] def sliceWithKnownDelta(from: Int,until: Int,delta: Int): Repr
override def tail: Repr
def scan[B >: A, That](z: B)(op: (B, B) => B)(implicit cbf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def filterNot(p: A => Boolean): Repr
def ++:[B >: A, That](that: Traversable[B])(implicit bf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def ++:[B >: A, That](that: scala.collection.TraversableOnce[B])(implicit bf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def repr: Repr

trait IterableLike
----------------------------------------
def dropRight(n: Int): Repr
def takeRight(n: Int): Repr
def sliding(size: Int): Iterator[Repr]

========================================
Checking scala.collection.immutable.StreamView
========================================

trait SeqLike
----------------------------------------
override def view(from: Int,until: Int): scala.collection.SeqView[A,Repr]
override def view: scala.collection.SeqView[A,Repr]
def sortBy[B](f: A => B)(implicit ord: scala.math.Ordering[B]): Repr
def sortWith(lt: (A, A) => Boolean): Repr
def distinct: Repr
def combinations(n: Int): Iterator[Repr]
def permutations: Iterator[Repr]
override protected[this] def toCollection(repr: Repr): Seq[A]

trait TraversableLike
----------------------------------------
def inits: Iterator[Repr]
def tails: Iterator[Repr]
private[package scala] def sliceWithKnownBound(from: Int,until: Int): Repr
private[package scala] def sliceWithKnownDelta(from: Int,until: Int,delta: Int): Repr
override def tail: Repr
def scan[B >: A, That](z: B)(op: (B, B) => B)(implicit cbf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def filterNot(p: A => Boolean): Repr
def ++:[B >: A, That](that: Traversable[B])(implicit bf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def ++:[B >: A, That](that: scala.collection.TraversableOnce[B])(implicit bf: scala.collection.generic.CanBuildFrom[Repr,B,That]): That
def repr: Repr

trait IterableLike
----------------------------------------
def dropRight(n: Int): Repr
def takeRight(n: Int): Repr
def sliding(size: Int): Iterator[Repr]

I will plug these and backstop with a test that runs this report.

There are of course deeper issues with views that these two changes won't address: their strength (transparency) is also a weakness: it's hard to predict whether they will be forced when you hand them off to unknown code that operates over, e.g, Seq[A].

[retronym]

I've plugged the gaps in sequential views in #3192

[retronym]

PLS REBUILD ALL

[scala-jenkins]

(kitty-note-to-self: ignore 29199430)
🐱 Roger! Rebuilding pr-scala for 2ce7b12, e2fd3039. 🚨

[soc]

@retronym regarding “We have to tear things down, before we can build them up anew.”, I agree. Even if we would reintroduce them with improved/fixed semantics and better performance, a deprecation seems to be the right thing to do.

[paulp]

That will force all the prior operations over then entire collection, but, hey, that's better than a UnsupportedOperationError!

For the record, I disagree, and that's why I never did it. Lots of manual maintenance is a possible way to promote the current view architecture from unusable to terrible, and allow you to keep it for a lot longer. If adding a method to the collections requires changes in half a dozen places, and if you miss one it all compiles and runs and breaks in the distant future when someone actually uses views, then it's being done wrong and you will never run out of UnsupportedOperationErrors to chase down (and heaven help them if you ever decide to do something better with your time.)

Independently of all that, it's still not clear that it's better. If you are naively using views it is very likely the execution model matters. Would you rather see an exception the first time you run it, if the alternative is that your application is 100x slower than it should be? Not everyone is better off with the slow option, by any means.

Good doctors know that sometimes life support is not the merciful option. Views should have a DNR order.

[soc]

@paulp With the new test cases and the deprecation, isn't that already moving into exactly this direction?

Btw, I' curious of the impact on jar file size ... :-)

[paulp]

@paulp With the new test cases and the deprecation, isn't that already moving into exactly this direction?

As long as the white-knuckled clinging to the view code continues, the only direction I can see it moving in is circles.

[paulp]

Also, don't expect my approval of anything, this is lose/lose no matter how it goes. Remember that this is a good chunk of the reason I quit, so if you really do fix it now, imagine how that looks from here.

[retronym]

Views can only ever be a best-effort performance wise given the breadth of Traversable et al. You might not hit the USOE in your tests, but hit it in the future when someone else's code that your view passes through calls distinct or takeRight or whatever. If you care about performance you can run your tests with a breakpoint in newForced.

[retronym]

If adding a method to the collections requires changes in half a dozen places, and if you miss one it all compiles and runs and breaks in the distant future when someone actually uses views

That's what the test guards against. Looks like we have managed to deprecate Forwarder/Proxy, but otherwise the same approach would work for them. I know you you that, as you wrote the same reflective checks many moons ago.

[retronym]

That's what the test guards against

Sorry, I thought we were over in #3192, where that test lives.

[retronym]

PLS REBUILD ALL

[scala-jenkins]

(kitty-note-to-self: ignore 29207338)
🐱 Roger! Rebuilding pr-scala for 2ce7b12, e2fd3039. 🚨

[retronym]

Also, don't expect my approval of anything, this is lose/lose no matter how it goes. Remember that this is a good chunk of the reason I quit, so if you really do fix it now, imagine how that looks from here.

I'm not really sure what to make of that. Here's what's happened:

• I warned someone off views on the ML, pointing, eventually, to the "View Gaps" bug.
• Prompted by (deserved) derision on Twitter, I took a look a fresh look views and saw for the first time that GenXxx doesn't define view, and that Gen*View exist solely as a code-reuse mechanism.
• Armed with this knowledge, I saw that we could at least kill parallel views (perhaps as soon as 2.11) and a decent chunk of the trait linearization complexity in sequential views with them.
• After you pointed out the "To Kill a View", I found Martin's comment that the Gen*Views were a tipping point of implementation complexity. That happened to support my case so I quoted it here.
• Previously, I had no idea how to fix distinct and friends, and had assumed there was something fundamentally hard about it. But it seems the hardest part is avoiding bit rot, which we can't enforce with types, but we can with reflective tests. That's less that perfect, but far better than nothing.

I don't buy into arguments that we shouldn't fix code incrementally as that weakens that case for removing it altogether. All the more when the effort to fix it is relatively small.

[paulp]

@retronym Don't take anything I say about this as criticism of you. There's nothing wrong with your choices. You are an enabler to some extent, but so was I, more so and for longer.

[paulp]

I know you you that, as you wrote the same reflective checks many moons ago.

I hadn't seen the test you're talking about yet. It's true, and at the time I wrote that it wouldn't have done any good because it wouldn't have been run before code was checked in. Now things are different, you could integrate it into the PR tests.

[retronym]

I hadn't seen the test you're talking about yet. It's true, and at the time I wrote that it wouldn't have done any good because it wouldn't have been run before code was checked in. Now things are different, you could integrate it into the PR tests.

(Sounds like you've found it already, but for the benefit of onlookers)

It's this one:

https://github.com/scala/scala/pull/3192/files#diff-873f35ad470869e7a7df74e7169f9b56R1

[axel22]

While I fully agree that this is a step in the right direction - just to give my two cents: I second Paul's philosophy that something called a View that people use to obtain better performance should not result in worse performance just to have the same API, so that users are happy about how they added a view and everything works.
I strongly believe that that kind of design approach is a mistake, and it's lying and hiding the truth from the users.

I hope that the future redesign Views will not have all collection methods. They should have an entirely different hierarchy than the rest of the collections, since they are, well, different. For example, if a view cannot lazily support distinct, then it should not have it.

Not being a native speaker and a professional scrabble player, I often don't understand Paul's remarks :)
However, if his remark about life support was meant in the same way as I wrote above, then I fully second that.

I'm not saying that this PR is in some way wrong, it's an iterative step mending the current state of views. In a future redesign, I would consider having them have a different hierarchy of methods more reasonable for their lazyness. For example, we've made the distinction between Reducables and Zippables in the new parallel collections for this reason. I'm think of doing something similar for parallel views there.

[adriaanm]

Agreed this is a step in the right direction, and I hope we can address Paul & Alex's very valid concerns by making the deprecation message much stronger. Something along the lines of "Do not call view on a parallel collection! It's implemented as seq.view." We could offer an off-line binary patch that replaces these seq.view with throw new UnsupportedOperation in scala-libary.jar :-)

[Ichoran]

Without having tried to do so, my intuition is that we can alert ourselves to the necessity of attending to view performance in addition to keeping them up to date via reflection. Usually problems with view performance are easy to detect (factors of two), and we can use the same mechanism to demand performance tests as we use to demand an implementation so you don't throw runtime errors.

[retronym]

@adriaanm : @paulp and @axel22 are discussing a broader problem with all views: as they implement, e.g. Seq, they are forced to implement some methods for which no shortcut is possible (e.g. sorted, distinct). So its hard to know if/when they will be forced. An alternative design would offer a restricted set of operations, and make forcing explicit. It's a tradeoff, of course: such a view cannot be passed to existing code that operates on Seq.

That said, I don't mind strengthening the wording on the deprecation. I suspect we'll have approximately zero clients of that API, though. We didn't have any in our test suite.

To recap: my pair of view PRs don't attempt to solve this problem, but instead:

• remove an esoteric flavour of view (parallel) so as to bring the implementation complexity down
• fill in missing operations in sequential views that currently trigger runtime failures (some efficiently, like takeRight, and others by forcing, like distinct)

[paulp]

@axel22 Don't worry, native english speakers find me equally inscrutable. Thanks, your statement isn't precisely what I meant but it's close enough and I certainly agree with it.

[retronym]

PLS REBUILD ALL

[scala-jenkins]

(kitty-note-to-self: ignore 29295187)
🐱 Roger! Rebuilding pr-scala for 2ce7b12, e2fd3039. 🚨

[adriaanm]

thanks for clearing that up, I glossed over a little too fast there

[retronym]

This one is good to go from my side. I think we have a consensus that we are source compatible enough, the only clients affected are those that referred to the Gen*View traits or Par*View traits directly, which we consider to be implementation detail and effectively private[scala].

I'd better give it a:

PLS REBUILD ALL

To make sure it doesn't clash with the recently merged #3192.

[scala-jenkins]

(kitty-note-to-self: ignore 29396995)
🐱 Roger! Rebuilding pr-scala for 2ce7b12, 51cd474. 🚨