-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
special case for integer log10 #86930
Conversation
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
Some timings of old versus new:
Benchmark details
macro_rules! bench {
($T:ident, $old:ident, $new:ident) => {
// for example for u32, compute:
// * 1, 2, 3, ..., 255
// * 1 << 8, 2 << 8, 3 << 8, ..., 255 << 8
// * 1 << 16, 2 << 16, 3 << 16, ..., 255 << 16
// * 1 << 24, 2 << 24, 3 << 24, ..., 255 << 24
#[bench]
fn $old(bench: &mut Bencher) {
for n in 0..(<$T>::BITS / 8) {
bench.iter(|| {
for i in 1..=(u8::MAX as $T) {
let i = i << (n * 8);
black_box(old::$T(i, 10));
}
});
}
}
#[bench]
fn $new(bench: &mut Bencher) {
for n in 0..(<$T>::BITS / 8) {
bench.iter(|| {
for i in 1..=(u8::MAX as $T) {
let i = i << (n * 8);
black_box(int_log10::$T(i));
}
});
}
}
};
}
bench! { u8, u8_old, u8_new }
bench! { u16, u16_old, u16_new }
bench! { u32, u32_old, u32_new }
bench! { u64, u64_old, u64_new }
bench! { u128, u128_old, u128_new } |
This comment has been minimized.
This comment has been minimized.
@bors rollup=never |
@bors r+ |
📌 Commit ed76c11 has been approved by |
⌛ Testing commit ed76c11 with merge c014ff7e6d366543042d6b70e40d023cf27dbdf7... |
💔 Test failed - checks-actions |
The failure seems spurious, or am I missing something? |
1 | ||
} else { | ||
0 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried doing a binary search instead a linear chain of inequalities? I'm not sure if it would actually be faster due to unpredictable branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try it and it seemed to be slightly slower (just over the error thresholds), so I thought it better to leave the simpler code since there was no clear gain.
@bors retry |
⌛ Testing commit ed76c11 with merge 8879bfb0f219936a8255549ba640b7f2f40b793d... |
💥 Test timed out |
@bors retry |
⌛ Testing commit ed76c11 with merge 3f75f8508a00f5046784fa6cec436c926ab6a1b8... |
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Now that #80918 has been merged, this PR provides a faster version of
log10
.The PR also adds some tests for values close to all powers of 10.