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

jjl · 2021-07-27T05:20:02Z

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 ;)

…ments that turned up in the process.

…_ptr_hatch`, update docs

fogti

good. I appreciate your efforts.

I have two additional notes to the stuff I annotated:

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.
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), 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)

src/lib.rs

src/receiver.rs

src/sender.rs

fogti · 2021-07-27T21:41:13Z

src/sender.rs

-            } else if state.recv() {
-                Poll::Ready(Ok(that))
+    #[cfg(feature="async")]
+    #[must_use = "Wait does nothing unless you oll it as a Future."]


I don't think this must_use is required, because if Wait is a Future, it would issue a warning anyways.
And the comment has a typo: oll.

I don't think they're automatic. I guess I'll have a play and figure out. And fix the typo.

src/receiver.rs

jjl · 2021-07-28T04:07:55Z

* 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 · 2021-07-28T04:59:16Z

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 · 2021-08-05T12:39:43Z

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 🙄

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 · 2021-08-09T08:33:09Z

I'm thinking no at the current time.

…ag mangling

jjl · 2021-08-23T16:31:02Z

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.

fogti · 2021-08-23T19:23:13Z

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
rust-lang/futures-rs#2239

jjl · 2021-08-24T06:26:07Z

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.

… the tests temporarily

jjl · 2021-08-24T08:26:06Z

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

…and run with async disabled and miri run quicker. Benchmarks still broken.

fogti · 2021-09-19T16:25:45Z

src/sender.rs

        let flags = self.sender.lock();
-        // Our cleanup is to remove the waker. Also release the lock.
        let shared = unsafe { &mut *self.sender.hatch.inner.get() };


Safety remarks?

It seemed a little excessive given the lock is taken in the previous line.

ah ok, so they are directly related, e.g. sender.lock locks the hatch?

yes. the spinlock (taken via sender.lock / receiver.lock) provides mutual exclusion for the value and wakers (in the async case).

we assume that the other side has always made a change (acquire) and we release our changes when we unlock.

I know we used to have individual locks, but it didn't seem to be winning us anything while it was increasing complexity. now we just have the one lock for all the mutable hatch data.

the sender.lock / receiver.lock return either when they have the lock or they detect the other side has closed (by reading the returned flags). in the latter case, they will update the sender or receiver's local copy of the flags, allowing us to elide further atomics.

a mutex is the wrong primitive, it implies that on failure (usually after a few spins) it will use a CondVar to get the OS to sleep the current thread. That is the absolute last thing we want in an async environment because it is just asking for the executor to be held up.

so we do the next best thing, a spinlock.

actually, you accidentally stumbled upon something quite interesting about the current design of async rust - there is no better way to handle this than a spin loop.

imagine the sender and receiver are on different threads. the sender takes the lock and then the OS deschedules it, then the receiver tries to take the lock. that spinlock is not going to succeed until the sender thread gets rescheduled.

in a non-async environment, we would actually want the OS to be involved, but in an async environment, that is not acceptable.

i have mooted previously that it would be nice if you could ask to reschedule the current async task. There is a hack for doing that (wake yourself, return pending), but it only does a reasonable job if you have fair scheduling (e.g. tokio's scheduler uses a 'next' slot which would just cause the same future to be executed again in all likelihood. it's a common optimisation and the workaround (also scheduling a dummy) would penalise users of executors that do provide fair queuing if we didn't lock it behind (yet another) feature flag. also it's just horrible and i'm hopeful that eventually we can get better support from the rust stdlib.

It is definitively possible to implement a mutex that is compatible with async, although it definitively takes a bit more work... Have you already looked at async-lock?

no, it's not.

because ultimately, you have to do whatever it is you're doing or set a waker to be woken up again. but the lock construct sets a waker! under the hood, even atomic-waker is a spinlock.

Additionally, we'd no longer be able to squeeze all our flags down to a single atomic if we used an existing implementation

… performance blooper where the fast path was never taken

jjl · 2021-09-19T17:01:52Z

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.

src/lib.rs

fogti · 2021-09-20T09:25:20Z

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)

jjl · 2021-09-20T09:30:56Z

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 · 2021-09-20T09:35:41Z

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 · 2021-09-20T09:36:20Z

Maybe we should compromise and have both 👀

fogti · 2021-09-20T09:48:59Z

I would be fine with that.

… and the addition of set_* methods for the Sender flags

jjl · 2021-09-20T09:52:38Z

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

jjl · 2021-09-20T10:09:44Z

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.

fogti · 2021-09-20T15:20:00Z

thanks

noib3 · 2025-01-05T10:36:34Z

Sorry to necrobump, but has this been abandoned?

jjl · 2025-01-05T17:02:28Z

Sorry to necrobump, but has this been abandoned?

yes. the new maintainer of this crate didn't want it and i don't want to maintain it. feel free to do something with it.

jjl added 9 commits March 17, 2021 17:17

attempt proofs for unsafety

9ed9ef6

enough to show off_

10d04cb

first cut of new async-hatch, mostly implemented with nice benchmarks

7fbc786

Documentation and commenting improvements, minor bugfixes and improve…

b42ed6b

…ments that turned up in the process.

Sender.wait, tests, benchmarks, docs etc.

9c3b4c0

Tests for refs and borrowed pointers, rename ptr_hatch to `borrowed…

