Skip to content

Comments

Remove strlen in C++ benchmark#63

Merged
ulfjack merged 1 commit intoulfjack:masterfrom
dtolnay:bench
Aug 8, 2018
Merged

Remove strlen in C++ benchmark#63
ulfjack merged 1 commit intoulfjack:masterfrom
dtolnay:bench

Conversation

@dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Aug 7, 2018

Avoiding strlen makes the benchmark more comparable across languages that would not want a null terminator, such as Rust.

This strlen in bench32 was 20% of the iteration time on my machine.

Avoiding strlen makes the benchmark more comparable across languages
that would not want a null terminator, such as Rust.

This strlen in bench32 was 20% of the iteration time on my machine.
StephanTLavavej added a commit to StephanTLavavej/ryu that referenced this pull request Aug 7, 2018
Accepting ulfjack#63 will change bench32()'s timings (in a desirable way).
That makes this a good time to simplify bench64() (changing its timings
in a minor, neutral way).

Instead of calling a 32-bit Mersenne Twister twice, which exactly
replicated the previous third-party implementation, we can simply use
the Standard's 64-bit Mersenne Twister. It generates every uint64_t
with equal probability, so the test is essentially unaffected. (The
exact values differ from the previous technique, so the timings might
change very slightly, as if a different seed were chosen.)

Calling std::mt19937 twice versus calling std::mt19937_64 once has
somewhat different performance characteristics, but the benchmark
doesn't time this, so it doesn't matter.
@ulfjack ulfjack merged commit 91a7d8c into ulfjack:master Aug 8, 2018
@ulfjack
Copy link
Owner

ulfjack commented Aug 8, 2018

Thanks! Yeah, I remember that that was a large change in the 64-bit case. Weird that I missed updating the 32-bit case. shrug

@dtolnay dtolnay deleted the bench branch August 10, 2018 07:14
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.

2 participants