console

console copied to clipboard

Currently, console-subscriber records every timestamp by calling SystemTime::now(). This is not necessarily the most efficient way to record a timestamp; every SystemTime::now() call requires a system call.

Also, perhaps more importantly, I don't think using system timestamps is really the most correct approach. The main reason the console records timestamps at all is to calculate durations. However, the system clock can go backwards; it's not guaranteed to be monotonic. This means that durations calculated using SystemTime might be incorrect. We should really be using monotonic time, such as Instants.

I believe the main reason console-subscriber doesn't currently use monotonic timestamps is that it sends timestamps on the wire using prost_types::Timestamp, which has a From<SystemTime> impl. Since Instants are opaque, there's no From<Instant> implementation for protobuf timestamps.

One solution might be to switch to recording timestamps using Instants, but anchor them against a fixed SystemTime, so that they can be converted into SystemTimes. The way we would do this is by recording a fixed SystemTime and Instant pair when the ConsoleLayer is created. Then, when we need to serialize an Instant timestamp, we would take the duration elapsed between that Instant and the "anchor" instant. Since the anchor Instant and anchor SystemTime represent (nearly) the same timestamp, we could then convert the Instant timestamp into a SystemTime for serialization, by adding the elapsed duration from the anchor Instant to the anchor SystemTime.

Another option we might want to consider is using the quanta crate by @tobz. This would offer a few advantages. In particular, quanta::Instant does provide an Into<prost_types::Timestamp> implementation. Additionally, quanta::Instants can be converted into u64s (unlike std's instants). This would allow them to be stored in atomics, which could be beneficial for #238. Finally, quanta may introduce less overhead compared std::time::Instant, at least on x86 platforms, where it uses rdtsc

For what it's worth: Instant::as_u64 has been removed in a yet-to-be-released change as part of slimming down the public API prior to a 1.0 release. Users have used crossbeam's AtomicCell to atomically store an Instant, though, as it can safely be transmuted to an AtomicU64.

As far as the conversion to prost_types::Timestamp, it does exist, but related to your comment about system time not being monotonic, I haven't thought very deeply about the nature of what it would mean to always convert a quanta::Instant to what is effectively std::time::SystemTime, in terms of accurately representing time that is anchored to UTC. While our calibration source is always the equivalent of CLOCK_MONOTONIC, which is adjusted to ultimately converge with an NTP-corrected version of UTC, we don't re-calibrate ourselves over time, so there is the possibility for drift.

There's likely a way, or ways, to compensate for this if conversion to an UTC-anchored time is required, and I'm happy to try and provide an honest description of what quanta can and cannot provide here if you do want to consider seriously switch to it.

For what it's worth: Instant::as_u64 has been removed in a yet-to-be-released change as part of slimming down the public API prior to a 1.0 release. Users have used crossbeam's AtomicCell to atomically store an Instant, though, as it can safely be transmuted to an AtomicU64.

Hmm, I'm a bit bummed about this, as I would have kind of preferred to be able to do it without requiring an additional dependency on crossbeam just to get the AtomicCell type. But, that's fine, I guess.

but related to your comment about system time not being monotonic, I haven't thought very deeply about the nature of what it would mean to always convert a quanta::Instant to what is effectively std::time::SystemTime, in terms of accurately representing time that is anchored to UTC.

In this case, we don't actually need a UTC-anchored timestamp at all; all these timestamps are only used to calculate durations. The only reason we send timestamps on the wire rather than durations is so that the console UI can continue to calculate durations across updates.

It's possible what we really should be doing, rather than using protobuf's timestamp type in the wire format, is sending our own monotonic, non-UTC instant type instead...but I was lazy and didn't want to write that.

I guess crossbeam_util is probably in enough people's dependency trees already that it's maybe okay to add the dependency...