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.

Sign up for GitHub

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

Jump to bottom

Improve doc on implementing Default and Clone #383

Merged
merged 5 commits into from
Apr 16, 2018
Merged

Improve doc on implementing Default and Clone #383

merged 5 commits into from
Apr 16, 2018

Conversation

pitdicker
Copy link
Contributor

Fixes #378.

cc @newpavlov

@newpavlov
Copy link
Member

Looks good to me!

vks
vks approved these changes Apr 10, 2018
View reviewed changes
Copy link
Collaborator

@vks vks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me as well!

@pitdicker pitdicker mentioned this pull request Apr 10, 2018
Merged
dhardy
dhardy reviewed Apr 11, 2018
View reviewed changes
rand_core/src/lib.rs Outdated
/// support optional at the crate level in PRNG libs.
/// - `Clone`, but only if the clone will have identical output to the original
/// (i.e. be a true clone), or if the clone would be another handle to the
/// same generator.
Copy link
Member

Choose a reason for hiding this comment

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

I never really got the point of saying this. If you put anything in an Rc<..> type then clone() will copy the reference, not the object — so why even say it?

If you're going to be pedantic like this then you might as well update the next point about Copy too, though I don't see much point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it from your comment, but now understand why you left it out. With OsRng in mind I think it starts to make more sense, as that is the same principle but without Rc<..>?

Copy link
Member

Choose a reason for hiding this comment

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

Sort of. I'm wondering if we should say much about Clone at all now. Maybe just that on normal PRNGs clone should imply identical output? Even that may not make sense given that we might try to make StdRng reseed on clone in the future(?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just reworded it as "Clone if possible"

@pitdicker
Copy link
Contributor Author

Spelling out my concerns on implementing Clone for JitterRng in #384 (comment) helped me seeing it a bit more clearly, and I now believe it to not really be a problem (see the commit).

Even that may not make sense given that we might try to make StdRng reseed on clone in the future(?).

I think you mean ReseedingRng (StdRng does not do reseeding)? Did we discuss that before, as it is something I eventually would like to see? (No success yet implementing that, and it would also require EntropyRng to implement clone in practice)

@dhardy
Copy link
Member

dhardy commented Apr 12, 2018

Yes, I meant ReseedingRng. Sorry, forgetting that StdRng doesn't do the reseeding; I wonder if there is any point in it not doing so given that it's not supposed to be reproducible anyway and there's negligible aggregate performance difference?

I don't see any reason ReseedingRng can't reseed on clone?

@pitdicker
Copy link
Contributor Author

Added a commit to implement Clone for ReseedingRng. It needs some more work (test+documentation), but do you think it is correct?

@pitdicker pitdicker changed the title Improve doc on implementing Default Improve doc on implementing Default and Clone Apr 12, 2018
@vks
Copy link
Collaborator

vks commented Apr 12, 2018 •
edited
Loading

Does it make sense to implement Clone when it is equivalent to just generating a new one?

@pitdicker
Copy link
Contributor Author

Does it make sense to implement Clone when it is equivalent to just generating a new one?

I don't care much about the Clone implementations to be honest. But I suppose it helps with generic code, even when it sometimes basically means reinitializing an RNG.

@pitdicker pitdicker mentioned this pull request Apr 13, 2018
Merged
dhardy
dhardy reviewed Apr 13, 2018
View reviewed changes
src/jitter.rs Outdated
@@ -51,6 +51,7 @@ const MEMORY_SIZE: usize = MEMORY_BLOCKS * MEMORY_BLOCKSIZE;
/// [Jitterentropy](http://www.chronox.de/jent.html) version 2.1.0.
///
/// [`OsRng`]: ../os/struct.OsRng.html
#[derive(Clone)]
Copy link
Member

Choose a reason for hiding this comment

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

Don't we at the very least need to clear data_half_used to prevent next_u32 returning the same result from each part?

Otherwise we could simply implement this with JitterRng::new — except that timer might not be the default function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Glad you are sharp 😄.

src/reseeding.rs
inner: self.inner.clone(),
reseeder: self.reseeder.clone(),
threshold: self.threshold,
bytes_until_reseed: 0, // reseed clone on first use
Copy link
Member

Choose a reason for hiding this comment

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

Nice way of forcing a reseed!

@pitdicker pitdicker force-pushed the impl_default_doc branch 2 times, most recently from 6394566 to a68ec1d Compare April 13, 2018 16:26
dhardy
dhardy approved these changes Apr 15, 2018
View reviewed changes
src/jitter.rs
data: self.data,
rounds: self.rounds,
timer: self.timer,
mem_prev_index: self.mem_prev_index,
Copy link
Member

Choose a reason for hiding this comment

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

Do you think mem_prev_index should be adjusted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we are okay leaving it as is. They probably have different locations for the memory slice on the stack. Having the same location twice should not make the CPU much better at predicting. And after the first round the values will diverge, as memaccess is called a variable number of times.

@dhardy
Copy link
Member

dhardy commented Apr 16, 2018

Okay, I was going to merge, but there are conflicts.

@pitdicker
Copy link
Contributor Author

Rebased

@dhardy dhardy merged commit 7b14992 into rust-random:master Apr 16, 2018
@pitdicker pitdicker deleted the impl_default_doc branch April 29, 2018 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhardy dhardy dhardy approved these changes

@vks vks vks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pitdicker @newpavlov @dhardy @vks