Fix overflow in precise_time_ns() on Windows, #22788
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
This commit deprecates the entire libtime library in favor of the
externally-provided libtime in the rust-lang organization. Users of the
`libtime` crate as-is today should add this to their Cargo manifests:
[dependencies.time]
git = "https://github.com/rust-lang/time"
To implement this transition, a new function `Duration::span` was added to the
`std::time::Duration` time. This function takes a closure and then returns the
duration of time it took that closure to execute. This interface will likely
improve with `FnOnce` unboxed closures as moving in and out will be a little
easier.
Due to the deprecation of the in-tree crate, this is a:
[breaking-change]
cc #18855, some of the conversions in the `src/test/bench` area may have been a
little nicer with that implemented
|
I feel that this may not be the time to whip out |
|
@vadimcn you might consider taking the approach of splitting the u64 into the high- and low- order 32-bit parts, as I show here: pnkfelix@b6d2ccb Of course the approach I show there is slower than yours, since it uses two divisions (and a modulo, but that should be folded into one of the divisions if the target architecture supports doing them together) and two multiples, in place of your one division and one multiply. But it preserves the low order bits without needing |
|
@pnkfelix: But it doesn't lose any bits! Had I wanted that, I'd have simply written Now, it's true that my code will start losing low bits when (numer * denom) overflows, but that doesn't happen until we get into 20 GHz territory (10 GHz if using signed integers). These days multiprocessor machines don't seem to use RDTSC for perf counters, so the actual frequencies we are likely to see will be in MHz, not GHz. BTW, your version seems to lose low bits too, and sooner than mine. I have not yet figured out why. Check this out: http://is.gd/uX6JH4 |
|
@vadimcn weird. I did not expect that. (Update: Ah, I am at least losing bits during Anyway, I no longer object to your non-asm version. (I still agree with @alexcrichton that we need not include the asm version right now.) |
|
The asm version uses true 128-bit operations, so it doesn't lose low bits at all, which is nice, IMO. |
|
True, but using unstable language features generally makes updating said features much tougher, for example. I'm also generally pretty wary of usage of |
|
@vadimcn by the way, I'm inclined to agree with your assessment on #17845 that this overflow seems like it could also hit the unix platforms as well. (I guess there's something special about windows that I only hit it there in the overflow work.) We should definitely consider moving this portable routine that you have written out of the windows-specific code and into one of the core or std num modules, even if only for use within std alone... |
|
@pnkfelix: I'd like to hear from Mac people that this is indeed a problem, before attempting to fix it. |
which starts happening after ~2 hours of machine uptime.
vadimcn
force-pushed
the
fix-precise_time_ns
branch
from
a79a177
to
5dd001b
Compare
|
@alexcrichton: Ok, I dropped the asm version. |
…elix which starts happening after ~2 hours of machine uptime. Closes rust-lang#17845
…elix which starts happening after ~2 hours of machine uptime. Closes rust-lang#17845
which starts happening after ~2 hours of machine uptime.
Closes #17845