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

replace rng with deterministic indexer #104

Merged
merged 2 commits into from
Nov 22, 2022
Merged

replace rng with deterministic indexer #104

merged 2 commits into from
Nov 22, 2022

Conversation

yoshuawuyts
Copy link
Owner

@yoshuawuyts yoshuawuyts commented Nov 21, 2022

Closes #85. Additionally it also significantly simplifies our iteration logic by baking it into the Indexer struct directly:

// before
let len = this.streams.len();
let r = this.rng.generate(len as u32) as usize;
for index in (0..len).map(|n| (r + n).wrapping_rem(len)) { ... }

// after
for index in this.indexer.iter() { ... }

Performance

The numbers overall seem pretty comparable, but they spiked a bit on non-allocating APIs which are otherwise fairly simple. impl Race for array showed one of the bigger changes:

❯ critcmp main patch
group             main                    patch
-----             ----                    -----
array::race 10    1.22   652.2±24.21ns    1.00   535.9±31.39ns
array::race 100   1.52      4.8±0.27µs    1.00      3.2±0.02µs
array::race 1000  1.07     28.4±0.56µs    1.00     26.6±0.58µs

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Nov 21, 2022
Copy link
Collaborator

@matheus-consoli matheus-consoli left a comment

Choose a reason for hiding this comment

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

I like how this simplifies the iteration, and the strategy seems to make sense to me!

Once we fix the offset increment, we should be good to go

src/utils/indexer.rs Outdated Show resolved Hide resolved
src/utils/indexer.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@matheus-consoli matheus-consoli left a comment

Choose a reason for hiding this comment

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

This looks great!

@yoshuawuyts yoshuawuyts merged commit a33e87e into main Nov 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the round-robin branch November 22, 2022 16:11
/// Generate a range between `0..max`, incrementing the starting point
/// for the next iteration.
pub(crate) fn iter(&mut self) -> IndexIter {
// Increment the starting point for next time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to try to save where we ended up last time and pick up from there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deterministically pick the next future to poll
3 participants