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

fix accuracy of IteratorRandom::choose() #1059

Merged
merged 3 commits into from
Oct 17, 2020

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Oct 11, 2020

Fix accuracy bug in #1058. This is a value-stability-breaking change.

IteratorRandom::choose also guarantees O(1) performance for slices, so
there is no need to recommend an alternative
@dhardy
Copy link
Member

dhardy commented Oct 12, 2020

Could you add before/after benchmarks please?

@kazcw kazcw force-pushed the iteratorrandom-choose branch from 6d5041d to 7d8af10 Compare October 13, 2020 02:39
@kazcw
Copy link
Contributor Author

kazcw commented Oct 13, 2020

Ran benches.

The commit removing the special case for upper == Some(lower) caused a substantial performance regression in seq_iter_choose_from_1000. Upon some investigation, it's because when compiling <iter::SliceMut as IteratorRandom>::choose, the optimizer knows that upper == Some(lower) after inlining SliceMut::size_hint, and separating that case enables the optimizer to in effect specialize the function for an ExactSizeIterator by removing the whole loop. Neat! I'll add a comment in the source about why that's there.

After backing that commit out, the two benchmarks affected by these changes are seq_iter_unhinted_choose_from_1000 and seq_iter_window_hinted_choose_from_1000.

master:
test seq_iter_unhinted_choose_from_1000 ... bench: 17,443 ns/iter (+/- 312)
test seq_iter_window_hinted_choose_from_1000 ... bench: 3,330 ns/iter (+/- 77)

this branch:
test seq_iter_unhinted_choose_from_1000 ... bench: 13,387 ns/iter (+/- 162)
test seq_iter_window_hinted_choose_from_1000 ... bench: 3,091 ns/iter (+/- 105)

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Thanks kazcw. Can you squash-merge?

@vks
Copy link
Collaborator

vks commented Oct 15, 2020

The value stability tests seem to be failing. Could you please fix this?

Can you squash-merge?

Why squash? The commits seem to be independent enough.

f64 reciprocal is inexact, with residuals on the order of 1 / 2^54.

For example: the probability gen_bool(1.0 / 3) == true is:
6004799503160661/18014398509481984
Using gen_range is exact for all values of `consumed`.

NOTE: this is a value stability-breaking change

fixes rust-random#1058
@kazcw kazcw force-pushed the iteratorrandom-choose branch from 7d8af10 to a37a599 Compare October 16, 2020 17:17
@dhardy
Copy link
Member

dhardy commented Oct 17, 2020

Why squash? The commits seem to be independent enough.

Because the individual commits are tiny, and GitHub preserves the individual commits in the PR anyway (e.g. 05a7ab3 is a squash while its PR has 16 commits). For larger PRs I don't think this is optimal but for small ones it seems sensible, if not really significant.

@dhardy
Copy link
Member

dhardy commented Oct 17, 2020

OTOH these commits have useful commit messages, so I'll go ahead and use a merge commit.

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