[libs] Promise linking is unnecessary

[Piotr Tarsa]

Hi guys,

Sorry for the clickbait title but I have an actually working code already ;) The project demonstrating that the problem of leaking memory can be solved by simply extracting closure to separate method (thus avoiding accidental closure over flatMap/ recoverWith parameters) is here: https://github.com/tarsa/FuturesFix

I was trying hard to understand why we were leaking memory in scenarios as Viktor Klang described in: https://github.com/viktorklang/blog/blob/master/Futures-in-Scala-2.12-part-9.md but the only explanation that came to my mind was that the Scala compiler was simply closing over too many variables, so helping the compiler by extracting the closure to separate method should fix the problem. My experiments shown that it was indeed the case. If you go to that project I've mentioned ( https://github.com/tarsa/FuturesFix ) there's a file FuturesChainingTests.scala which contains the regression tests that demonstrate effectiveness of promise linking and my fix on flatMap and recoverWith.

My fix is pretty small but profound one. What was originally:

def flatMap[S](f: T => Future[S])(implicit executor: ExecutionContext): Future[S] = {
val p = Promise[S]()
onComplete {
case f: Failure[_] => p complete f.asInstanceOf[Failure[S]]
case Success(v) => try f(v) onComplete p.complete catch { case NonFatal(t) => p failure t }
}
p.future
}

I've changed to:

def flatMap[S](f: T => Future[S])(implicit executor: ExecutionContext): Future[S] = {
val p = Promise[S]()
onComplete {
case f: Failure[_] => p complete f.asInstanceOf[Failure[S]]
case Success(v) => try f(v) onComplete feedPromise(p) catch { case NonFatal(t) => p failure t }
}
p.future
}

private def feedPromise[S](promise: Promise[S]) =
promise.complete(_: Try[S])

The change is from "p.complete" to "feedPromise(p)" in the line that starts from "case Success(v)"

Regards,

Piotr

[Naftoli Gugenheim]

You can open a pull request

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Viktor Klang]

Hi Piotr!

Thanks for reading my blog post, and for wanting to help make Futures better; I really appreciate it!

This response is still a bit early sine I haven't had my morning espresso yet, alas I'd like to make some preliminary comments:

First of all—if `feedPromise` indeed lessens the variables captured by the closure generated, I'd personally consider it a deficiency of the Scala compiler. (Compiler closing over things which are not used in the closure). Have you verified what gets captured in either case?

Perhaps the regression test for the promise linking is too naïve—it verifies the absence of a leak by tracking closed values, but what it should be doing is to also track living (Default)Promise-instances. Because even with `feedPromise`, we're still building up a chain of Promises, which is exactly the thing that promise linking is designed to avoid.

I assume you've seen it already but I think the following comment is pretty good:

https://github.com/viktorklang/scala-futures/blob/master/src/main/scala/scala/future/impl/Promise.scala#L23

--

You received this message because you are subscribed to the Google Groups "scala-internals" group.

To unsubscribe from this group and stop receiving emails from it, send an email to scala-internals+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Cheers,

√

[Piotr Tarsa]

Hi Viktor,

I've disassembled the inner closure (inner onComplete) compiled with Scala 2.11 and scalac compiles it to non-static inner class that holds reference to the enclosing closure (outer onComplete) and that enclosing closure holds reference to the function that was passed to flatMap. That obviously creates unnecessary memory leak as the inner closure doesn't need the referenece to the whole enclosing closure, it only needs access to promise that is returned from flatMap.

Nevertheless, compiling source code taken from before implemeting promise linking and compiling it with Scala 2.12 resulted in bytecode that didn't have that memory leak and therefore `feedPromise` wasn't needed to pass the tests that you've described in your blog.

But as you've said, even without unnecessary closure over functions passed to flatMap/ transformWith/ whatever we're still left with strongly referenced promises chain if we don't do promises linking. I've added UnboundedAcpsTests.scala to my project to check that and with small enough -Xmx (eg -Xmx10m) it's easy to see that only with promises linking we're getting a program that happily runs forever without memory space exceptions/ errors.

So I'm back to the point where I don't understand how promise linking solve problems :) But at least the description you've shown on you blog is misleading - the leaking arrays are due to compiler deficiency, not due to promises chaining. Promises linking seems to solve problem of long chains of promises, but we don't need any complicated closures closing over big state to show that.

