refactor: Simplify GetTime #24871

MarcoFalke commented at 11:18 AM on April 16, 2022: member

The implementation of GetTime is confusing:

The value returned by GetTime is assumed to be equal to GetTime<std::chrono::seconds>(). Both are mockable and the only difference is return type, the value itself is equal. However, the implementation does not support this assumption.
On some systems, time_t might be a signed 32-bit integer (https://en.cppreference.com/w/c/chrono/time), thus breaking in the year 2038, whereas GetTime<std::chrono::seconds> does not. Also, time_t might be -1 "on error", where "error" is unspecified.
GetTime<std::chrono::seconds> calls GetTimeMicros, which calls GetSystemTime, which calls std::chrono::system_clock::now, which doesn't have the above issues. See https://en.cppreference.com/w/cpp/chrono/system_clock/now
GetTimeMicros and the internal-only GetSystemTime will likely be renamed (to clarify they are the non-mockable non-monotonic system time) or removed in the future to be replaced by appropriate std::chrono::time_point<Clock> getters.

Fix all issues by:

making GetTime() an alias for GetTime<std::chrono::seconds>().count().
inlining the needed parts of GetSystemTime directly instead of needlessly increasing the function call stack with functions that are likely to be removed in the future.

Simplify GetTime 0000a63689

MarcoFalke added the label Refactoring on Apr 16, 2022

laanwj commented at 11:35 AM on April 16, 2022: member

Concept ACK, GetTime should either use std::chrono or C time functions, not a mix for different types, that's really weird.

in src/util/time.cpp:123 in 0000a63689

 119 | @@ -129,6 +120,8 @@ int64_t GetTimeSeconds()
 120 |      return int64_t{GetSystemTime<std::chrono::seconds>().count()};
 121 |  }
 122 |  
 123 | +int64_t GetTime() { return GetTime<std::chrono::seconds>().count(); }

prusnak commented at 12:41 PM on April 16, 2022:

Nit: Why not using the same syntax/formatting as above in GetTimeSeconds?

MarcoFalke commented at 12:59 PM on April 16, 2022:

I think the narrowing check is a bit overkill and probably doesn't make much sense. Mostly relevant isn't the maximum number of bits in the value, but the minimum, which is 35 bits according to https://en.cppreference.com/w/cpp/chrono/duration .

In either case the function is deprecated (see comment in the header file), and will likely be removed in the future anyway.

brunoerg commented at 3:32 PM on April 16, 2022: member

Strong Concept ACK

DrahtBot commented at 5:01 AM on April 17, 2022: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24697 (refactor address relay time by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

theStack commented at 5:16 PM on April 17, 2022: member

Concept ACK

martinus commented at 12:56 PM on April 18, 2022: contributor

Code review, untested ACK 0000a63689036dc4368d04c0648a55fdf507932f. By the way strictly speaking std::chrono::system_clock is only guaranteed to be based on the unix epoch starting with C++20: https://en.cppreference.com/w/cpp/chrono/system_clock But I have never seen a system where that is not already the case.

martinus approved

fanquake commented at 1:13 PM on April 18, 2022: member

But I have never seen a system where that is not already the case. @martinus note that we have ChronoSanityCheck() which should fail in that case.

theStack approved

theStack commented at 4:13 PM on April 18, 2022: member

Code-review ACK 0000a63689036dc4368d04c0648a55fdf507932f

I was first confused on why we don't have to std::chrono::duration_cast<T> the mock time, but C++ seems to be nice enough to implicitly convert between duration types if there is no precision loss (e.g. in our case seconds -> milliseconds and seconds -> microseconds). Note though that on master explicit instantiation with something less precise than seconds works:

template std::chrono::minutes GetTime();

while on the PR branch it doesn't. OTOH the chance that we ever need less precision than seconds is close to zero, so I think it's fine.

MarcoFalke commented at 7:35 AM on April 19, 2022: member

Yes, the change was intentional. Given that we've never used std::chrono::minutes GetTime, it seems unlikely to ever be used in the future. Also, all of this code will likely be removed anyway soon(TM) and be replaced by a Now helper that returns time points (a type denoting a duration with a clock type embedded) instead of "flat" durations.

fanquake requested review from ajtowns on Apr 19, 2022

in src/util/time.cpp:73 in 0000a63689

  69 | @@ -80,11 +70,12 @@ template <typename T>
  70 |  T GetTime()
  71 |  {
  72 |      const std::chrono::seconds mocktime{nMockTime.load(std::memory_order_relaxed)};
  73 | -
  74 | -    return std::chrono::duration_cast<T>(
  75 | +    const auto ret{

ajtowns commented at 8:12 AM on April 19, 2022:

const T ret ?

Dropping the duration_cast around mocktime means you can't use GetTime<T>() with T set to minutes or hours or similar. Seems fine, just noting.

MarcoFalke commented at 8:27 AM on April 19, 2022:

I think auto is fine for now. This will also reduce the diff in commit 8299fd2e9703a660ca948a5f29e9d3d2bf9abdf2, where the type is changed from T to Clock::time_point::duration.

fanquake merged this on Apr 19, 2022

fanquake closed this on Apr 19, 2022

MarcoFalke deleted the branch on Apr 19, 2022

sidhujag referenced this in commit 5eb935fb62 on Apr 19, 2022

Fabcien referenced this in commit 06a488e349 on Jul 22, 2022

Fabcien referenced this in commit 7177049f17 on Jul 22, 2022

DrahtBot locked this on Apr 19, 2023