Pass multiplexer from ConnectionManager rather than channel #1062
Conversation
Motivation: The multiplexer is what's actually required and is expensive to look up from the channel. Modifications: Pass the multiplexor through the Idle Handler and the ConnectionManager to the requester. Update tests to match. Result: Approximately 3% faster on unary calls.
| channel = state.readyChannelPromise.futureResult | ||
| switch self.state { | ||
| case .idle: | ||
| self.startConnecting() |
There was a problem hiding this comment.
Would it be easier for startConnecting to just return the future you want?
There was a problem hiding this comment.
Probably not as in getHTTP2MultiplexerOptimistic the one you want is different. I guess the logic could be pushed in but then the decision point wouldn't match for the other cases.
Sources/GRPC/ConnectionManager.swift
Outdated
| internal struct IdleState { | ||
| var configuration: ClientConnection.Configuration | ||
| } | ||
| internal struct IdleState {} |
There was a problem hiding this comment.
May as well delete this type instead.
Sources/GRPC/ConnectionManager.swift
Outdated
| } | ||
|
|
||
| /// Returns a future for the current channel, or future channel from the current connection | ||
| /// Returns a future for the current channel multiplexer, or future channel multiplexer from the current connection |
There was a problem hiding this comment.
nit: let's call it an HTTP/2 stream multiplexer rather than a channel multiplexer
Sources/GRPC/ConnectionManager.swift
Outdated
| // Unexpected to get here. | ||
| self.invalidState() | ||
| case .connecting: | ||
| // Unexpected to get here. |
There was a problem hiding this comment.
nit: add a comment saying why.
Sources/GRPC/ConnectionManager.swift
Outdated
|
|
||
| case let .active(state): | ||
| channel = state.candidate.eventLoop.makeSucceededFuture(state.candidate) | ||
| case .active: |
There was a problem hiding this comment.
nit: might as well collapse the remaining states: case .active, .ready, .transientFailure, .shutdown:
Sources/GRPC/ConnectionManager.swift
Outdated
| self | ||
| .state = | ||
| .active(ConnectedState(from: connecting, candidate: channel, multiplexer: multiplexer)) |
There was a problem hiding this comment.
swiftformat makes some real beauties sometimes... can we have something like the following instead:
let connected = ConnectedState(from: ...)
self.state = .active(connected)| channel: channel, | ||
| inboundStreamInitializer: nil | ||
| ) | ||
| _ = channel.pipeline.addHandler( |
|
I believe I've fixed all your comments George. |
|
@glbrntt Notice the sneaky extra commit. |
|
@PeterAdams-A you're hitting an invalid state: |
Sources/GRPC/ConnectionManager.swift
Outdated
| promise.fail(state.reason) | ||
| } | ||
| } | ||
|
|
||
| return self.eventLoop.flatSubmit { |
There was a problem hiding this comment.
Since getHTTP2Multiplexer ensures we're on the event loop we no longer need to submit the work to it
Sources/GRPC/ConnectionManager.swift
Outdated
| connecting.candidate | ||
| .whenComplete { _ in fulfillMuxPromiseGivenState(promise: muxPromise) } |
There was a problem hiding this comment.
I'm not sure if this is correct: the channel future can complete before we receive channel active, i.e. we can still be in the connecting state when fulfilling the promise.
There was a problem hiding this comment.
This turned out to be quite a big change involving adding an extra promise to state.
There was a problem hiding this comment.
This looks good to me, thanks @PeterAdams-A! I added a couple of suggestions to fix pre-existing typos.
Motivation:
The multiplexer is what's actually required and is expensive to
look up from the channel.
Modifications:
Pass the multiplexor through the Idle Handler and the ConnectionManager
to the requester.
Update tests to match.
Result:
Approximately 3% faster on unary calls.