Fixed precision issue at high progress rates #293
Conversation
src/utils.rs
Outdated
|
|
||
| /// This helper function takes an explicit elapsed duration so that it could also be used in | ||
| /// test code, which must be deterministic. | ||
| fn record_step_with_elapsed_time(&mut self, value: u64, elapsed: Duration) { |
There was a problem hiding this comment.
Is there some way we can test this that does not depend on just extracting a single-caller method here?
There was a problem hiding this comment.
An alternative option that I have considered was to just add an current time Instant parameter to record_step() (instead of calling .elapsed() internally), however I have discarded that approach as it's changing the public API even more. I agree that single-caller helper is not ideal. It's a compromise between deterministic testability and public API churn.
There was a problem hiding this comment.
Actually, Estimate is not part of the public API at all, so I think we can just change whatever we like here.
There was a problem hiding this comment.
Aha, ok. pub-s gave the impression that it is. I'll see what record_step() idea looks like.
There was a problem hiding this comment.
Looks a bit neater. Also improved test_time_per_step() itself a little bit.
There was a problem hiding this comment.
Let's change record_step to take a now: Instant argument, instead.
src/state.rs
Outdated
| @@ -84,16 +84,23 @@ impl ProgressState { | |||
| } | |||
|
|
|||
| /// Return the current average time per step | |||
| #[allow(dead_code)] // Keep for API compatibility. Should deprecate. | |||
There was a problem hiding this comment.
I think I'd like to just keep this but also add some explanation to the docs here about the limitations of Duration resolution.
There was a problem hiding this comment.
What might be a reason to keep Duration around? It might be a source of bugs for users of the crate, similar to the bugs this PR fixes.
There was a problem hiding this comment.
I think that Duration as a type is easier to work with than a floating point type: it has a bunch of facilities that cover common use cases for dealing with durations, and it's more obvious to understand when you have it in your code. There are definitely use cases where the 1ns resolution is not an issue, so those users might like to get the more obvious type.
There was a problem hiding this comment.
I'm definitely not going to argue about this much further and it's not a hill I'm willing to die on, but it seems that average time per step can very easily get <1ns when dealing with bytes. Surfacing this to the user of the library allows them to introduce a bug that won't be caught at compile time and may not even happen until their application runs on a sufficiently fast machine (i.e. reading files from SATA vs NVMe SSD or using 1 vs 10 GbE). I'll be happy with just the minimal fix that allows built-in estimates and draw rate throttling functionality to work in my application :)
src/state.rs
Outdated
| secs_to_duration(self.est.seconds_per_step()) | ||
| } | ||
|
|
||
| /// Return the current average time in seconds per step. |
There was a problem hiding this comment.
Style nit: please strip the period from the first comment line (it's a sentence fragment, not a sentence), and add an empty line between the two lines.
kayru
force-pushed
the
time_per_item_fix
branch
2 times, most recently
from
9aea8f4
to
1f4e4d2
Compare
|
I've gone ahead and just merged this, will iterate a bit on it. |
|
Yay, thank you! ❤️ |
Fixes #10.
Estimate::time_per_step()returns aDuration, which has a limited precision of 1ns. Unfortunately, this makes it impossible to handle high progress rates (>1 billion items per second).The proposed solution is to add
seconds_per_step()and deprecate time_per_step() in a future release. This PR intentionally leaves the old APIs in place to preserve backwards compatibility.