Make LazyList's empty-or-not status lazy too #7000

NthPortal · 2018-08-04T19:19:57Z

src/library/scala/collection/immutable/LazyList.scala

NthPortal · 2018-08-06T06:02:39Z

LazyList.empty uses a cached LazyList.State, so it is only evaluated once (at which point it stops being lazy). Thus, whether or not LazyList.empty.toString returns "LazyList(?)" or "LazyList()" is not locally deterministic.

This makes some of the tests non-deterministic.

julienrf · 2018-08-06T07:36:25Z

Thanks @NthPortal! I think it would be better to have LazyList.empty to be strictly evaluated. The important thing is to provide the ability to create a LazyList that we don’t know if it is empty or not until we evaluate it (but that doesn’t mean that all LazyList instances should be like that).

allanrenucci · 2018-08-06T09:01:49Z

src/library/scala/collection/immutable/LazyList.scala

+
+  private lazy val state: State[A] = {
+    stateEvaluated = true
+    val res = lazyState()


Should the two lines above be swapped (in case evaluating the state throws)?

I don't think so. stateEvaluated is an indicator of whether or not we can touch state without breaking laziness. IMO, even if evaluating state throws, it should be considered evaluated and safe to touch.

allanrenucci · 2018-08-06T09:06:58Z

src/library/scala/collection/immutable/LazyList.scala

+
+      lazy val head: A = {
+        hdEvaluated = true
+        val res = hd()


Should the two lines above be swapped (in case evaluating hd throws)?

It's probably better to keep retrying instead of switching from an exception to nulls.

@Ichoran if it throws, hd won't get set to null, so it will keep retrying

src/library/scala/collection/immutable/LazyList.scala

NthPortal · 2018-08-06T21:21:49Z

I'm having trouble figuring out why filter_all_foreach_allows_GC is failing

SethTisue · 2018-08-07T01:18:13Z

is this only WIPpy because of the failing test, or is there other known WIPpiness here?

NthPortal · 2018-08-07T03:02:50Z

@SethTisue the WIP was because I was still in the process of removing tailDefined (which no longer corresponds to anything), and hadn't had the chance to rewrite addStringNoForce.

... I have now done that, and will push it shortly. And I will get rid of the WIP label :D

SethTisue · 2018-08-07T03:26:32Z

tentatively marking as an M5 blocker since if we're going make a basic change like this we need to leave folks time to test it

NthPortal · 2018-08-07T04:22:57Z

This is ready for review

NthPortal · 2018-08-07T05:06:16Z

What should we do about methods like drop, dropWhile, and other methods inherited from and optimised for LinearSeqOps (and potentially from SeqOps?). Currently, they are are implemented eagerly, which may help avoid stack overflow problems, but on the other hand, it seems like drop ought to be lazy.

dwijnand · 2018-08-07T07:06:08Z

JFYI LazyListTest.filter_all_foreach_allows_GC failed again.

julienrf · 2018-08-07T13:15:42Z

What should we do about methods like drop, dropWhile, and other methods inherited from and optimized for LinearSeqOps (and potentially from SeqOps?). Currently, they are implemented eagerly

This is a bug then, they should be implemented using the view-based approach.

javax-swing · 2018-08-07T19:50:42Z

This was introduced in #6608 to stop a stack safety issue. Just my 2 cents, but I think that drop and dropWhile should still use memory sharing.

perhaps something akin to

def drop(n : Int) : LazyList[A] =  {
  newLL {
    super.drop(n).state
  }
}

NthPortal · 2018-08-07T20:03:07Z

(introduced by me no less)

julienrf · 2018-08-08T17:14:37Z

Alternatively, we could move the overridden methods from LinearSeqOps to StrictOptimizedLinearSeqOps, since they are optimizations that really only work for strict collections.

Yes, that’s what we should do.

NthPortal · 2018-08-09T01:59:51Z

Does anyone have any idea why filter_all_foreach_allows_GC is failing? I can't reproduce it locally, and it seems like the primary things blocking this PR. withFilter is backed by Stream#filter too, and its tests aren't failing.

javax-swing · 2018-08-09T07:15:32Z

From looking at the previous implementation, it used to call iterableFactory.filteredTail on the head of the LazyList, this lazily took the value of the head and tail and stuck it into a new LazyList.Cons. This meant that once the function had exited, nothing was holding on to a reference to the head of the LazyList.

However in the new design the lazy State function for the filtered LazyList (for the head) holds a reference to the head of the Unfiltered LazyList. So in order for it to GC the head of the unfiltered LazyList it must first GC the lazyState function, so it could be purely that the GC path is more complicated for the JVM and it just doesn't feel like doing it 😁

Also the lazyState function wraps a byName parameter, which could also be in play here (in terms of GC). It may work locally for you due to differences in memory, cpu and jvm args.

Hope that this is useful or at the very least correct 😁

lrytz · 2018-08-09T13:39:38Z

The filter_all_foreach_allows_GC test fails when the optimizer is enabled, you can reproduce it by running setupValidateTest then testOnly *.LazyListTest in sbt. I suspect this is due to scala/bug#9137, will take a look.

NthPortal · 2018-08-09T14:14:35Z

@lrytz thanks! I think I have fixed it with @noinline, which I will push shortly

Extract Stream into its own file, and inline methods and implementation into Stream or its companion object.

Remove LazyListOps and LazyListFactory. Change LazyList to have a lazy state. The state can be either a cons cell with a lazy head, or an empty state.

NthPortal · 2018-08-09T21:38:29Z

I think everything brought up so far has been taken care of, and all the tests pass. Ready for final review so this can get in by tomorrow.

Ichoran

I have a couple small comments, but I think it overall looks good and what really needs to happen is that it gets into the library so it's easier to play with it and figure out if there are any bugs and if it has the right characteristics with regard to laziness etc..

Ichoran · 2018-08-09T22:17:47Z

src/library/scala/collection/immutable/LazyList.scala

+
+      lazy val head: A = {
+        hdEvaluated = true
+        val res = hd()


It's probably better to keep retrying instead of switching from an exception to nulls.

Ichoran · 2018-08-09T22:25:52Z

src/library/scala/collection/immutable/LazyList.scala

    }
+  }
+
+  private class LazyIterator[+A](private[this] var lazyList: LazyList[A]) extends AbstractIterator[A] {


Are we certain this should be private? We seem to rely on it for implementing a fair bit of functionality. How are library users supposed to replicate the functionality if this is private? If they can, why don't we?

Since it's specific to LazyList, it's not clear to me why you'd need to replicate the functionality - you can just call .iterator

To be clear, I don't feel strongly about it either way

julienrf · 2018-08-10T13:25:12Z

test/files/jvm/serialization-new.check

-x = LazyList(_, ?)
-y = LazyList(_, ?)
+x = LazyList(?)
+y = LazyList(?)


The implementation of addString is very hard to review. I guess all the places where you have changed the expected output of the tests are places where we previously knew that the lazy list was not empty and now we don’t know?

I agree that it's hard to review (it was also very hard to understand well enough to change to not use tailDefined). The tests were changed either because (a) previously we knew it was not empty, but now we don't, or (b) previously the tail hadn't been evaluated to a cycle, but since the tail is no longer lazy (since the whole thing is lazy), it now knows it's cyclic slightly earlier

julienrf · 2018-08-10T13:25:44Z

test/files/run/lazylist-dropwhile-stack-safe.scala

-  val stream = Iterator.iterate(Stream.from(0))(_.dropWhile(_ => false)).drop(10000).next
-  stream.head // superfluous, because `head` should be eager
-  stream.tail.head
-}


Why did you remove that test?

The test was added in #6608, which made drop and dropWhile stack-safe but not lazy. Since I changed them back to being lazy, they are no longer stack-safe

julienrf · 2018-08-10T13:35:18Z

test/junit/scala/collection/immutable/LazyListTest.scala

@@ -101,12 +101,12 @@ class LazyListTest {

  @Test // scala/bug#9134
  def filter_map_properly_lazy_in_tail: Unit = {
-    assertLazyListOpLazyInTail(_.filter(_ % 2 == 0).map(identity), List(1, 2))
+    assertLazyListOpLazyInTail(_.filter(_ % 2 == 0).map(identity), List(1))


IIUC this means that when we call filter and map we evaluate no elements of the lazy list, whereas we previously evaluated the first element?

julienrf · 2018-08-10T17:57:48Z

Thanks a lot to all the people who contributed to that work!

SethTisue · 2018-08-10T18:00:29Z

🎉

hrhino · 2018-08-10T18:09:45Z

🎉 💯

Thanks for making this actually happen, @NthPortal!

NthPortal · 2018-08-10T21:04:38Z

🎉
You're welcome! And thanks to everyone who made this happen, through reviews, prior PRs and discussions!

SethTisue · 2018-08-13T19:07:20Z

intermittent LazyListTest.foreach_allows_GC failure

java.lang.AssertionError: null, took 0.364 sec
[error]     at scala.collection.immutable.LazyListTest.assertLazyListOpAllowsGC(LazyListTest.scala:47)
[error]     at scala.collection.immutable.LazyListTest.foreach_allows_GC(LazyListTest.scala:52)
[error]     ...

https://scala-ci.typesafe.com/view/scala-2.13.x/job/scala-2.13.x-integrate-windows/663/consoleFull

NthPortal · 2018-08-13T21:38:01Z

could it be another inlining problem? I can try @noinlineing foreach, but I'm not sure how to tell if it fixes the problem

NthPortal · 2018-08-13T21:44:25Z

@SethTisue is it only a Windows thing? Also, how intermittent?

SethTisue · 2018-08-13T22:44:52Z

not sure yet on either question. so far I've only seen the one Windows failure, and then the next run was green

SethTisue · 2018-08-17T20:45:48Z

followup discussion taking place at scala/bug#11089 on whether this is now too lazy...

NthPortal added the WIP label Aug 4, 2018

scala-jenkins added this to the 2.13.0-M5 milestone Aug 4, 2018

NthPortal changed the title ~~Make LazyList fully lazy~~ [WIP] Make LazyList fully lazy Aug 4, 2018

NthPortal mentioned this pull request Aug 4, 2018

Lazier LazyList. #6880

Closed

NthPortal changed the title ~~[WIP] Make LazyList fully lazy~~ [WIP] Make LazyList fully lazy [ci: last-only] Aug 4, 2018

NthPortal mentioned this pull request Aug 6, 2018

Make LazyList#withFilter more lazy #6960

Closed

NthPortal commented Aug 6, 2018

View reviewed changes

src/library/scala/collection/immutable/LazyList.scala Outdated Show resolved Hide resolved

NthPortal commented Aug 6, 2018

View reviewed changes

src/library/scala/collection/immutable/LazyList.scala Outdated Show resolved Hide resolved

NthPortal requested review from julienrf and szeiger August 6, 2018 05:35

allanrenucci reviewed Aug 6, 2018

View reviewed changes

julienrf reviewed Aug 6, 2018

View reviewed changes

src/library/scala/collection/immutable/LazyList.scala Outdated Show resolved Hide resolved

NthPortal force-pushed the topic/lazyList/PR branch from b4dc368 to 3aebd91 Compare August 6, 2018 19:10

SethTisue self-assigned this Aug 7, 2018

NthPortal changed the title ~~[WIP] Make LazyList fully lazy [ci: last-only]~~ Make LazyList fully lazy Aug 7, 2018

NthPortal removed the WIP label Aug 7, 2018

SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Aug 7, 2018

NthPortal force-pushed the topic/lazyList/PR branch from e5c2060 to 5d18cca Compare August 7, 2018 04:22

NthPortal added 2 commits August 9, 2018 11:34

Stop Stream from sharing implementation with LazyList

22187d4

Extract Stream into its own file, and inline methods and implementation into Stream or its companion object.

Redesign LazyList to be fully lazy

5a985e9

Remove LazyListOps and LazyListFactory. Change LazyList to have a lazy state. The state can be either a cons cell with a lazy head, or an empty state.

NthPortal force-pushed the topic/lazyList/PR branch from bd62c88 to 5a985e9 Compare August 9, 2018 15:35

Ichoran approved these changes Aug 9, 2018

View reviewed changes

julienrf reviewed Aug 10, 2018

View reviewed changes

julienrf approved these changes Aug 10, 2018

View reviewed changes

SethTisue added the release-notes worth highlighting in next release notes label Aug 10, 2018

SethTisue merged commit a2213ce into scala:2.13.x Aug 10, 2018

NthPortal deleted the topic/lazyList/PR branch August 10, 2018 21:03

SethTisue mentioned this pull request Aug 15, 2018

LazyList.fill and LazyList.tabulate aren't lazy in the head scala/bug#11083

Closed

2 tasks

SethTisue changed the title ~~Make LazyList fully lazy~~ Make LazyList's empty-or-not status lazy too Aug 22, 2018

NthPortal mentioned this pull request Aug 26, 2018

Fix LazyList and add tests #7139

Merged

NthPortal mentioned this pull request Sep 3, 2018

Update documentation for LazyList scala/bug#11131

Closed

1 task

NthPortal mentioned this pull request May 20, 2020

Overhaul documentation of LazyList #9000

Merged

Make LazyList's empty-or-not status lazy too #7000

Make LazyList's empty-or-not status lazy too #7000

Conversation

NthPortal commented Aug 4, 2018 • edited Loading

NthPortal commented Aug 6, 2018

julienrf commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthPortal commented Aug 6, 2018

SethTisue commented Aug 7, 2018

NthPortal commented Aug 7, 2018

SethTisue commented Aug 7, 2018

NthPortal commented Aug 7, 2018

NthPortal commented Aug 7, 2018

dwijnand commented Aug 7, 2018

julienrf commented Aug 7, 2018

javax-swing commented Aug 7, 2018 • edited Loading

NthPortal commented Aug 7, 2018

julienrf commented Aug 8, 2018 • edited Loading

NthPortal commented Aug 9, 2018

javax-swing commented Aug 9, 2018 • edited Loading

lrytz commented Aug 9, 2018

NthPortal commented Aug 9, 2018

NthPortal commented Aug 9, 2018

Ichoran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrf commented Aug 10, 2018

SethTisue commented Aug 10, 2018

hrhino commented Aug 10, 2018

NthPortal commented Aug 10, 2018 • edited Loading

SethTisue commented Aug 13, 2018

NthPortal commented Aug 13, 2018

NthPortal commented Aug 13, 2018

SethTisue commented Aug 13, 2018 • edited Loading

SethTisue commented Aug 17, 2018

NthPortal commented Aug 4, 2018 •

edited

Loading

julienrf commented Aug 6, 2018 •

edited

Loading

javax-swing commented Aug 7, 2018 •

edited

Loading

julienrf commented Aug 8, 2018 •

edited

Loading

javax-swing commented Aug 9, 2018 •

edited

Loading

NthPortal commented Aug 10, 2018 •

edited

Loading

SethTisue commented Aug 13, 2018 •

edited

Loading