Add arrays to collection strawman #1414

odersky · 2016-07-25T18:17:49Z

This PR investigates what it takes to extend CollectionStrawMan5 to
arrays. Review by @szeiger ?

This PR investigates what it takes to extend CollectionStrawMan5 to arrays.

odersky · 2016-07-25T18:31:50Z

Actually, to see what changes it's best to diff CollectionStrawMan5 and CollectionStrawMan6.

Demonstrates how to integrate lazy non-view collections in the framework.

By making LinearSeq an IterableLike, we can use tail-recursion on drop.

substDealias did not follow aliases when the prefix of a typeref changed under substitution. This was exhibited by a bug in extensionMethods which was first discovered in CollectionStrawMan6 and was minimized in extmethods.

Makes it clearer what it is. Also, fixed check file.

The closures generated by elimByName did not get the InSuperCall flag set. This caused problems in lambda lift which led to a verify error for the new version CollectionStrawMan6. That version replaces explicit function parameters in class LazyList by by-name parameters. Also: Clarify logic for liftLocals in LambdaLift (liftLocals caused the immediate problem but was in the end not to blame).

@2

Test generated code before but fails with verify error at runtime. Here's the message: Exception in thread "main" java.lang.VerifyError: Bad type on operand stack Exception Details: Location: D$.<init>()V @2: invokedynamic Reason: Type uninitializedThis (current frame, stack[1]) is not assignable to 'D$' Current Frame: bci: @2 flags: { flagThisUninit } locals: { uninitializedThis } stack: { uninitializedThis, uninitializedThis } Bytecode: 0x0000000: 2a2a ba00 1f00 00b7 0022 2ab3 0024 b1 at Test$.main(t3048.scala:13) at Test.main(t3048.scala) With the fix in last commit, test causes backend to crash with java.lang.AssertionError: assertion failed: val <none> at scala.Predef$.assert(Predef.scala:165) at scala.tools.nsc.backend.jvm.BCodeHelpers$BCInnerClassGen$class.assertClassNotArray(BCodeHelpers.scala:214) at scala.tools.nsc.backend.jvm.BCodeHelpers$BCInnerClassGen$class.assertClassNotArrayNotPrimitive(BCodeHelpers.scala:219) at scala.tools.nsc.backend.jvm.BCodeHelpers$BCInnerClassGen$class.getClassBTypeAndRegisterInnerClass(BCodeHelpers.scala:238) at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.getClassBTypeAndRegisterInnerClass(BCodeSkelBuilder.scala:51) at scala.tools.nsc.backend.jvm.BCodeHelpers$BCInnerClassGen$class.internalName(BCodeHelpers.scala:210)

odersky · 2016-07-27T12:46:29Z

@DarkDimius Can you take this over? We had a verify error before that turned into a crash
in backend with the fix I added in the last commit. See commit message for details.

DarkDimius · 2016-07-27T12:59:08Z

@odersky, looking at it now.

DarkDimius · 2016-07-27T14:02:59Z

It's a funny interaction between:

elim-by-name(and erasure specifically);
lift-static;
supercalls.

object E extends F2(new B {})

Here we have an anonymous class new B {} that looks like it is created by erasure.
For some reason this class forgets the link to original anonymous class: SymDenot(E$annon1).initial.phase == erasure. I guess this is a bug.

Additionally, the owner of E$annon1 is an anonymous method inside E, that is inSuperCall and thus we have an anonymous nested class that has enclosingClass be package. This class looks like a top-level anonymous class breaking a lot of assumptions in shared backend and taking multiple branches in unexpected ways.

I don't want to rush it, I need to think about what's the right way to do this.

@odersky

It's a funny interaction between: elim-by-name(and erasure specifically); lift-static; supercalls. object E extends F2(new B {}) Here we have an anonymous class new B {} that looks like it is created by erasure. For some reason this class forgets the link to original anonymous class: SymDenot(E$annon1).initial.phase == erasure. I guess this is a bug. Additionally, the owner of E$annon1 is an anonymous method inside E, that is inSuperCall and thus we have an anonymous nested class that has enclosingClass be package. This class looks like a top-level anonymous class breaking a lot of assumptions in shared backend and taking multiple branches in unexpected ways. I'm not sure that this is a proper fix. I assume there's a bigger bug around, but I don't quite understand it right now and I need to work on other stuff. Making a quick fix to unblock @odersky.

odersky · 2016-07-27T14:31:03Z

Thanks for the quick fix, @DarkDimius !

DarkDimius · 2016-07-27T14:33:21Z

/rebuild
Needed to trigger CI by hand for some reason.

