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

Replace custom Deque implementation with swift-collection Deque #67277

Merged
merged 1 commit into from
Jul 23, 2023

Conversation

FranzBusch
Copy link
Member

Motivation

For AsyncStream we created a custom internal Deque implementation to use for the buffering. This implementation was relatively bare-bones compared to the one in swift-collections. Furthermore, it lacked some methods that we need to implement the new AsyncStream APIs that support producer backpressure.

Modification

This PR copies over the Deque implementation of swift-collections and makes it internal and non-inlinable.

Result

We now have a fully functional Deque.

@FranzBusch FranzBusch requested review from ktoso and kavon as code owners July 13, 2023 12:30
@FranzBusch
Copy link
Member Author

@swift-ci please test

@FranzBusch FranzBusch requested a review from lorentey July 13, 2023 12:30
@MaxDesiatov MaxDesiatov added the Concurrencу Area → standard library: The `Concurrency` module under the standard library umbrella label Jul 13, 2023
@FranzBusch
Copy link
Member Author

@swift-ci please smoke test

@ktoso
Copy link
Contributor

ktoso commented Jul 18, 2023

LGTM about getting the swift-collection's deque.

This keeps it internal so that's fine, but maybe prepare a pitch as well to make it part of stdlib? It is used all over the place being copied around 🤔 WDYT @lorentey ?

If we intend to keep this copy around like this though, would it be good to automate copying the files a bit so it is easier to keep the two impls in sync? Or working towards removing the one in swift-collections in favor or a public pitched one from the stdlib.

@FranzBusch
Copy link
Member Author

Yeah it would be great if we could get Deque officially moved into the stdlib. It is the one type that I consistently reach for in every AsyncSequence implementation.

@ktoso
Copy link
Contributor

ktoso commented Jul 19, 2023

Up to @lorentey to give the thumbs up or down here perhaps.
But yeah this type would definitely be useful in concurrency library even for our internals

@lorentey
Copy link
Member

lorentey commented Jul 19, 2023

Yes, this is clearly an interim solution. Manually vendoring these implementations into every single ABI stable OS component is not going to be a good approach in the long term.

The big blocker for adding Deque as a public stdlib type is that there is a really good chance that Swift's incoming ownership features will render the implementation obsolete in a few months (years?) -- so we'd probably regret if we baked its current layout into the ABI right now.

Until we're ready to do that though, this is the best we can do! 👍 Mechanically copying a known-good implementation is better than having multiple parallel reimplementations of the same thing.

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

I recommend adding prominent comments at the top of source files pointing to their upstream location, to try prevent things from diverging too much from upstream.

In case the _Concurrency module ends up needing to make substantial changes/fixes to this code, those ought to be propagated back into the swift-collections package.

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed thoughts @lorentey! Looks good then.

Maybe a README or something in the Deque directory pointing at https://github.com/apple/swift-collections/tree/main/Sources/DequeModule could do it as well?

@FranzBusch
Copy link
Member Author

@swift-ci please smoke test

@FranzBusch
Copy link
Member Author

@swift-ci please test

@FranzBusch
Copy link
Member Author

@swift-ci please smoke test

@FranzBusch
Copy link
Member Author

@swift-ci please test

@FranzBusch
Copy link
Member Author

@swift-ci please test macos

@FranzBusch
Copy link
Member Author

@swift-ci please smoke test

@FranzBusch
Copy link
Member Author

@swift-ci please test

# Motivation
For AsyncStream we created a custom internal Deque implementation to use for the buffering. This implementation was relatively bare-bones compared to the one in swift-collections. Furthermore, it lacked some methods that we need to implement the new `AsyncStream` APIs that support producer backpressure.

# Modification
This PR copies over the Deque implementation of swift-collections and makes it internal and non-inlinable.

# Result
We now have a fully functional Deque.
@FranzBusch
Copy link
Member Author

@swift-ci please smoke test

@FranzBusch
Copy link
Member Author

@swift-ci please test macos

@FranzBusch FranzBusch merged commit b4ee68b into swiftlang:main Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concurrencу Area → standard library: The `Concurrency` module under the standard library umbrella
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants