-
Notifications
You must be signed in to change notification settings - Fork 170
Add MultiProducerSingleConsumerChannel
#305
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
base: main
Are you sure you want to change the base?
Conversation
ddd7523
to
65e8957
Compare
@swift-ci please test |
4904039
to
662693e
Compare
AsyncBackpressuredStream
proposal and implementationMultiProducerSingleConsumerChannel
|
||
### Upstream producer termination | ||
|
||
The producer will get notified about termination through the `onTerminate` |
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.
nit: can you add calls to onTerminate
to the snippets below?
}) | ||
} | ||
} catch { | ||
// `send(contentsOf:)` throws if the asynchronous stream already terminated |
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.
Would this be clearer as another case on the sendResult?
/// - Parameters: | ||
/// - low: When the number of buffered elements drops below the low watermark, producers will be resumed. | ||
/// - high: When the number of buffered elements rises above the high watermark, producers will be suspended. | ||
/// - waterLevelForElement: A closure used to compute the contribution of each buffered element to the current water level. |
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.
Oh, neat. This works nicely for collections.
...orithms/MultiProducerSingleConsumerChannel/MultiProducerSingleConsumerChannel+Internal.swift
Outdated
Show resolved
Hide resolved
.../AsyncAlgorithms/MultiProducerSingleConsumerChannel/MultiProducerSingleConsumerChannel.swift
Outdated
Show resolved
Hide resolved
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#if compiler(>=6.0) |
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.
Please can we consider offering a pre-Swift 6 variant of this type. There are several places we'd like to adopt this that need to also support Swift 5.9+ for the foreseeable.
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.
Hm not sure yet. Especially since it is part of the public API in the Source
. Making the Source
copyable makes the type behave way differently.
.../AsyncAlgorithms/MultiProducerSingleConsumerChannel/MultiProducerSingleConsumerChannel.swift
Outdated
Show resolved
Hide resolved
Hi!
|
@ser-0xff Thanks for trying it out. We don't plan on support library evolution mode in any of our packages. What we normally advise is to not compile packages as frameworks but rather add this library as an |
Thank you for the reply... |
@ser-0xff I guess we need to wrap up the API surface we want to support then and just do internal import as suggested? But let's take that offline from this PR - thanks @FranzBusch for quick reply. |
Hi, @FranzBusch Here is a minimized test showing the usage pattern we want if you will find some time to look at. |
The source has a |
Missed |
HI, Franz! |
You are right. I haven't gotten around to implement the correct handling of multiple sources yet. |
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.
I think with some minor edits the pitch is good to go; id like to at the bare minimum see some of that testing reenabled to ensure it isn't leaking.
/// It supports arbitrary many elements but if only up to one ``Element`` is stored it does **not** allocate separate storage on the heap | ||
/// and instead stores the ``Element`` inline. | ||
@usableFromInline | ||
struct _TinyArray<Element> { |
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.
I wonder if this could perhaps be subsumed by InlineArray
instead since the original intent was to have a faster storage (this is not a blocking concept)
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.
InlineArray
isn't capable of fully replacing the need here. The idea behind TinyArray
is to have a fast path for a single element that doesn't allocate and one that allocates if there are more. Since we can't tell at compile time how many producers we have we need this runtime dynamism.
@@ -17,24 +17,35 @@ import Glibc | |||
import WinSDK | |||
#endif | |||
|
|||
internal struct Lock { | |||
@usableFromInline | |||
internal class Lock { |
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.
umm why is this a non-final class? If we are moving to a class then we should just use Mutex
from Synchronization
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.
I removed all of the changes here since the latest code now uses Mutex
.
.../AsyncAlgorithms/MultiProducerSingleConsumerChannel/MultiProducerSingleConsumerChannel.swift
Outdated
Show resolved
Hide resolved
final class MultiProducerSingleConsumerChannelTests: XCTestCase { | ||
// MARK: - sequenceDeinitialized | ||
|
||
// Following tests are disabled since the channel is not getting deinited due to a known bug |
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.
This might be a requirement to landing; id rather not introduce a type with a known leak
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.
I added significantly more tests now and also re-enabled those.
|
||
```swift | ||
// Writing new values and providing a callback when to produce more | ||
try source.send(contentsOf: sequence, onProduceMore: { result in |
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.
can this be written without ambiguity as a trailing closure?
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.
Yes in a synchronous context both send
methods can be called without ambiguity, e.g. this compiles:
try source.send(1)
source.send(2) { result in
print(result)
}
The only thing that needs some special handling is if you want to call a sync send from an async context since the async methods are preferred; however, users can workaround this by creating an inline closure, e.g.:
try { try source.send(1) }()
662693e
to
944b727
Compare
# Motivation The pitch to add external backpressure support to the standard libraries `AsyncStream` got returned for revision since there are larger open questions around `AsyncSequence`. However, having external backpressure in a source asynchronous sequence is becoming more and more important. # Modification This PR adds a modified proposal and implementation that brings the Swift Evolution proposal over to Swift Async Algorithms.
f086eda
to
216e38f
Compare
216e38f
to
ae3f677
Compare
ae3f677
to
b30ec17
Compare
6b24e93
to
8d2f4d6
Compare
/// The channel will only automatically be finished if all existing sources have been deinited. | ||
/// | ||
/// - Returns: A new source for sending elements to the channel. | ||
public mutating func copy() -> sending Self { |
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.
here (and a in a few other places) we have sending Self
where Self
conforms to Sendable
. in such cases is the sending
doing anything meaningful, since the type can already be freely passed across isolations? i think adding a diagnostic to flag such cases as redundant has also been proposed/drafted. i suppose if the Sendable
conformance were to be dropped in the future, maybe such a change would be 'less breaking' if things are spelled this way?
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.
Thanks for calling this out. I think the right thing is to actually remove the Sendable
conformance on Source
since it is in fact not Sendable
.
/// - Parameters: | ||
/// - low: When the number of buffered elements drops below the low watermark, producers will be resumed. | ||
/// - high: When the number of buffered elements rises above the high watermark, producers will be suspended. | ||
public static func watermark(low: Int, high: Int) -> BackpressureStrategy { |
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.
apologies if i missed this having been discussed somewhere in the prior history of this work, but did you consider using a ClosedRange
of an appropriate unsigned integer type to model the watermark state? naively it seems that might obviate some of the invariant checks.
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.
I think it's interesting but I personally prefer having to Int
's here since the watermark strategy is not really about the range but a low and high limit where the production is started and stopped respectively.
mutating func didSend(elements: Deque<Element>.SubSequence) -> Bool { | ||
if let waterLevelForElement = self._waterLevelForElement { | ||
for element in elements { | ||
self._currentWatermark += waterLevelForElement(element) |
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.
it looked to me like this logic runs at various points while the state machine lock is held – any concerns about invoking client code while holding a lock?
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.
I updated the public docs to call this out. I think the reality is that we need to call this code while being under a lock since the state management otherwise would become almost impossible with potentially any code interleaving.
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.
Over all I think this is a lot of complicated machinery, it is definitely filling a gap of use; but we need to be careful about how widely this is expected to be used; the long name right now is definitely keeping that progressive disclosure correct, but I think we can soften some of the requirements a bit and find a better name; right now it is pretty wordy.
All that being said; id say that since the need (and conjunction with other efforts makes it reasonable) then it is worth running this to get some additional feedback by focusing the review on how we can make some of the uses a bit more straightforward and suggestions on what the appropriate name is.
/// | ||
/// This error is thrown when the channel is already finished when | ||
/// trying to send new elements to the source. | ||
public struct MultiProducerSingleConsumerAsyncChannelAlreadyFinishedError: Error { } |
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.
This seems like it should be a nested type; it is a keyboard full of naming here...
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.
I agree it is a long but we can't nest it due to the MPSCCAsyncChannel
being generic.
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.
I guess folks aren't going to be typing this type out much eh, so perhaps not a huge deal; do we really even need this type to be public?
system and compares them to the behaviors of `Async[Throwing]Stream` and | ||
`Async[Throwing]Channel`. | ||
|
||
This section proposes a new type called `MultiProducerSingleConsumerAsyncChannel` |
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.
Id really love to come up with a smaller name. We have been using water metaphors, so perhaps something in that family of names? From my understanding of the pitch it is like you have many small tributaries contributing to a larger stream of values. Id like to come up with some alternatives than this.
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.
What if we call it MPSCAsyncChannel
. The German in me does like long explicit wordings but I'm happy to go with abbreviations here. The other name I was thinking about was MultiProducerAsyncChannel
or ExternalBackpressuredAsyncChannel
.
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.
MultiProducerAsyncChannel
sounds good to me!
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.
4 words is better than 6 I guess...
/// | ||
/// - Important: This token must only be passed once to ``MultiProducerSingleConsumerAsyncChannel/Source/enqueueCallback(callbackToken:onProduceMore:)`` | ||
/// and ``MultiProducerSingleConsumerAsyncChannel/Source/cancelCallback(callbackToken:)``. | ||
public struct CallbackToken: Sendable, Hashable { } |
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.
Is it possible that these tokens are somehow user provided?
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.
If you mean constructed by the user then no. There is no public init
for those.
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.
no, I was suggesting that the user provides a type to represent a token and this could be token that the framework provides. It would mean that things would need to be flipped around to be generic around that.
There are some APIs that have tokens associated with callbacks. Thinking particularly the CF APIs or the KVO APIs
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.
I see what you mean I would like to avoid that though since we are using those tokens to understand if there is a potential API mis-use. We wouldn't be able to do this if the user were to create those tokens.
/// A callback to invoke when the channel finished. | ||
/// | ||
/// This is called after the last element has been consumed by the channel. | ||
public func setOnTerminationCallback(_ callback: @escaping @Sendable () -> Void) |
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.
it might be useful to have a parameter in the closure to tell why the termination happened, either finished, failed, or cancelled.
Motivation
The pitch to add external backpressure support to the standard libraries
AsyncStream
got returned for revision since there are larger open questions aroundAsyncSequence
. However, having external backpressure in a source asynchronous sequence is becoming more and more important.Modification
This PR adds a modified proposal and implementation that brings the Swift Evolution proposal over to Swift Async Algorithms.