odersky · 2016-07-27T17:48:30Z

/rebuild

odersky · 2016-07-27T17:48:44Z

Not sure what's going on with CI today.

Following the other colltests, put each in a separate package.

odersky · 2016-07-28T09:08:39Z

I think we are on the right track here. Both Arrays (strict, external) and LazyLists (lazy, internal) were easy to integrate. I believe this is a good indication that the framework covers the right things.

odersky · 2016-07-28T09:10:38Z

As to review, I think it's best of @DarkDimius or @smarter could have a look at it, because the critical parts are compiler fixes in core and transforms.

szeiger · 2016-07-28T10:43:12Z

Also looking at it now, but I first have to familiarize myself with version 5 (and using dotty for the first time)

- Add proper :: to lists - Move some methods to IterableOps in order to keep Iterable clean - Rename knownLength to knownSize - Add some implentations for performance and completeness

szeiger · 2016-07-28T13:59:26Z

src/strawman/collections/CollectionStrawMan6.scala

+  import CollectionStrawMan6._
+
+  /** Convert array to iterable via view. Lower priority than ArrayOps */
+  implicit def arrayToView[T](xs: Array[T]): ArrayView[T] = new ArrayView[T](xs)


Why do we need two separate classes? Could the array operations be implemented in ArrayView?

Then they would not be considered in this scenario:

val x: Seq[Int] = Array(1, 2, 3)

odersky · 2016-07-28T15:16:12Z

OOM again.

odersky · 2016-07-28T15:22:26Z

/rebuild

szeiger · 2016-07-28T15:55:36Z

src/strawman/collections/CollectionStrawMan6.scala

+    }
+
+    /** A builder resulting from this builder my mapping the result using `f`. */
+    def mapResult[NewTo](f: To => NewTo) = new Builder[A, NewTo] {


Is this a good abstraction for efficiency? As used in ListBuffer, it adds overhead for the copy-on-write semantics, which could be avoided with a special builder class.

I am not sure I have understood where you see the inefficiency. When using a ListBuffer to build a List, there's one additional object created (namely the List builder which is the result of mapResult). True, we would could prevent allocation of that object by specialising.

It looks like ListBuffer only uses copy-on-write because it is used internally as a builder. COW may add overhead because it has to check aliased on every append (but I wouldn't be surprised if the impact turns out to be negligible in practice due to branch prediction doing the right thing).

ListBuffer always had copy on write semantics, and that's also heavily used in manual construction. Having a ListBuffer without COW would not gain us much I believe.

szeiger · 2016-07-28T16:49:42Z

src/strawman/collections/CollectionStrawMan6.scala

+
+    def this() = this(new Array[AnyRef](16), 0)
+
+    private var elems: Array[AnyRef] = initElems


Will the dotty linker be able to specialize this?

So far, no. If user insisted on AnyRef, it currently does nothing.

It would be great if this problem could be solved in a strawman proposal. Getting specialized arrays internally in the collection implementations would be a big improvement over the current collections library.

odersky · 2016-07-29T11:53:14Z

/rebuild

odersky · 2016-07-29T16:15:59Z

/rebuild

…alled on. This is achieved by putting it into a new trait, LinearSeqLike.

Avoids missing member in tangledCompanion.scala, which is a minimization of intermittent failures in CollectionStrawMan6. Intermittent, because it depended on order of compilation. CollectionTests have to be compiled together with but before CollectionStrawMan6 (this was _sometimes_ the case because partest did not honor indicated compilation order so far). I.e. dotc CollectionTests_2.scala CollectionStrawMan6_1.scala would trigger the error. tangledCompanion.scala captures the dependencies in a single file.

When a member is missing, print whether we were looking for a type or a value.

odersky · 2016-07-29T18:46:23Z

After tracking down the intermittent problem with "member not found", and fixing it in 2902000, force pushed a cleaned up version without added debug info.

odersky · 2016-07-30T10:36:03Z

@szeiger I think you might be right about IndexedView. The added overhead seems quite reasonble after all. In the old collections it was hard because IndexedView had to duplicate all transforms of View including those that do not make sense (the latter were forced or treated in an ad-hoc way).

@szeiger

Followinf @szeiger's suggestion, equip IndexView with optimized operations for map/drop/take.

smarter · 2016-07-30T22:07:51Z

src/strawman/collections/CollectionStrawMan6.scala

@@ -156,6 +159,8 @@ object CollectionStrawMan6 extends LowPriority {
    }
  }

+  type IndexedSeq[+A] = Seq[A] { def view: IndexedView[A] }


If you do this then any call to view on a value with type IndexedSeq[Foo] will end up going through reflection, no?

On Sun, Jul 31, 2016 at 12:07 AM, Guillaume Martres <
[email protected]> wrote:

In src/strawman/collections/CollectionStrawMan6.scala
#1414 (comment):

@@ -156,6 +159,8 @@ object CollectionStrawMan6 extends LowPriority {
}
}

