fix "zero-time measurements" bug#867
Open
zooko wants to merge 23 commits into
Open
Conversation
added 5 commits
May 25, 2025 16:17
…rack down this bug where measurements are zero
Author
|
Found another place where measurements can come out 0, while running benchmarks of my project. |
preliminary port of MachAbsoluteTime and RDPTSCP measurements from smalloc half-assed and non-compiling code to debug a bug in the RDPTSCP measurement
happy (This version still doesn't compile, but if you're a Criterion developer, you might want this patch.
and then give up and tell clippy to stop warning about that
fix the compile
…s call to it (which would cause the elapsed duration to be 0) then instead say that the elapsed duration is 1.
…e by a veeeeery small positive float instead
…me is 0 and if so setting it to 1 nanosecond
… other call site I could find in this file that computes a duration -- if the duration is < 1 then set it equal to 1.
…ments, fix a couple of docs
…act that it is not allowed to return 0; remove unit test that Criterion allows Measurements to return 0.
…urements: if it is 0, then set it to 1 nanosecond I don't 100% know that this is a problem, but I've getting irreproducible failures that mention "NaN", and I investigated and added a bunch of asserts, and determined that there are a couple of spots that divide by a number that is sometimes 0, and I guess that number might sometimes be 0 due to elapsed_time sometimes being 0. So, I don't know this is really a good fix for a specific bug, but maybe so, and it probably doesn't hurt.
…ing point numbers
hkBst
reviewed
Jun 26, 2025
| black_box(routine()); | ||
| } | ||
| self.value = self.measurement.end(start); | ||
| assert!(self.measurement.to_f64(&self.value) > f64::MIN_POSITIVE, "{}", self.measurement.to_f64(&self.value)); |
There was a problem hiding this comment.
should this not be ">=" or just "> 0.0"?
hkBst
reviewed
Jun 26, 2025
| .first() | ||
| .unwrap(); | ||
| assert!(t_now - t_prev > f64::MIN_POSITIVE); | ||
| let t = (t_prev + 2. * t_now) / 5.; |
There was a problem hiding this comment.
using "git rebase -i HEAD~23" you can change this commit to a fixup of your first commit.
hkBst
reviewed
Jun 26, 2025
| [lints.clippy] | ||
| uninlined_format_args = "allow" | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
Perhaps move the format_arg changes to a separate PR?
|
@zooko you should really clean up the commit history and remove temporary debug stuff and merge fixup commits with the commit they fix up. Also separate out all unrelated stuff, like clippy fixes. Perhaps then this stands a chance of being accepted. |
hkBst
reviewed
Jun 27, 2025
Comment on lines
+364
to
+369
| #[cfg(target_arch = "x86_64")] | ||
| pub mod plat_x86_64 { | ||
| use cpuid; | ||
| use crate::{Throughput, Measurement}; | ||
| use crate::measurement::ValueFormatter; | ||
| use core::arch::x86_64; |
hkBst
reviewed
Jun 27, 2025
Comment on lines
+279
to
+294
| /// A slightly better timer for Apple platforms. | ||
| pub mod plat_apple { | ||
| use mach_sys::mach_time::mach_absolute_time; | ||
| use mach_sys::mach_time::{mach_absolute_time, mach_timebase_info}; | ||
| use mach_sys::kern_return::KERN_SUCCESS; | ||
| use crate::measurement::ValueFormatter; | ||
| use std::mem::MaybeUninit; | ||
| use crate::{Measurement, Throughput}; | ||
|
|
||
| #[derive(Default)] | ||
| struct MachAbsoluteTimeMeasurement { } | ||
| /// Use the `mach_absolute_time()` clock, which is slightly better | ||
| /// in my [tests](https://github.com/zooko/measure-clocks) than | ||
| /// `Instant::now()`. | ||
| pub struct MachAbsoluteTimeMeasurement { } | ||
|
|
||
| struct MachAbsoluteTimeValueFormatter { } | ||
|
|
||
| use mach_sys::mach_time::mach_timebase_info; | ||
| use mach_sys::kern_return::KERN_SUCCESS; | ||
| /// Formatter | ||
| pub struct MachAbsoluteTimeValueFormatter { } |
hkBst
reviewed
Jun 27, 2025
| } | ||
| fn end(&self, i: &Self::Intermediate) -> Self::Value { | ||
| ( unsafe { mach_absolute_time() } - i ) | ||
| ( unsafe { mach_absolute_time() } - i ) |
hkBst
reviewed
Jun 27, 2025
Comment on lines
+358
to
366
| fn lt(&self, val: &Self::Value, other: &Self::Value) -> bool { | ||
| val < other | ||
| } | ||
| fn debugprint(&self, val: &Self::Intermediate, other: &Self::Value) { | ||
| eprintln!("val: {val:?}, other: {other:?}"); | ||
| } | ||
| fn to_f64(&self, val: &Self::Value) -> f64 { | ||
| *val as f64 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes #862