Significantly improve the performance of `Collection<FutureType>.flatten

[MrMage]

This further halves the runtime in vapor/template-kit#50 from 0.1s to 50ms (Release mode), because submitting several blocks to the run loop for every single array element can be fairly inefficient.

I'd expect this to tremendously speed up template rendering as well (results forthcoming).

Example Profile result before this optimization:

And afterwards:

[MrMage]

Added a benchmark:

[MrMage]

Sorry, that benchmark was compiled in Debug mode. Here's the Release variant:

[MrMage]

I just noticed that this is essentially reverting 439d6dc#diff-945d1a38da3eef0446f42a82a64b7ead, however all tests still pass. This PR should not be affected by the issue described in #184 because we are ensuring that each element is acted upon on the same event loop.

In addition, the 3x performance gain from this PR is too important to pass up, given that flatten is actually becoming a bottle neck in several almost-synchronous parts (i.e. we use futures, but almost all of them are fulfilled right away) of Vapor (e.g. vapor/template-kit#50). (We still need to maintain correctness, of course.)

@adtrevor, would you mind taking a look at this PR?

[calebkleveter]

The .flatten method was recently changed in this PR because it was found that it had some threading issue. You might want to make sure this PR doesn't re-introduce them.

[PopFlamingo]

@MrMage Everything looks correct to me! :)

[MrMage]

The .flatten method was recently changed in this PR because it was found that it had some threading issue. You might want to make sure this PR doesn't re-introduce them.

See #199 (comment) :-)

[calebkleveter]

@MrMage I guess we posted at about the same time 😄. Sounds good!

[tanner0101]

This LGTM, thanks for looking into this.

Just to double check, would it be possible for us to use whenAllSucceeded like we are doing for Vapor 4 in NIOKit?

https://github.com/vapor/nio-kit/blob/master/Sources/NIOKit/EventLoop/EventLoop%2BFlatten.swift#L12

If not, then let's merge this.

[MrMage]

Just to double check, would it be possible for us to use whenAllSucceeded like we are doing for Vapor 4 in NIOKit?

No idea about the performance of whenAllSucceed. I think we should merge this PR now, and when upgrading Core to NIO2, benchmark the performance of whenAllSucceed vs. this implementation before using whenAllSucceed.

Or will this Core.flatten method simply be removed with Vapor 4 in favor of NIOKit.flatten? If so, we should still benchmark the performance of NIOKit.flatten vs. Core.flatten.

[calebkleveter]

@MrMage Core is being remove from Vapor 4, so you would go with the second option. But in reality, we won't be updating Core to NIO 2 because Vapor 3 uses NIO 1.

[MrMage]

@MrMage Core is being remove from Vapor 4, so you would go with the second option. But in reality, we won't be updating Core to NIO 2 because Vapor 3 uses NIO 1.

Thanks for the info! That makes sense. I think we should benchmark NIOKit's flatten implementation then.

[MrMage]

Update: no need for changes in NIOKit; I hope to upstream these into NIO with apple/swift-nio#943.

[tanner0101]

Thanks @MrMage !

[penny-coin]

Hey @MrMage, you just merged a pull request, have a coin!

You now have 19 coins.