async-oneshot icon indicating copy to clipboard operation
async-oneshot copied to clipboard

[WIP] Rewrite and rename to 'async-hatch'

Open jjl opened this issue 3 years ago • 17 comments

The library has undergone significant improvements as part of another rewrite. The API has changed slightly to accommodate the improvements. Most importantly, you need to sender.send(42).now() instead of sender.send(42) (though your code should start throwing warnings if you don't).

Notable changes:

  • Send more than one message (i.e. stop being one shot, hence name change)
  • Recover - recreate the other half when it has closed.
  • Support use without an allocator.
  • Impressive performance gains.
  • Better and simpler use of unsafe.
  • LOTS more I can't be bothered to write about.

There's a little left to do before release:

  • Tests for recovery and recycling
  • Github actions setup

I also only have benchmarks for boxed hatches because criterion makes writing the others quite difficult.

Feedback very wanted. I haven't had much so far from people who actually use it. Wouldn't complain if someone wrote those tests either ;)

jjl avatar Jul 27 '21 05:07 jjl

* I think we should wrap the `Flags` in an additional newtype struct, so that we can easily mark `flags` (the struct field) on `Sender` and `Receiver` as `pub`, and put the get/set methods on them. I also think that the set-methods should have get a `&mut self` instead of `self` and then return `&mut Self`. This would be more ergonomical, and easier to compose.

For a few months of experimentation, I actually had them as a repr(u8) enum. I just ultimately got fed up of it being a lot more code than was necessary. It was pretty nice though.

* I'm not sure if I correctly understand these multiple layers of `Option` wrapping, and why they are done... It might be a good idea to invest some time in trying to wrap this into a move-semantic workflow/type-state pattern instead ([see also](http://cliffle.com/blog/rust-typestate/)), but that could also be done after the merge (although it would be a good idea to get, if we would do that, ready before a release, because otherwise it would be annoying to have such a rapidly changing API)

The options are really just so we can disable the destructors when we know there is no further cleanup to do.

I love typestate style APIs, but they make life difficult for storing them inside an object (such as a future...)

jjl avatar Jul 28 '21 04:07 jjl

Correct me if I'm wrong, but I think broadly, you don't dislike the public api.

The internal API is incredibly up for grabs. Here are a few things we could do:

  • Bring back a type for the flags. It was nice, it's just by the time you've implemented all the bitop traits etc., it's quite a lot of code.
  • Replace those options with a flag. It means less checking. Unlikely to make it significantly faster tho.
  • Bring back a flag for 'has value'. Possibly lock it separately. This could possibly enable wait-free sending again, but maybe it has impacts on other operations.
  • Bring back individual locks for the wakers. This one is probably a deoptimisation. Anything touching the value will have to touch the wakers.
  • Be less optimistic (e.g. do a read rather than assuming it will be in the right state). Could go either way depending on your workload I think.

The more flags we use, the harder it gets to keep the locking logic simple and readable. And could also impact performance negatively.

In terms of performance, while we are very fast, we're probably not optimal. I think with rather more work than seems strictly necessary, we could probably shave some further time off, though as the times aren't very long already (except destruction), it seems like something that we might do gradually.

One thing I think is essential is to keep the Sender and Receiver methods requiring only &mut self. This allows us to use iter_batched_ref in the benchmarks and not pay the cost of the destructor (which dwarves the time required for all the other operations). I've got a private benchmarks repo where I compare against other things and while tokio by not doing this makes it very easy for us to look good by comparison, i'd prefer we could honestly compare them so i can get a feel for what costs what.

jjl avatar Jul 28 '21 04:07 jjl

I have mostly been working on the (forthcoming shortly) async-spsc this week, which offers a bounded alternative.

I am wondering whether I should just put them both into async-spsc to deal with the naming issue. Or at least to cause a different naming issue :roll_eyes:

