Skip to content
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

Handle time conversions from constants. #148

Closed
miladHakimi opened this issue Dec 2, 2022 · 1 comment · Fixed by #151
Closed

Handle time conversions from constants. #148

miladHakimi opened this issue Dec 2, 2022 · 1 comment · Fixed by #151
Assignees
Labels
bug Something isn't working

Comments

@miladHakimi
Copy link
Contributor

miladHakimi commented Dec 2, 2022

We use 2 different clocks, TimestampClock and DurationClock. They are aliases for std::chrono::system_clock and std::chrono::steady_clock. I created a wrapper around these clocks to make them easier to work with. E.g: #134,
I had 2 assumptions in mind when writing these classes.
First, the DurationClock and TimestampClock have the same precision. This caused a bug on a system in which this didn't hold. I fixed it in #145 by casting the duration from one clock to the other.

My second assumption was that the clocks always are in the nanoseconds precision. The second bug happens in this code:

class Timestamp {
 public:
  static Timestamp FromNanoseconds(int64_t nanos) {
    return Timestamp(
        TimestampClock::time_point(TimestampClock::duration(nanos)));
  }

  Timestamp(TimestampClock::time_point timestamp) : timestamp_{timestamp} {}

  int64_t ToNanoseconds() const {
    return std::chrono::nanoseconds(timestamp_.time_since_epoch()).count();
  }

 private:
  TimestampClock::time_point timestamp_;
};

The Timestamp class is a wrapper around a TimestampClock. It has a FromNanoseconds() factory method that receives an input and stores it in the underlying TimestampClock::time_point variable assuming the TimestampClock has a nanoseconds precision. That is why this line of test fails in a system that has a different clock.

https://github.com/google/vulkan-performance-layers/blob/main/layer/unittest/trace_event_log_tests.cc#L198

@kuhar kuhar added the bug Something isn't working label Dec 2, 2022
@kuhar kuhar self-assigned this Dec 3, 2022
kuhar added a commit to kuhar/performance-layers that referenced this issue Dec 6, 2022
Do not assume that the underlying clock precision is nanoseconds.

Fixes: google#148
@miladHakimi
Copy link
Contributor Author

Knowing more about time_point, duration, and clock helps solve this problem.
chrono::time_point: Is a point in time. It stores a duration type value indicating the time since the clock's epoch.
chrono::duration: Holds the number of ticks, where the tick period is a compile-time rational fraction representing the time in seconds from one tick to the next.
clock: Is a bundle containing a time_point and a duration. The origin of the clock's time_point is referred to as the clock's epoch.

To create a time_point from a number, we should create a TimestampClock::duration. The given number is used to set the ticks of the duration variable of the time_point. Since we want this number to represent a duration in nanoseconds, the period of the duration must be in nanoseconds. The problem with the current FromNanoseconds is that the TimestampClock can vary from one system to another. Hence, the result will not always be a duration in nanoseconds precision.
To fix this issue, we could write something like this:

std::chrono::nanoseconds duration_ns{nanos};

To create a time_point from the duration_ns, we must convert the ticks in duration_ns into the ticks in a TimestampClock::duration. To do that, we have to explicitly cast the duration; otherwise, the implicit cast leads to a compile time error when the precision of the TimestampClock::duration is less than nanoseconds. The explicit cast tells the compiler we are aware of the possible data loss.

TimestampClock::duration casted_dur = std::chrono::duration_cast<TimestampClock::duration>(duration_ns);

Now that we have a duration in TimestampClock::duration, we can create a TimestampClock::time_point from it.

 TimestampClock::time_point timestamp(casted_dur);

A sample code for different casting scenarios: https://godbolt.org/z/5cEEoGdon

references:

kuhar added a commit to kuhar/performance-layers that referenced this issue Dec 7, 2022
Do not assume that the underlying clock precision is nanoseconds.
Use regex to match timestamps in unit test to avoid inherent fp precision
differences.

Fixes: google#148
@kuhar kuhar closed this as completed in #151 Dec 7, 2022
kuhar added a commit that referenced this issue Dec 7, 2022
Do not assume that the underlying clock precision is nanoseconds. Use
regex to match timestamps in unit test to avoid inherent fp precision
differences.

Fixes: #148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants