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

Flume Overhaul #84

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Flume Overhaul #84

wants to merge 4 commits into from

Conversation

zesterer
Copy link
Owner

@zesterer zesterer commented Jul 7, 2021

This is a new from-scratch implementation of Flume with an async core.

A few things to note about the design:

  • The core is async-only and is no_std. I use my own pollster crate for sync support.
  • The design uses more channel flavour specialisations
  • Performance is generally comparable to or better than the master branch
  • The select API has been overhauled and is now easier to work into a select! macro
  • Most (or perhaps all?) of the test suite passes

Some unresolved issues:

  • Dropping a stream/future early may swallow items due to the way that items are eagerly allocated to receivers. This is quite a difficult to problem to solve (the master branch uses a lot of complicated hackery to ensure that the receiver is properly deregistered, but it's still not ideal: it's possible to hold on to a receiver for a long time without polling it, resulting in a 'dead' item that is in the queue, but cannot be received by anything else. The simplest fix would be to use a poll-based model rather than an intrusive wakeup model, but I've found this to perform considerably worse.
  • Documentation
  • Missing APIs (they're mostly there, but some things still need implementing)
  • Discussion about sufficiently separating everything to allow for possible future integration into std. This is more difficult than might be obvious. Crates like hashbrown can be depended upon by std because they are themselves no_std. However, a crate that requires OS sync primitives (like flume) cannot be depended upon. A possible solution is for std to depend upon the no_std core and then provide a minimal pollster-like async executor to evaluate futures.

@cloudhead
Copy link

Does this mean flume wouldn't work without an async runtime like tokio?

@Restioson
Copy link
Collaborator

It would still support sync through the use of pollster internally

@zesterer
Copy link
Owner Author

zesterer commented Aug 2, 2021

Yep. Flume's internal futures would be runtime-agnostic.

@vojtechkral
Copy link

vojtechkral commented Oct 18, 2021

Hi. Hows the rewrite going? I was considering to add task sync functions such as .close() (like async-channel has) and .closed() (like tokio mpsc has), but if things are in flux, it's probably not a good idea to do that now...

@zesterer
Copy link
Owner Author

zesterer commented Oct 18, 2021

Hi. Hows the rewrite going? I was considering to add task sync functions such as .close() (like async-channel has) and .closed() (like tokio mpsc has), but if things are in flux, it's probably not a good idea to do that now...

You're welcome to implement these and I'll merge/release them if you do (the latter is already implemented as .is_disconnected()). I've been trying to make time for this rewrite but it's looking like I might require another month to fulfill my existing commitments.

@vojtechkral
Copy link

@zesterer FWIW, closed() is a bit different, it's an async fn / future that actually waits for the closing event.
However I've given it more thought and probably I'll do the synchronization with a standalone structure rather than through those channels...

@zesterer zesterer force-pushed the master branch 3 times, most recently from ce28b9c to 24505d2 Compare March 10, 2022 20:50
@Restioson
Copy link
Collaborator

Restioson commented Aug 17, 2022

Sorry for the necro here, but I ended up doing something a bit similar to this in xtra in this pull request: Restioson/xtra#85. It has since evolved a lot with contributions from @thomaseizinger, but the core concepts remain the same, many of which were borrowed from flume and what I learned whilst working on it. It's a bit different as it has to support three kinds of channels in one, but overall the concept is similar to this. It lacks a blocking interface but this could use something lightweight like pollster or futures::executor::block_on.

One thing that we've looked at is replacing the bespoke waiting sender/receiver queues (VecDeque<(Waker, Booth<T>)> in this PR) with oneshot channels (VecDeque<OneshotTx<T>>). We currently use a catty oneshot to represent a waiting receiver (the choice of which oneshot is not super important, though maybe some logic could be encapsulated in a sufficiently flexible oneshot implementation). This does the job of holding the item and managing receiver waiting for the channel. Currently, we do use a bespoke type for a waiting sender, as there is no rendezvousing send in catty (I do want to implement this though).

I'm not sure if this is still planned at all but I would potentially be interested in looking into it or offering advice based on my experiences implementing xtra's channel, which is very similar to flume's in concept as mentioned previously.

@zesterer
Copy link
Owner Author

zesterer commented Aug 17, 2022

I'm not sure if this is still planned at all but I would potentially be interested in looking into it or offering advice based on my experiences implementing xtra's channel, which is very similar to flume's in concept as mentioned previously.

I would really love to get round to dedicating more time to work on this, but if I'm really honest I think I'm going to struggle given other obligations I have right now (both toward other projects, and toward things in the rest of my life).

If someone is interested in picking up the challenge of rewriting Flume with a simpler async core (something that I think would be very rewarding), I'd be very happy to have a discussion with them and pass on what knowledge I have / review changes though.

@Restioson
Copy link
Collaborator

I would really love to get round to dedicating more time to work on this, but if I'm really honest I think I'm going to struggle given other obligations I have right now (both toward other projects, and toward things in the rest of my life).

Of course, I completely understand!

If someone is interested in picking up the challenge of rewriting Flume with a simpler async core (something that I think would be very rewarding), I'd be very happy to have a discussion with them and pass on what knowledge I have / review changes though.

Awesome :) I might give it a shot if I find some time, and if so you can leave a review if you have time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants