Seems like this should be backported to 2.12.x

@hepin1989 The 2.12.x tree does not use the same implementation as 2.13.x. I'd have to run benches for 2.12.x separately.

I should note that this also applies to any ECs created using the createDefaultExecutorService machinery.

@SethTisue Let me know what is needed to get this merged. Would love to see this in before rc1

There are some ~15% regressions for CallbackFutureBenchmark and FirstCompletedOfFutureBenchmark... Will they be mitigated in future?

@plokhotnyuk I hope to be able to bring them back to around where they were for 2.13.0-M5. As for The lower performance for CallbackBenchmark is due to how the bench is written, as the bench prefers as much parallelism as possible (one can argue that this is a faulty test but given that it is a callback it can only do side-effects). As for the regression of FirstCompletedOfBenchmark this is due to the fact that FirstCompletedOf is an inherent race and does not benefit from trampolining. I'll see if I can do something about that.

Given the impact for the typical use-cases of stacking transformations I think the improvements for global is significant. (Remember that the change does not affect all ECs)

@SethTisue Let me know what is needed to get this merged

well, in scala.concurrent we consider you the primary arbiter of that, actually :-)

I can run the community build on it if you like (though the 2.13 build has ~70 projects currently, compared to ~180 on 2.12, so it's somewhat less assurance)

Would love to see this in before rc1

(RC1 won't be out before January, so there's time)

@SethTisue :) I'm always uneasy about approving my own work!
I'm investigating how to regain some performance for some of the operators, so let's just keep this open a week or so.

@plokhotnyuk I've made some extra optimizations and we're almost back to HEAD in performance of FirstCompletedOf:

[info] FirstCompletedOfFutureBenchmark.post     gbl         8192          1  thrpt   10  0.975 ± 0.009  ops/ms
[info] FirstCompletedOfFutureBenchmark.pre      gbl         8192          1  thrpt   10  0.934 ± 0.010  ops/ms

(We've got about 10% to go, will see if I can improve that further, but it is doing more work in that case so it is hard to regain everything, but I'll try)

For CallbackFutureBenchmark I'm at:

[info] CallbackFutureBenchmark.post     gbl         8192          1  thrpt   10  0.845 ± 0.007  ops/ms
[info] CallbackFutureBenchmark.pre      gbl         8192          1  thrpt   10  0.935 ± 0.007  ops/ms

Which means the post-bench is back to baseline and the prebench has about 10% to go. Will look into this some more.

@plokhotnyuk In the pre-case it is really difficult to claw back extra performance because the 10% hit is the extra allocation for the batch, which is always discarded after only having had a single item in it. I will see what the effect is when the benchmark is executed from within a Future.

@plokhotnyuk When the CallbackFutureBenchmark is executed from within foreach of an already completed Future on the same EC then the bench is 4-5x faster, which I think is a pretty nice price to pay for 10% reduction in performance if performed from the "outside":

[info] CallbackFutureBenchmark.post gbl 8192 1 thrpt 10 4.014 ± 0.042 ops/ms
[info] CallbackFutureBenchmark.pre gbl 8192 1 thrpt 10 4.015 ± 0.022 ops/ms

I'll see when I can do to make that 10% perf gap even smaller, but it does seem like a trade-off situation.

@viktorklang .pre is the performance of the operation on a Future from a Promise which is already completed, and .post is the performance of the operation on a Future from a Promise which is not yet completed -- is that correct?

@plokhotnyuk

If I add a method to Batchable, and let each batchable decide whether it is ready to be batched or not, then I can mark foreach and onComplete as non-batchable, but the downside is that 4-5x speedup when executed in a nested fashion is almost completely lost (only about 30% left), and then we regain the baseline performance (even adds a bit too):

// Running the benchmark as usual
[info] CallbackFutureBenchmark.post     gbl         8192          1  thrpt   10  0.937 ± 0.009  ops/ms
[info] CallbackFutureBenchmark.pre      gbl         8192          1  thrpt   10  1.003 ± 0.007  ops/ms

// Running the benchmark within a precompleted future on the same EC
[info] CallbackFutureBenchmark.post     gbl         8192          1  thrpt   10  1.345 ± 0.004  ops/ms
[info] CallbackFutureBenchmark.pre      gbl         8192          1  thrpt   10  1.300 ± 0.032  ops/ms

I added a bit of optimization to FirstCompletedOf which gives a pretty significant boost in the precompleted section:

[info] FirstCompletedOfFutureBenchmark.post     gbl         8192          1  thrpt   10   1.223 ± 0.007  ops/ms
[info] FirstCompletedOfFutureBenchmark.pre      gbl         8192          1  thrpt   10  22.503 ± 1.127  ops/ms

@NthPortal Exactly. pre = precompleted (completed before the operations are added), post = postcompleted (completed after the operations are added)

@viktorklang do we care about one of those more than the other (perhaps because it is more common)?

@NthPortal It's use-case specific and timing-related (as the futures could be completed either before, during, or after)

@NthPortal @plokhotnyuk On second thought, I'd prefer not to introduce the new method to Batchable since it is likely to create difficult-to-reason about performance implications for end-users.
So I'll add the rest of the optimizations I added as a part of this PR, and then I can always special-case Future in the global EC's batching-decisions if there are any problems for end-users, down the line since that change would be completely binary compatible and hidden from user code.

I just added some improvements. Will update the text of the PR when I have cleaned up the bench results.

@NthPortal @SethTisue Alright, I believe this one is ready for community build and then RC1 :)

https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1635/

it finished at https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1637/ , I see only the usual failures.

Thanks @SethTisue !

I just pushed a new commit which should improve performance a bit further.
There's a new implementation of BatchingExecutor.Batch which only allocates an array for elements if it gets more than 1 element, this reduces the cost for operations which only submits a single Runnable per batch.

@SethTisue Could you run the community build against this latest commit. I believe it should be as good as it gets right now.

@SethTisue Found even more optimizations! Looking really good now!

Just updated the PR with the latest perf numbers compared to the 2.13.x mainline—some operators have dramatically better performance on the Global EC with these changes.

@SethTisue I'm merging this now. I've squashed the commits. Let me know if/when you run the next community-build on the 2.13.x mainline.

Let me know if/when you run the next community-build on the 2.13.x mainline.

I haven't seen any regressions since scala/community-build@ee79f0f

Is there a reason why is the BatchedExecutor trait is private[concurrent]? Are users supposed to copy and paste that code or make their own if they need a separate ExecutionContext, e.g. to avoid blocking the global pool, but want it to not have worse performance?

@shado23 I wasn't sure it should be supported end-user API. If you want to avoid blocking the global pool then you can use blocking{ … }—this will use ManagedBlocker up to the configured maximum extra threads.

@viktorklang Thanks for the reply. I hear what you're saying, but I think there are cases where having a truly separate ExecutionContext is advantageous. Slick for instance does exactly that for its JDBC IO threads. In my experience the blocking context behavior can be a cure worse than the disease. Yes, you may avoid thread starvation, but can in return end up with thousands(!) of threads and an OOM exception if you're not very careful about how much work you schedule (which Future doesn't make particularly easy). In the cases I've run into it was always easier and safer to provide a right sized thread pool from the get go, because even if you happen to avoid the OOM issue your system won't be able to deal with an infinite amount of concurrent IO ops anyway and it could even be that you're dealing with background jobs that you don't want to take up a significant amount of resources. Even limiting the extra threads of the global pool isn't the best solution, as you still end up mixing IO and computation work, which has quite different needs, and without control of the scheduling you can still end up blocking all of them. Ultimately even pure computational tasks can look like blocking to the rest of the system if they're heavy enough, so even there you have to put thought into how and where they are executed.

I do think that having the executor be batching by default is a good change, if I'm not mistaken it essentially gives you the behavior that Twitter Future and Monix Task (and probably others) already have where a simple map doesn't immediately incur a trip through the ExecutionContext and associated context switch in most cases. I think that's great, because you currently have to contort yourself to try to avoid this if your transformations are small or accept a significant performance loss.

Ultimately, I think it would be useful to allow users to create their own BatchedExecutor. It doesn't seem like it would expose any delicate internals or be very risky. I understand that binary compatibility concerns etc. create an incentive to be careful about the public API you expose, but in some cases it can also be quite annoying as a user. In this case copy and pasting the implementation wouldn't be the worst thing in the world, but it feels petty to me to disallow external usage if there aren't good reasons to do it. (Maybe there are?)

@shado23

Yes, you may avoid thread starvation, but can in return end up with thousands(!) of threads and an OOM exception if you're not very careful about how much work you schedule (which Future doesn't make particularly easy).

The global default EC has a default number of extra threads of 256 (configurable via System Properties)—this means that if you use blocking{} you are able to limit impact. Speaking of that, it would also be possible to add a configuration parameter to, instead of executing the blocking logic, throw an exception. This would preserve liveness. It is also possible to install your own BlockContext which does that.

Ultimately, I think it would be useful to allow users to create their own BatchedExecutor. It doesn't seem like it would expose any delicate internals or be very risky

The biggest issues I see is that users/extenders of BatchingExecutor will not realize the implications on the code it executes—which is why the synchronous EC used by Future is not exposed directly to end users.

Also note that you are able to create a "clone" of the global EC by calling this: https://github.com/scala/scala/blob/2.13.x/src/library/scala/concurrent/ExecutionContext.scala#L153

I have some additional optimizations to BatchingExecutor that I hope I'll be able to ship with 2.13.0-RC1.

The global default EC has a default number of extra threads of 256 (configurable via System Properties)—this means that if you use blocking{} you are able to limit impact. Speaking of that, it would also be possible to add a configuration parameter to, instead of executing the blocking logic, throw an exception. This would preserve liveness. It is also possible to install your own BlockContext which does that.

Sure, in many cases that will probably work well enough. To be really seamless and safe most likely requires some cooperation from the VM to allow avoiding blocking IO (such as what Go does). Maybe project Loom will provide something here in the future.

Also note that you are able to create a "clone" of the global EC by calling this: https://github.com/scala/scala/blob/2.13.x/src/library/scala/concurrent/ExecutionContext.scala#L153

That's good enough for me then.

I have some additional optimizations to BatchingExecutor that I hope I'll be able to ship with 2.13.0-RC1.

Sounds good. I'm looking forward to being able to take advantage of all the great optimization work you're doing.

@shado23 Thanks!