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

Add a pooled channel #1284

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Add a pooled channel #1284

merged 2 commits into from
Oct 1, 2021

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Sep 30, 2021

Motivation:

We added the various internals of the connection pool but are yet to
surface it as public API. This PR does just that.

Modifications:

  • Add a PooledChannel, a GRPCChannel which uses the connection pool
  • Add GRPCPooledChannel and GRPCPooledChannel.Configuration which
    produces PooledChannels as GRPCChannels.
  • Add a handful of tests

Result:

We have a usable connection pool in the public API.

@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Sep 30, 2021
@glbrntt glbrntt requested a review from Lukasa September 30, 2021 15:16
@glbrntt
Copy link
Collaborator Author

glbrntt commented Sep 30, 2021

I have a follow up PR with some allocation counting tests.

Copy link
Collaborator

@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 have one minor note regarding inlinability but otherwise I think this patch is fantastic, and I'm really excited to have this functionality available in grpc-swift.

/// Default HTTP/2 configuration.
public static let defaults = HTTP2()

public static func with(_ configure: (inout HTTP2) -> Void) -> HTTP2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

All your various with functions here should probably aim to be @inlinable.

Motivation:

We added the various internals of the connection pool but are yet to
surface it as public API. This PR does just that.

Modifications:

- Add a `PooledChannel`, a `GRPCChannel` which uses the connection pool
- Add `GRPCPooledChannel` and `GRPCPooledChannel.Configuration` which
  produces `PooledChannel`s as `GRPCChannel`s.
- Add a handful of tests

Result:

We have a usable connection pool in the public API.
@glbrntt
Copy link
Collaborator Author

glbrntt commented Oct 1, 2021

Unfortunately we regress 1 alloc per RPC but I think it's worth it since switching to the pooled channel allocates quite a bit less and should be preferred.

@glbrntt glbrntt merged commit e180dfa into grpc:main Oct 1, 2021
@glbrntt glbrntt deleted the gb-pooled-client branch October 1, 2021 10:50
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.

2 participants