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

Use a single Arc shared between Wakers in WakerArray and WakerVec #118

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wishawa
Copy link
Contributor

@wishawa wishawa commented Jan 9, 2023

This PR is dependent on #117

after = this PR
before = #117

group                after                                  before
-----                -----                                  ------
array::join 10       1.00      2.3±0.12µs        ? ?/sec    1.13      2.6±0.20µs        ? ?/sec
array::join 100      1.00     23.5±0.71µs        ? ?/sec    1.17     27.5±0.39µs        ? ?/sec
array::join 1000     1.10  1521.6±41.26µs        ? ?/sec    1.00  1377.7±21.95µs        ? ?/sec
array::merge 10      1.00      2.3±0.04µs        ? ?/sec    1.39      3.3±0.07µs        ? ?/sec
array::merge 100     1.00     52.5±6.18µs        ? ?/sec    1.09     57.5±2.20µs        ? ?/sec
array::merge 1000    1.00      2.5±0.06ms        ? ?/sec    1.04      2.6±0.07ms        ? ?/sec
array::race 10       1.00  1158.9±19.21ns        ? ?/sec    1.00  1159.8±25.91ns        ? ?/sec
array::race 100      1.00     12.2±0.33µs        ? ?/sec    1.02     12.5±0.48µs        ? ?/sec
array::race 1000     1.02    144.0±5.25µs        ? ?/sec    1.00    140.7±1.84µs        ? ?/sec
tuple::join 10       1.00      2.1±0.04µs        ? ?/sec    1.17      2.5±0.06µs        ? ?/sec
tuple::merge 10      1.00      2.8±0.06µs        ? ?/sec    1.10      3.1±0.07µs        ? ?/sec
tuple::race 10       1.05  1147.9±36.64ns        ? ?/sec    1.00  1089.3±19.36ns        ? ?/sec
vec::join 10         1.00      2.5±0.03µs        ? ?/sec    1.16      2.9±0.09µs        ? ?/sec
vec::join 100        1.00     34.5±1.06µs        ? ?/sec    1.15     39.6±0.80µs        ? ?/sec
vec::join 1000       1.07  1989.7±40.57µs        ? ?/sec    1.00  1863.9±26.66µs        ? ?/sec
vec::merge 10        1.00      2.9±0.13µs        ? ?/sec    1.25      3.6±0.06µs        ? ?/sec
vec::merge 100       1.00     57.3±1.04µs        ? ?/sec    1.28     73.3±1.38µs        ? ?/sec
vec::merge 1000      1.00      3.6±0.07ms        ? ?/sec    1.17      4.2±0.08ms        ? ?/sec
vec::race 10         1.01  1157.2±18.82ns        ? ?/sec    1.00  1147.6±14.78ns        ? ?/sec
vec::race 100        1.03     12.0±0.19µs        ? ?/sec    1.00     11.6±0.23µs        ? ?/sec
vec::race 1000       1.02    141.4±5.12µs        ? ?/sec    1.00    138.5±3.85µs        ? ?/sec

Also: made dummy waker easier to use and allocation-free.

Copy link
Owner

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Thanks so much for this PR! - I'm trying to better understand everything that's going on, so this first round of reviews is mostly focused on documentation and clarifying what's going on in the unsafe blocks. I've mentioned this in the review comments as well: but the intent of this crate is to upstream parts of it to the stdlib, which means we need to build it with that in mind. I hope that makes sense; thanks!

src/utils/wakers/shared_arc.rs Outdated Show resolved Hide resolved
src/utils/wakers/shared_arc.rs Outdated Show resolved Hide resolved
src/utils/wakers/shared_arc.rs Outdated Show resolved Hide resolved
src/utils/wakers/shared_arc.rs Outdated Show resolved Hide resolved
src/utils/wakers/vec/waker_vec.rs Outdated Show resolved Hide resolved
src/utils/wakers/shared_arc.rs Outdated Show resolved Hide resolved
src/utils/wakers/shared_arc.rs Show resolved Hide resolved
src/utils/wakers/shared_arc.rs Outdated Show resolved Hide resolved
src/utils/wakers/shared_arc.rs Outdated Show resolved Hide resolved
@wishawa
Copy link
Contributor Author

wishawa commented Feb 12, 2023

@yoshuawuyts thanks for the review! I added more documentation/comments, renamed things so that they (hopefully) make more sense, and simplified the code quite a bit[1]. Please take a look!

[1]
The simplification fixed the

if BY_REF {
            std::mem::forget(arc);
}

and

            wake::<A, false>,
            wake::<A, true>,

weirdness.

Previously I was using Arc::from_raw to get access to the Arc content, and dropping/forgetting the created Arc depending on whether the reference count should be decremented. In the updated code I dereference the raw pointer directly and, as you suggested, use Arc::decrement_strong_count properly.

@wishawa wishawa requested a review from yoshuawuyts February 12, 2023 07:37
@wishawa wishawa force-pushed the pr-2-shared-wakers-arc branch from 5099224 to 6466c29 Compare April 25, 2023 05:32
@wishawa
Copy link
Contributor Author

wishawa commented Apr 25, 2023

@yoshuawuyts @matheus-consoli is there still interest in this change? I think reducing the number of allocations from N to 1 is very beneficial.

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.

2 participants