type IndexedSeq[+A] = Seq[A] { def view: IndexedView[A] }

If you do this then any call to view on a value with type IndexedSeq[Foo]
will end up going through reflection, no?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lampepfl/dotty/pull/1414/files/dabf044485c60245e0fdf64c28eaa90285242a75..8db8c110a2b2a287875222a74511511d80be2b15#r72894931,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAwlVn1LM5xD42dSM8pPrJcmLIQls_Cjks5qa8s5gaJpZM4JUZlv
.

I don't think so, since view is already a method on Seq.

odersky · 2016-08-14T07:54:14Z

I am going to merge now, since I think it's good to have this one on record, and in the tests.

Add arrays to collection strawman

0c489f5

This PR investigates what it takes to extend CollectionStrawMan5 to arrays.

odersky added the In progress label Jul 25, 2016

odersky added 6 commits July 26, 2016 23:51

Further extension with LazyList

84466af

Demonstrates how to integrate lazy non-view collections in the framework.

Improve drop

568e489

By making LinearSeq an IterableLike, we can use tail-recursion on drop.

Fix substDealias

0d44e9e

substDealias did not follow aliases when the prefix of a typeref changed under substitution. This was exhibited by a bug in extensionMethods which was first discovered in CollectionStrawMan6 and was minimized in extmethods.

Rename fromLikeIterable -> fromIterableWithSameElemType

aa0bf12

Makes it clearer what it is. Also, fixed check file.

Simplify lexicallyEnclosingClass

87cff22

Make colltest6 self-contained

a591802

Following the other colltests, put each in a separate package.

odersky added Needs review and removed In progress labels Jul 27, 2016

Tweaks to strawman

09b81e5

- Add proper :: to lists - Move some methods to IterableOps in order to keep Iterable clean - Rename knownLength to knownSize - Add some implentations for performance and completeness

szeiger reviewed Jul 28, 2016
View reviewed changes

odersky added 3 commits July 29, 2016 20:39

Drop on LinearSeq needs to return collection of same type as it was c…

9ab6ce7

…alled on. This is achieved by putting it into a new trait, LinearSeqLike.

Print kind of missing member

dabf044

When a member is missing, print whether we were looking for a type or a value.

odersky force-pushed the add-array-strawman branch from df2b3b8 to dabf044 Compare July 29, 2016 18:44

More systematic treatement of IndexedView

8db8c11

Followinf @szeiger's suggestion, equip IndexView with optimized operations for map/drop/take.

smarter reviewed Jul 30, 2016
View reviewed changes

odersky added 2 commits July 31, 2016 10:17

Fix problem with IndexedView.iterator.length

d88a280

Add missing import

4e95a10

odersky merged commit 3265323 into scala:master Aug 14, 2016

odersky removed the Needs review label Aug 14, 2016

scabug mentioned this pull request Apr 7, 2017

scanLeft (and right) for TraversableViewLike is very ineffective scala/bug#9902

Closed

allanrenucci deleted the add-array-strawman branch December 14, 2017 15:01


		def this() = this(new Array[AnyRef](16), 0)

		private var elems: Array[AnyRef] = initElems

Add arrays to collection strawman #1414

Add arrays to collection strawman #1414

Uh oh!

Conversation

odersky commented Jul 25, 2016

Uh oh!

odersky commented Jul 25, 2016

Uh oh!

odersky commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DarkDimius commented Jul 27, 2016

Uh oh!

DarkDimius commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

DarkDimius commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Jul 27, 2016

Uh oh!

odersky commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Jul 28, 2016

Uh oh!

szeiger commented Jul 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky commented Jul 28, 2016

Uh oh!

odersky commented Jul 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky commented Jul 29, 2016

Uh oh!

odersky commented Jul 29, 2016

Uh oh!

odersky commented Jul 29, 2016

Uh oh!

odersky commented Jul 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky commented Aug 14, 2016

Uh oh!

Uh oh!

odersky commented Jul 27, 2016 •

edited

Loading

DarkDimius commented Jul 27, 2016 •

edited

Loading

DarkDimius commented Jul 27, 2016 •

edited

Loading

odersky commented Jul 27, 2016 •

edited

Loading

odersky commented Jul 28, 2016 •

edited

Loading

odersky Jul 29, 2016 •

edited

Loading