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

Remove MaybeDone from tuple::join #103

Merged

Conversation

matheus-consoli
Copy link
Collaborator

@matheus-consoli matheus-consoli commented Nov 20, 2022

Ref #22

Remove the need for MaybeDone on Join for tuples.

This is a spiritual successor of #74, thank you @yoshuawuyts and @poliorcetics for your reviews and ideas!

The perf results are really nice now:

>> critcmp main maybegone -f tuple::join
group             main                                   maybegone
-----             ----                                   ---------
tuple::join 10    1.00    246.2±0.79ns        ? ?/sec    2.86    703.8±1.49ns        ? ?/sec

edit: removes reference to #21, as we're no longing implementing "perfect" waking

@matheus-consoli matheus-consoli force-pushed the join-tuple-remove-maybedone branch 2 times, most recently from 072998c to 1d6b20f Compare November 20, 2022 03:23
@matheus-consoli
Copy link
Collaborator Author

oh, yes, this also makes the implementation of join fair

@matheus-consoli matheus-consoli changed the title Remove MaybeDone from tuple::join Remove MaybeDone from tuple::join and make it fair Nov 20, 2022
@poliorcetics
Copy link
Contributor

I don't understand the perf result, isn't maybegone slower ?

@matheus-consoli
Copy link
Collaborator Author

ohnoooo, I'm stupid, I read the result backwards again
this sucks, sorry

@yoshuawuyts
Copy link
Owner

yoshuawuyts commented Nov 21, 2022

oh, yes, this also makes the implementation of join fair

Oof, yeah that's a pretty big slowdown. To be honest: the rng stuff probably shouldn't be included since all futures only poll once - and then have to wait for each other anyway. While this uses the rng things, I think it's already fair without it.

Given that #104 has shown meaningful speedups for non-allocating futures by removing the rng - perhaps things would be as fast as they are now if you just went back to a for 0..LEN {} loop?

@matheus-consoli
Copy link
Collaborator Author

matheus-consoli commented Nov 22, 2022

I finally got some positive results!

I implemented and tested many variations today, and some were 4.5x slower than main.

The biggest improvements came from stopping using WakerArray, and not using the Indexer type (introduced on #104)

Here're the results:

>> critcmp main maybegone-inlinetuple21 -f tuple::join
group             main                                   maybegone-inlinetuple21
-----             ----                                   -----------------------
tuple::join 10    1.06    246.7±0.58ns        ? ?/sec    1.00    233.0±0.66ns        ? ?/sec

edit:
For reference, those are the results using Indexer (src):

>> critcmp main maybegone-inlinetuple-indexer -f tuple::join
group             main                                   maybegone-inlinetuple-indexer
-----             ----                                   -----------------------------
tuple::join 10    1.00    246.7±0.58ns        ? ?/sec    3.94    971.5±8.50ns        ? ?/sec

@matheus-consoli matheus-consoli changed the title Remove MaybeDone from tuple::join and make it fair Remove MaybeDone from tuple::join Nov 22, 2022
@yoshuawuyts
Copy link
Owner

@matheus-consoli We probably should keep using the WakerArray, even if it costs us some extra performance on the current benchmarks. The reason for that is that our benchmarks only really measure futures with fast poll implementations.

At work we've seen cases where calls to poll ended up being a bottleneck, which isn't recommended but can absolutely happen if you don't pay super close attention. And in those cases having "perfect" waking will guard against the worst performance implications because it changes the waking algorithm from O(n^2) to O(n).

@matheus-consoli
Copy link
Collaborator Author

Updated to use WakerArray

>> critcmp main maybegone-inlinetuple-waking2 -f tuple::join
group             main                                   maybegone-inlinetuple-waking2
-----             ----                                   -----------------------------
tuple::join 10    1.00    246.7±0.58ns        ? ?/sec    2.12    524.0±1.11ns        ? ?/sec

@matheus-consoli matheus-consoli merged commit 9678a9e into yoshuawuyts:main Nov 23, 2022
@matheus-consoli matheus-consoli deleted the join-tuple-remove-maybedone branch November 23, 2022 00:55
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.

3 participants