I have an idea about inverting the direction of promise linking - ie linking from newer promise to older promise. This way we shouldn't need to migrate callbacks over and over as they all would accumulate at the oldest promise. However, I have yet to fully understand how promise linking works in current Scala library to start implementing and testing my own.

Regards,

Piotr

To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Cheers,

√

[Viktor Klang]

Hey Piotr,

On Wed, Feb 8, 2017 at 1:44 AM, Piotr Tarsa <piotr...@gmail.com> wrote:

Hi Viktor,

I've disassembled the inner closure (inner onComplete) compiled with Scala 2.11 and scalac compiles it to non-static inner class that holds reference to the enclosing closure (outer onComplete) and that enclosing closure holds reference to the function that was passed to flatMap. That obviously creates unnecessary memory leak as the inner closure doesn't need the referenece to the whole enclosing closure, it only needs access to promise that is returned from flatMap.

Thanks for verifying.

 

Nevertheless, compiling source code taken from before implemeting promise linking and compiling it with Scala 2.12 resulted in bytecode that didn't have that memory leak and therefore `feedPromise` wasn't needed to pass the tests that you've described in your blog.

Ah, I've not tried that—interesting!

 

But as you've said, even without unnecessary closure over functions passed to flatMap/ transformWith/ whatever we're still left with strongly referenced promises chain if we don't do promises linking. I've added UnboundedAcpsTests.scala to my project to check that and with small enough -Xmx (eg -Xmx10m) it's easy to see that only with promises linking we're getting a program that happily runs forever without memory space exceptions/ errors.

Cool.

 

So I'm back to the point where I don't understand how promise linking solve problems :)

Huh? I thought you just said that «it's easy to see that only with promises linking we're getting a program that happily runs forever without memory space exceptions/ errors.»?

 

But at least the description you've shown on you blog is misleading - the leaking arrays are due to compiler deficiency, not due to promises chaining. Promises linking seems to solve problem of long chains of promises, but we don't need any complicated closures closing over big state to show that.

I'll try coming up with a better example then :)

 

I have an idea about inverting the direction of promise linking - ie linking from newer promise to older promise. This way we shouldn't need to migrate callbacks over and over as they all would accumulate at the oldest promise. However, I have yet to fully understand how promise linking works in current Scala library to start implementing and testing my own.

Looking forward to see what you come up with!

 

To unsubscribe from this group and stop receiving emails from it, send an email to scala-internals+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Cheers,

√

[Piotr Tarsa]

I think I've understood the promise linking now. Before I thought that older promises were pointing to newer promises, but actually it's the other way around, thus my idea about reversing direction of promise linking is invalid.

As for the example, I think that you should remove the leaking arrays as those leaks are related to compiler deficiency, not promises chains. So basically the tests that I've included in UnboundedAcpsTests.scala show the problem accurately - without promise linking we're slowly building up a promise callback chain that can't be garbage collected. But the pace at which the memory usage grows is very low so setting low Xmx (eg to 10m) is crucial as otherwise user would need to wait a long time for memory exhaustion exception.

Regards,

Piotr

[Viktor Klang]

On Thu, Feb 9, 2017 at 12:48 AM, Piotr Tarsa <piotr...@gmail.com> wrote:

I think I've understood the promise linking now. Before I thought that older promises were pointing to newer promises, but actually it's the other way around, thus my idea about reversing direction of promise linking is invalid.

Ah, yes, the "root" nomenclature can be confusing at first.

 

As for the example, I think that you should remove the leaking arrays as those leaks are related to compiler deficiency, not promises chains. So basically the tests that I've included in UnboundedAcpsTests.scala show the problem accurately - without promise linking we're slowly building up a promise callback chain that can't be garbage collected. But the pace at which the memory usage grows is very low so setting low Xmx (eg to 10m) is crucial as otherwise user would need to wait a long time for memory exhaustion exception.

I'm not sure it's possible to tune the -Xmx param for individual tests so we'd need a fastfailing regression tests.

I'll think about it. Perhaps creating a lot of promises in parallel can help take down the time to OOME…

 

To unsubscribe from this group and stop receiving emails from it, send an email to scala-internals+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Cheers,

√