They are both SPSC channels differing primarily in buffer capacity, although since my rewrite, this library is becoming a slightly more versatile synchronisation primitive than most channels.

Does anyone have any strong feelings about this?

jjl avatar Aug 05 '21 12:08 jjl

I'm thinking no at the current time.

jjl avatar Aug 09 '21 08:08 jjl

ok, the options are gone and so are a bunch of potential branches thanks to some neat bit fiddling. the code is also somewhat shorter and easier to read.

jjl avatar Aug 23 '21 16:08 jjl

This might be interesting/contributing to the Send / Sync discussion: https://github.com/sslab-gatech/Rudra#send-sync-variance-unrestricted-send-or-sync-on-generic-types https://github.com/rust-lang/futures-rs/issues/2239

fogti avatar Aug 23 '21 19:08 fogti

That is interesting, yes.

I thought a lot about the Send/Sync bounds the other week and i'm happy that if T: Send, we can be Send + Sync, at least in the current state where we require &mut self to do anything useful. I will revisit it at some point to consider what things we might be able to do with &self only.

My main observation is that while it's more convenient to go from &mut self to &self, the gain isn't nearly as much as going from self to &mut self. All the other oneshot channels take self afair. Obviously as a reusable channel, we couldn't even take self anymore... However, non-oneshot channels tend to only require &self.

Because logically there are only two parties, the entire library was based around this assumption, which Sync + not requiring &mut self would potentially break as a combination. I need to reason about this somewhat more closely.

jjl avatar Aug 24 '21 06:08 jjl

I decided the SendError splitting was worthwhile and went with it. On the other hand it broke all the tests and I can't be fucked to fix them today.

Also: fine-grained the async/sync send operation

jjl avatar Aug 24 '21 08:08 jjl

The benchmarks are working again and they've been extended somewhat. I figured out how to do the remaining difficult benchmarks (at least with boxes).

Some of our benchmark times are slightly worse than they were before because of a change to benchmarking methodology: not unwrapping options. I suspect that llvm was being devious and that this should better reflect real-world performance.

I've also done a little comparative testing with tokio's oneshot and we're a few percent faster for oneshot send+receive (sync and async) on my dev machine.

jjl avatar Sep 19 '21 17:09 jjl

I still think that mark_on_drop and close_on_* methods should take &mut self instead of self`. (especially now because the interface moves away from being designed kinda builder-stylish)

fogti avatar Sep 20 '21 09:09 fogti

I still think that mark_on_drop and close_on_* methods should take &mut self instead of self`. (especially now because the interface moves away from being designed kinda builder-stylish)

hrm, the reason for that was because i anticipated you would want to immediately configure them and it turns it into a one liner. I suppose we could return &mut Self so you could still chain them?

jjl avatar Sep 20 '21 09:09 jjl

Ah hrm, so that allows you to chain subsequent modifications, but you still need to have the original data stored, so it means you lose the ability to do a one-liner.

I tried it just now and got this error:

error[E0308]: mismatched types
  --> src/lib.rs:42:6
   |
42 |     (s.close_on_send(true), r.close_on_receive(true))
   |      ^^^^^^^^^^^^^^^^^^^^^ expected struct `sender::Sender`, found mutable reference
   |
   = note:         expected struct `sender::Sender<'static, T>`
           found mutable reference `&mut sender::Sender<'_, _>`

jjl avatar Sep 20 '21 09:09 jjl

Maybe we should compromise and have both :eyes:

jjl avatar Sep 20 '21 09:09 jjl

I would be fine with that.

fogti avatar Sep 20 '21 09:09 fogti

I've just finished some improvements to the Sender. Mostly in the way of documentation and comments, but it includes these.

jjl avatar Sep 20 '21 09:09 jjl

I've ripped out the borrowed pointer stuff. i also can't think of a use case that is still not served by other means.

jjl avatar Sep 20 '21 10:09 jjl

thanks

fogti avatar Sep 20 '21 15:09 fogti