Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue-2598: Fix memory leaks and minimisations. #2602

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

diesalbla
Copy link
Contributor

From #2598, we add a leak test for pure streams, one with flatMap and uncons, which does not involve parallelism.

We then make the changes in pull interpreter that would fix that specific leak test case.

From typelevel#2598, we add
a leak test for pure streams, one with flatMap and uncons,
which does not involve parallelism.

We then make the changes in `pull` interpreter that would
fix that specific leak test case.
@diesalbla diesalbla requested a review from mpilquist September 9, 2021 12:25
@diesalbla diesalbla changed the title Issue_2598_A: Fix pure-streams leak. Issue-2598-A: Fix pure-stream memory leak. Sep 9, 2021
@diesalbla diesalbla changed the title Issue-2598-A: Fix pure-stream memory leak. Issue-2598: Fix memory leaks and minimisations. Sep 9, 2021
We add another incremental fix on the issue memory leak, by addressing
a leak test case that combines onFinalize, flatten, and a drain
(which is a call to uncons). For this we need to restore some
of the code we dealt away on previous PR.
@mpilquist
Copy link
Member

Memory tests pass but looks like we're getting a stack overflow exception on map:

test("regression #2353 - stack safety of map") {
@tailrec
def loop(str: Stream[Pure, Int], i: Int): Stream[Pure, Int] =
if (i == 0) str else loop(str.map((x: Int) => x + 1), i - 1)
loop(Stream.emit(1), 10000).compile.toList //
}

  + map - regression #1335 - stack safety of map 0.004s
java.lang.StackOverflowError
  | => cat fs2.Pull$.fs2$Pull$$viewL$1(Pull.scala:839)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1161)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)
	at fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1190)

@diesalbla
Copy link
Contributor Author

@mpilquist That stack overflow is similar to the one that appears with FlatMap, because the call to go has a left-recursive call to go. Adding a F.unit >> in front of it seems to delay that recursive call.

@mpilquist mpilquist merged commit d9bcdf6 into typelevel:main Sep 10, 2021
@diesalbla diesalbla deleted the fix_flatMapFlatMapLeak branch September 12, 2021 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants