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

Use the pre-succeeded void future when possible for async pipeline operations #1766

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Feb 24, 2021

Motivation:

We added synchronous pipeline operations to allow the caller to save
allocations when they know they are already on the correct event loop.
However, we missed a trick! In some cases the caller cannot guarantee
they are on the correct event loop and must use an asynchronous method
instead. If that method returns a void future and is called on the event
loop, then we can perform the operation synchronously and return a
cached void future.

Modifications:

  • Add API to EventLoop for creating a 'completed' future with a
    Result (similar to EventLoopPromise.completeWith)
  • Add an equivalent for making completed void futures
  • Use these when asynchronously adding handlers and the caller is
    already on the right event loop.

Result:

  • Fewer allocations on the happiest of happy paths when adding handlers
    asynchronously to a pipeline.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this, I'd like to let @weissi review the API additions as well.

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very cool! We just have some extra parameters that (I think) we don't need :)

Sources/NIO/EventLoop.swift Outdated Show resolved Hide resolved
Sources/NIO/EventLoop.swift Outdated Show resolved Hide resolved
/// - Parameters:
/// - result: The value that is used by the `EventLoopFuture`
/// - Returns: A completed `EventLoopFuture`.
public func makeCompletedVoidFuture(_ result: Result<Void, Error>) -> EventLoopFuture<Void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So technically, we don't need these two methods. We can just use the fully generic one and do:

if Success.self == Void.self {
    // code for makeCompletedVoidFuture here
} else {
   // code for makeCompletedFuture here
}

and because of the @inlinable we get the same perf and will have a nicer API. WDYT @Lukasa / @glbrntt ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't even need to do that I think, makeCompletedFuture does it for us :)

…erations

Motivation:

We added synchronous pipeline operations to allow the caller to save
allocations when they know they are already on the correct event loop.
However, we missed a trick! In some cases the caller cannot guarantee
they are on the correct event loop and must use an asynchronous method
instead. If that method returns a void future and is called on the event
loop, then we can perform the operation synchronously and return a
cached void future.

Modifications:

- Add API to `EventLoop` for creating a 'completed' future with a
  `Result` (similar to `EventLoopPromise.completeWith`)
- Add an equivalent for making completed void futures
- Use these when asynchronously adding handlers and the caller is
  already on the right event loop.

Result:

- Fewer allocations on the happiest of happy paths when adding handlers
  asynchronously to a pipeline.
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay! thank you George!

@weissi weissi merged commit 178636d into apple:main Feb 25, 2021
@glbrntt glbrntt deleted the gb-save-some-allocs branch February 25, 2021 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants