feat: Rework the internal structure of the event-listener crate #51

notgull · 2023-03-16T21:15:42Z

Alright, here we go again.

This PR once again reworks the internal structure of event-listener. An intrusive linked list is the fastest way to handle an event queue, but isn't available in libstd. Therefore, this PR reworks the internals such that the implementation of event handling used depends on whether or not the libstd feature is enabled:

If std is enabled, then it uses the intrusive linked list.
If std isn't enabled, then it falls back to the current master strategy of using a linked list arena falling back to an atomic queue.

I'm pretty happy with this iteration at this stage. Draft because I still need to refine the tests.

benches/bench.rs

fogti · 2023-03-29T22:14:27Z

ok, so some changes do affect lifetimes and sprinkle as_mut() all over the place, can we split them out as much as possible (perhaps even do them without changing any functionaliy, e.g. via PhantomData for lifetimes, so that the noise is reduced)? Reviewing this in the current state is really hard, as it is not clear what is moved and how.

fogti · 2023-03-29T22:17:42Z

src/std.rs

@@ -188,7 +188,7 @@ impl Inner {
        match next {
            None => self.tail = prev,
            Some(n) => unsafe {
-                n.as_ref().next.set(prev);
+                n.as_ref().prev.set(prev);


what happens here?

There was a bug in there that was causing the MIRI issues. It should be fixed now.

fogti · 2023-03-29T22:22:00Z

The commit fmt should be squashed with the commit which introduced the formatting divergence (that is, modified that code segment previously).
The commit Wait tests are hanging on MIRI, not sure why which seems to affect the first commit should also be squashed into it.
The second commit Build ndf into C/I appears ok, but the commit message should be refined to explain it better.
The first commit Incomplete should then be split into several, optimally independent commits which should compile on their own and demonstrate what code is moved. This branch should not be squashed upon merging, as the structure and hopefully step-wise natrue of the resulting commits is by then probably necessary to understand what is going on.
The last commit might be further refined, but also appears currently ok

This move abstracts them to be a sub-module of the inner list.

Ideally, we should have as few dependencies as possible. This change inlines most of the logic from `slab` that we used into the `ListenerSlab` struct.

Everything from that crate can either be inlined or ignored. I also fix a bug with the list implementation as well.

- Renames "list.rs" and the "list" folder to "no_std" - Uses `no_std` as the `sys` module in preparation for the `std` module - Inlines `sync.rs` into `lib.rs`

First part of EventListener reworks. The idea here is that, in the future, we can add an "EventListenerRef" structure that just takes an "&'a Inner" rather than an "Arc<Inner>". This way, you don't need to do an atomic clone for listening.

This optimization is inspired by what futures_lite does for the block_on function. This should prevent users from dealing with the overhead of creating a parker and unparker every time wait() is called.

The EventListener for the upcoming libstd-based implementation needs to be pinned, so this commit sets up the infrastructure for the pinned EventListener. This is a breaking change.

This implementation is around 20% faster than the no_std one, according to benchmarks. Therefore, we should use it where we can.

- Adds the unit test set to the no_std implementation - Fixes doc tests in event-listener-strategy

notgull · 2023-03-31T21:11:35Z

I've split the Incomplete commit into about 15 sub-commits, so this should be easier to review incrementally now.

src/lib.rs

src/list.rs

src/std.rs

fogti · 2023-04-02T17:12:33Z

thanks, looks overall LGTM.

notgull force-pushed the notgull/rework-int-list branch from b5d631a to b0ffcac Compare March 16, 2023 22:49

notgull marked this pull request as ready for review March 17, 2023 19:02

notgull requested a review from fogti March 29, 2023 16:21

fogti reviewed Mar 29, 2023

View reviewed changes

benches/bench.rs Outdated Show resolved Hide resolved

fogti reviewed Mar 29, 2023

View reviewed changes

notgull added 18 commits March 30, 2023 09:14

Move inner data to lib.rs

9e32177

Move list guard/inner list to list.rs

0ddf3d6

Move listener data into list.rs

be047ad

Trim unnecessary abstractions

1a4df18

Move the lock method to list.rs and rename it to try_lock

2d171ae

Move node.rs and queue.rs to be submodules of list.rs

393566a

This move abstracts them to be a sub-module of the inner list.

Remove the slab dependency

1188962

Ideally, we should have as few dependencies as possible. This change inlines most of the logic from `slab` that we used into the `ListenerSlab` struct.

Remove the crossbeam-utils dependency

5c83b86

Everything from that crate can either be inlined or ignored. I also fix a bug with the list implementation as well.

High-level module reorganization

0c76968

- Renames "list.rs" and the "list" folder to "no_std" - Uses `no_std` as the `sys` module in preparation for the `std` module - Inlines `sync.rs` into `lib.rs`

Replace Listener with Option<sys::Listener>

09ded13

Use a cached Parker/Unparker pair when possible

c659cf8

This optimization is inspired by what futures_lite does for the block_on function. This should prevent users from dealing with the overhead of creating a parker and unparker every time wait() is called.

EventListener is now used through pinning

996ee4d

The EventListener for the upcoming libstd-based implementation needs to be pinned, so this commit sets up the infrastructure for the pinned EventListener. This is a breaking change.

Add a libstd implementation of the inner list

20c69a1

This implementation is around 20% faster than the no_std one, according to benchmarks. Therefore, we should use it where we can.

Fix tests

3fefa6c

- Adds the unit test set to the no_std implementation - Fixes doc tests in event-listener-strategy

Unsplit the 'Incomplete' commit

1ec136a

Build tests for no-default-features into C/I

2ac3be0

Avoid allocation in the benchmarks

bc07d43

notgull force-pushed the notgull/rework-int-list branch from 4150045 to bc07d43 Compare March 31, 2023 20:45

notgull added 2 commits March 31, 2023 13:58

Fix code review/CI issues

17952d6

Forgot to adjust length in remove()

8208309