-
Notifications
You must be signed in to change notification settings - Fork 36
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
inline poll_states
and remove Fuse
for vec::merge
#79
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I had a couple small suggestions for potential improvements, but I'll leave them up to you if you want to take them. Feel free to merge whenever!
rng: RandomGenerator, | ||
complete: usize, | ||
wakers: WakerList, | ||
state: PollStates, | ||
done: bool, | ||
len: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is the number of streams we're merging? I might suggest count
or num_streams
as alternate names, but there's plenty of precedent for using len
for this sort of thing, so I'm happy to leave it up to your judgement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'd suggest adding a len()
method that returns self.streams.len()
and that way we save 8 bytes off the size of the Merge
struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I wish we could set methods! - Because we're dealing with a pin projection it generates a struct for us containing the projection. Andy any self
methods would need to point to that.
Because we don't control the projection we can't define methods. And so we basically have to define everything in-line. Which is like, another reason why I'd reallllly like us to have safe pin projections, so we can split manually authored futures into separate internal methods etc. Not being able to do this is another reason authoring futures by hand is hard :/
Ref #22
Counterpart to #70. Removes the use of
Fuse
fromimpl Merge for Vec
, and also inlines thepoll_states
.Performance
Compared to main
Performance remains roughly comparable.
Compared to last release
The delta here remains mostly dominated by the move to "perfect" waker semantics: