-
Notifications
You must be signed in to change notification settings - Fork 6k
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
TimePoint::Now uses DartTimestampProvider #27737
Conversation
fml/time/dart_timestamp_provider.cc
Outdated
DartTimestampProvider::~DartTimestampProvider() = default; | ||
|
||
fml::TimePoint DartTimestampProvider::Now() { | ||
return fml::TimePoint::FromTicks(Dart_TimelineGetTicks()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I don't understand this API but this seems off to me. Here, we are depending on the timebases of fml::TimePoint
and Dart_Timeline
to be identical. Looking at what was recently exposed we'd need to take into the timebase using Dart_TimelineGetTicksFrequency
. Maybe we are getting away with these because both are in nanoseconds?
TEST(TimePoint, DartClockIsMonotonic) { | ||
using namespace std::chrono_literals; | ||
const auto t1 = DartTimelineTicksSinceEpoch(); | ||
std::this_thread::sleep_for(1us); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chinmaygarde that was a good catch, I am now converting from frequency base to nano seconds. But looks like on windows the frequency can be less than once per nano second (based on the test failure on windows bots), so I've added this sleep for 1us. Is it reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also the resolution seems to be < 1us (on windows), so had to change to ASSERT_LE
rather than ASSERT_LT
:-/ but it was always the case (not a regression)
Fixes flutter/flutter#76875