2ec963e

…_ptr_hatch`, update docs

synchronous tests

22502a7

warning fixes

2319437

Make recovery fall back to checking the atomic

2357043

This was referenced Jul 27, 2021

Preview/Feedback #17

Closed

How about Inner being Sync even when T isn't? #16

Open

Reuse inner if possible? #15

Open

Relax atomic memory ordering constraints faern/oneshot#10

Closed

fogti reviewed Jul 27, 2021

View reviewed changes

Eliding the default lifetime is apparently now deprecated

157a7cf

tweak Send+Sync bounds

925970b

Remove option wrapping and eliminate some branches with branchless fl…

c0546c0

…ag mangling

split SendError and combine the async/sync send operations. break all…

114fb4c

… the tests temporarily

jjl added 2 commits August 24, 2021 13:36

I condensed the tests into one file, fixed them and made them better …

4b68d0c

…and run with async disabled and miri run quicker. Benchmarks still broken.

Tidy the code up a bit

e96e941

fogti reviewed Sep 19, 2021

View reviewed changes

Got the benchmarks working again and extended them massibely. Fixed a…

8cee8d8

… performance blooper where the fast path was never taken

fogti reviewed Sep 20, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

fogti reviewed Sep 20, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Mostly documentation and comments. A minor optimisation in Wait::drop…

d687a79

… and the addition of set_* methods for the Sender flags

Remove the borrowed pointer support. It seems redundant

98a268d

Add examples to documentation

ef3b2b6

jjl added 8 commits September 20, 2021 17:27

documentation improvements

ab2a107

stop using const for orderings

f1a7657

fix sync tests

590c8af

change from usize to u8 for flags

585cc27

remove borrowed pointer tests

77d36c1

benchmark

d40b05a

Make everything implement RefUnwindSafe and UnwindSafe

7dcd16d

refactor flags into an opaque struct

32c2270

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

jjl commented Jul 27, 2021

Uh oh!

fogti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jjl commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjl commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjl commented Aug 5, 2021

Uh oh!

jjl commented Aug 9, 2021

Uh oh!

jjl commented Aug 23, 2021

Uh oh!

fogti commented Aug 23, 2021

Uh oh!

jjl commented Aug 24, 2021

Uh oh!

jjl commented Aug 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjl Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjl commented Sep 19, 2021

Uh oh!

Uh oh!

Uh oh!

fogti commented Sep 20, 2021

Uh oh!

jjl commented Sep 20, 2021

Uh oh!

jjl commented Sep 20, 2021

Uh oh!

jjl commented Sep 20, 2021

Uh oh!

fogti commented Sep 20, 2021

Uh oh!

jjl commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjl commented Sep 20, 2021

Uh oh!

fogti commented Sep 20, 2021

Uh oh!

jjl commented Jul 28, 2021 •

edited

Loading

jjl commented Jul 28, 2021 •

edited

Loading

jjl Sep 20, 2021 •

edited

Loading

jjl commented Sep 20, 2021 •

edited

Loading