await() on already-completed future could return value synchronously

[danarmak]

await(future) always compiles to future.onComplete(...), even if the future is already completed. (I think so based on the output of scalac --Ymacro-debug-lite in the 2.10 branch.) Why not return the value synchronously if it is already available?

Here's a simple implementation as a macro wrapping await():

  def fastAwait[T](fut: Future[T]): T = macro fastAwaitImpl[T]

  def fastAwaitImpl[T: c.WeakTypeTag](c: Context)(fut: c.Expr[Future[T]]): c.Expr[T] = {
    import c.universe._
    val tree =  q"{ val f = $fut; if (f.isCompleted) f.value.get.get else await(f) }"
    c.Expr[T](tree)
  }

I have code that passes very large amounts of short-lived Futures around. Many of these are already completed when awaited. This optimization helps (from initial tests) and I'm considering using it in my code - is there any reason not to do this?

[phaller]

I agree this should be optimized. I'd suggest to do it within the main macro, though. Dropping the dependency on quasiquotes would be good to simplify the 2.10 build.

[danarmak]

I used a separate macro just as a feasibility test. If I have time I'll try to make a PR that changes the main macro.

[phaller]

Ok, great. I'd be happy to review your PR if you have time. Thanks!

[danarmak]

@phaller, changing the main macro will make this a user-observable semantic change. Right now await is guaranteed to be asynchronous (at least, depending on the EC), but with this change it can be synchronous. This could theoretically break some user code that relies on this side effect to ensure code following await is async from the code invoking it.

Is that desirable? Would it require updating SIP 22?

[danarmak]

Please comment, otherwise I won't work on the PR because I fear breaking the SIP-22 contract.

[gabriel-bezerra]

I agree with you about avoiding semantic changes in the interface.

This is what the API doc says about onComplete:

If the future has already been completed, this will either be applied immediately or be scheduled asynchronously.

http://www.scala-lang.org/api/current/index.html#scala.concurrent.Future

I tried to find if it is already done in the existing Futures' API and the closest I could get in the current implementation was this line:
https://github.com/scala/scala/blob/v2.11.1/src/library/scala/concurrent/impl/Promise.scala#L267

It seems to always run in the ExecutionContext, even if the Future is already completed, because this method is always called:
https://github.com/scala/scala/blob/v2.11.1/src/library/scala/concurrent/impl/Promise.scala#L40

[danarmak]

Future.onComplete is much older and more widely used than scala-async, so I wouldn't propose such a change in behavior even if the API doc allowed it. But async/await is young enough to consider making even breaking changes.

But since it's the official implementation of an accepted SIP, it would require another SIP to change (or whatever the amendment procedure is). So I'd like to know what the SIP guarantees.

[retronym]

It is a pending SIP. We've still got some wiggle room to tweak these things.

[danarmak]

@gabriel-bezerra there's another reason why we can't change onComplete to be synchronous: it will cause code that calls onComplete in a loop to have stack overflows! The await macro has a synchronous-looking signature, so it seems not be vulnerable to this.

[danarmak]

@retronym then I'll try to find time to make a PR changing the existing macro.

[stanch]

Hi,

On Android, where there is a significant distinction between the UI thread and all other threads, I use a UiThreadExecutionContext to schedule things on the UI thread. In particular, I have mapUi, foreachUi, ..., xxxUi extension methods that call to map, foreach, ..., xxx and specify UiThreadExecutionContext:

Future {
  // background work
} mapUi { r ⇒
  // UI work
}

Changing onComplete semantics in Future will obviously break this approach (but there are workarounds, such as redirecting mapUi to flatMap over a correctly scheduled Future). But as I understand from the discussion, this change is not planned anyway.

Regarding scala-async, I also use this pattern to ensure that all code between await calls will run on the UI thread:

async {
  // UI thread
  await(...)
  // still UI thread
  await(...)
  // UI thread again
}(UiThreadExecutionContext)

Here the situation is a bit different, since a synchronous call to await will keep us in the same thread as before, which we know to always be the UI thread. Am I correct? I guess I’m trying to establish that the thread used between the calls to await is an invariant and the proposed change does not affect it.

[danarmak]

@stanch the proposed change is to make await either return synchronously if the future is already complete, or to behave the same way it has until now (i.e. call onComplete). If all you care about is to make all code run on a particular EC, that will keep happening.

[stanch]

@danarmak Yes, that’s what I thought. Thanks for confirming.

[danarmak]

I tried to make the PR, but the code was much bigger and more complex than I expected. I'm afraid I won't have time to work on it, since the simple wrapper macro satisfies my own needs.

If you're not going to do it yourselves, please feel free to close this ticket.

[danarmak]

I've encountered an interesting problem while using my fastAwait macro wrapping await. It seems the underlying async macro compiles all trees that contain a call to await as state machine transitions which eat up stack frames even if await is never actually called.

The following code fails with a StackOverflowException:

async {
  while (true) {
    if (false) {
      ???; fastAwait(Future.successful(()))
    }
  }
}

The if is of course never entered. And yet several stack frames are taken up by each iteration of the while loop.

Commenting out the call to fastAwait removes the stack overflow.

[danarmak]

Actually, I just realized this has nothing to do with fastAwait - the base async macro has this problem. I'll open a separate ticket.

[retronym]

Good catch! I'd be interested to figure out how the C# translation of await/async avoids this. Perhaps we ought to detect if the next state transition is on the stack of previously visited states, and return from resume, rather than recursing.

[danarmak]

@retronym I opened ticket #93.

[scott-abernethy]

@retronym Fantastic. I just tested out the 0.9.3 snapshot and this little optimisation gives a significant performance improvement for my channels implementation (it often awaits on essentially Future.successful(x)).

Over 3x faster on a sample test (410K messages/sec now compared to 120K messages/sec prior to the patch.

Thanks!