webatintel / aquarium

BSD 3-Clause "New" or "Revised" License
23 stars 7 forks source link

Use std::chrono where appropriate #192

Closed hujiajie closed 3 years ago

hujiajie commented 3 years ago
hujiajie commented 3 years ago

Diff between master and proposal A using duration

Diff between master and proposal B using ticks

Diff between proposal A and proposal B

hujiajie commented 3 years ago

Here is the implementation using nanosecond as our time measurement unit. Comparing with other alternative proposals, it worth pointing out the following shortcomings:

int64_t frequency = 200000000LL;  // 200 MHz
int64_t tick;
int64_t nanosecond;

tick = 100000000LL;  // 100M ticks, or 500,000,000 nanoseconds
nanosecond = tick / frequency * 1000000000;  // BAD, the result is 0

tick = 10000000000LL;
nanosecond = tick * 1000000000 / frequency;  // BAD, tick * 1000000000 overflows
                                             // In practice it's not a corner case!

// The libc++ way:
nanosecond = tick / frequency * 1000000000 + tick % frequency * 1000000000 / frequency;

Screenshot_20210301_231720

The second implementation is based on std::chrono::duration and aims to mitigate these concerns. The canonical unit is second now, however the histogram probably still stays imperfect:

Screenshot_20210301_231420

The third path uses std::chrono::duration too, and the idea is:

While it sounds in this way histogram plotting won't be hurt anymore, please do read the code as talk is always cheap.