-
Notifications
You must be signed in to change notification settings - Fork 84
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
Define abstraction for multiplexer holding channel. #215
Merged
Lukasa
merged 3 commits into
apple:master
from
Lukasa:cb-channel-multiplexer-abstraction
Jul 28, 2020
Merged
Define abstraction for multiplexer holding channel. #215
Lukasa
merged 3 commits into
apple:master
from
Lukasa:cb-channel-multiplexer-abstraction
Jul 28, 2020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: As part of the work in apple#214 we need a way for the HTTP2StreamMultiplexer to hold multiple kinds of channels. It would be best if doing so didn't require substantial changes to HTTP2StreamMultiplexer, which is difficult because right now it is tightly coupled to HTTP2StreamChannel. There are a few ways we could do this. The most obvious option to most Swift programmers would be to define a protocol that covers the surface area we want to use, and then hold that. This works fairly well, but the downside of it is that it will force all calls from HTTP2StreamMultiplexer to become dynamically dispatched. These indirect calls are optimisation boundaries and so provide some performance costs. Another option would be to create a subclassing relationship. This is ok, but it also forces us into the realm of dynamic dispatch on some of these calls, and also provides general difficulty if we ever want to provide a backing channel that isn't easily made into a subclass. The remaining option is to provide an abstract object that uses an `enum` to store the kinds of channel we know about, and jump through it. This works really well for small numbers of channel types (1 and 2 channels being the best outcomes), as it introduces very few extra branches which are highly predictable, as well as allowing direct function calls. This is great: one predictable branch that cannot possibly miss cache, followed by a direct function call. Joy of joys! However, it would be best if we didn't tie ourselves into this implementation strategy. After all, down the road we could well end up with more kinds of channels, and at a certain point the cost of the indirect function call becomes _lower_ than the cost of jumping through the enumeration. For that reason, we should define an abstraction. Enter: `MultiplexerAbstractChannel`. This concrete type defines the interface that `HTTP2StreamMultiplexer` will use to interact with stream channels. As it's internal, the abstraction is totally porous: Swift is capable of optimising it away to nothing. This type is explicitly a reference type, maintaining the semantic expected by `HTTP2StreamMultiplexer`. This provides a nice encapsulation over our abstraction. `HTTP2StreamMultiplexer` never needs to know about how we abstract over multiple channel types: the entire abstraction lives in a single, simple file, and can easily be rewritten without difficulty. Modifications: - Define `MultiplexerAbstractChannel` and the interface expected by `HTTP2StreamMultiplexer`. - Define a `BaseChannel` `enum` that will be used to dispatch to multiple `Channel` types. - Rewrite `HTTP2StreamMultiplexer` to use this new abstraction. Result: A customisation point for new channels will exist.
8 tasks
glbrntt
approved these changes
Jul 28, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one copyright nit.
// | ||
// This source file is part of the SwiftNIO open source project | ||
// | ||
// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors | |
// Copyright (c) 2020 Apple Inc. and the SwiftNIO project authors |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
As part of the work in #214 we need a way for the HTTP2StreamMultiplexer
to hold multiple kinds of channels. It would be best if doing so didn't
require substantial changes to HTTP2StreamMultiplexer, which is
difficult because right now it is tightly coupled to HTTP2StreamChannel.
There are a few ways we could do this. The most obvious option to most
Swift programmers would be to define a protocol that covers the surface
area we want to use, and then hold that. This works fairly well, but the
downside of it is that it will force all calls from
HTTP2StreamMultiplexer to become dynamically dispatched. These indirect
calls are optimisation boundaries and so provide some performance costs.
Another option would be to create a subclassing relationship. This is
ok, but it also forces us into the realm of dynamic dispatch on some of
these calls, and also provides general difficulty if we ever want to
provide a backing channel that isn't easily made into a subclass.
The remaining option is to provide an abstract object that uses an
enum
to store the kinds of channel we know about, and jump through it.This works really well for small numbers of channel types (1 and 2
channels being the best outcomes), as it introduces very few extra
branches which are highly predictable, as well as allowing direct
function calls. This is great: one predictable branch that cannot
possibly miss cache, followed by a direct function call. Joy of joys!
However, it would be best if we didn't tie ourselves into this
implementation strategy. After all, down the road we could well end up
with more kinds of channels, and at a certain point the cost of the
indirect function call becomes lower than the cost of jumping through
the enumeration. For that reason, we should define an abstraction.
Enter:
MultiplexerAbstractChannel
. This concrete type defines theinterface that
HTTP2StreamMultiplexer
will use to interact with streamchannels. As it's internal, the abstraction is totally porous: Swift is
capable of optimising it away to nothing. This type is explicitly a
reference type, maintaining the semantic expected by
HTTP2StreamMultiplexer
.This provides a nice encapsulation over our abstraction.
HTTP2StreamMultiplexer
never needs to know about how we abstract overmultiple channel types: the entire abstraction lives in a single, simple
file, and can easily be rewritten without difficulty.
Modifications:
MultiplexerAbstractChannel
and the interface expected byHTTP2StreamMultiplexer
.BaseChannel
enum
that will be used to dispatch tomultiple
Channel
types.HTTP2StreamMultiplexer
to use this new abstraction.Result:
A customisation point for new channels will exist.