homepage, bench improvement and doc fix #317
Conversation
| } | ||
| black_box(accum); |
There was a problem hiding this comment.
I had on my wish-list to investigate the benchmark 'problems'. So nice to see 😄. I also noticed some change if you set b.bytes before b.iter. Is that something you can confirm?
There was a problem hiding this comment.
Never tried that.
Something similar might be possible for the fill_bytes stuff, but it's probably less significant (already this has little impact on 64-bit outputs).
There was a problem hiding this comment.
It made a difference, but not any more with the accumulator.
|
I would like to take a closer look, that will probably be tomorrow. |
|
Added a bit more doc. I hope it's not too scary 😱 At this point maybe it's not worth mentioning the |
|
Your change really helps with the new parallel codegen. The same would be necessary for the distributions code. I gave it a try in https://github.com/pitdicker/rand/commits/doc_bench. Do you want to take that commit, or maybe do the same in some cleaner way? Then there is only one test left that is badly affected by parallel codegen: |
| @@ -6,7 +6,7 @@ license = "MIT/Apache-2.0" | |||
| readme = "README.md" | |||
| repository = "https://github.com/rust-lang-nursery/rand" | |||
| documentation = "https://docs.rs/rand" | |||
| homepage = "https://github.com/rust-lang-nursery/rand" | |||
| homepage = "https://crates.io/crates/rand" | |||
There was a problem hiding this comment.
I don't like either 😄. Every time I look up a crate on crates.io is surprisingly unhelpful, and I quickly click through to the repository or docs.rs.
But this seems like a good change.
rand-core/Cargo.toml
Outdated
| @@ -4,9 +4,9 @@ version = "0.1.0-pre.0" | |||
| authors = ["The Rust Project Developers"] | |||
| license = "MIT/Apache-2.0" | |||
| readme = "README.md" | |||
| repository = "https://github.com/rust-lang-nursery/rand" | |||
| repository = "https://github.com/rust-lang-nursery/rand/tree/master/rand-core" | |||
There was a problem hiding this comment.
I don't think this is right, as you can't do git checkout https://github.com/rust-lang-nursery/rand/tree/master/rand-core. This should just be the base repository, and cargo figures things out.
rand-core/Cargo.toml
Outdated
| @@ -4,9 +4,9 @@ version = "0.1.0-pre.0" | |||
| authors = ["The Rust Project Developers"] | |||
| license = "MIT/Apache-2.0" | |||
| readme = "README.md" | |||
| repository = "https://github.com/rust-lang-nursery/rand" | |||
| repository = "https://github.com/rust-lang-nursery/rand/tree/master/rand-core" | |||
| documentation = "https://docs.rs/rand" | |||
There was a problem hiding this comment.
Does this line need changing?
(documentation = "https://docs.rs/rand")
src/lib.rs
Outdated
| /// `(&mut rng).gen()`, (3) in some cases `mut rng: R` is required, and (4) | ||
| /// since `R` may not be a reference (it has no reborrow requirement) and no | ||
| /// `Copy` bound, there is no choice but to move or double-reference uses of | ||
| /// `rng: R` within `foo`. Both 2 and 3 are probably Rust bugs, but 4 isn't |
There was a problem hiding this comment.
This is a sentence of 12 lines. Do you happen to be a lawyer? 😄
src/lib.rs
Outdated
| /// - `fn foo<R: Rng>(rng: &mut R)` will not match type-erased `Rng`s; in order | ||
| /// to support dynamic dispatch use either | ||
| /// `fn foo<R: Rng + ?Sized>(rng: &mut R)` or `fn foo<R: Rng>(rng: R)` | ||
| /// (but see next point about the latter). |
There was a problem hiding this comment.
Added a bit more doc. I hope it's not too scary 😱
You give the technical reasons, and that is supposed to be a plus for programmers. But maybe it is good to help users a little. Maybe add something like:
Tip if you want to use
Rngin trait bounds:
The basic pattern isfn foo<R: Rng + ?Sized>(rng: &mut R).This way implementations can use the methods from both
RngCoreandRng, support dynamic dispatch (thanks to?Sized), and ...Technical details:
...
benches/distributions.rs
Outdated
| let mut accum: $ty = 0; | ||
| for _ in 0..::RAND_BENCH_N { | ||
| accum = accum.wrapping_add(rng.gen_range($low, high)); | ||
| high += fake_increment; | ||
| high += $incr; // force recalculation of range each time |
There was a problem hiding this comment.
How about high = high.wrapping_add(1) & std::%ty::MAX, so it also works for i8? (Not thought deeply about it to be honest)
There was a problem hiding this comment.
I think that would still wrap, and end up less than low? Current version seems to work anyway.
|
I removed the generic usage doc (still working on that stuff). Rest is okay to merge? |
|
Yes. What did you think of #317 (comment)? |
|
Didn't you see my reply? |
|
Sorry. I was comparing the benchmarks with and without parallel codegen, and checking the generated assembly on master, your branch, and my suggestion with a mask. But without the mask, all the range code is completely optimized out, at least for |
|
Ah, |
homepage, bench improvement and doc fix
No description provided.