[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

[macOS] OS::GetCurrentMonotonicTicks should probably use CLOCK_UPTIME_RAW. #55071

Closed
knopp opened this issue Feb 29, 2024 · 6 comments
Closed

[macOS] OS::GetCurrentMonotonicTicks should probably use CLOCK_UPTIME_RAW. #55071

knopp opened this issue Feb 29, 2024 · 6 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. needs-info We need additional information from the issue author (auto-closed after 14 days if no response)

Comments

@knopp
Copy link
Contributor
knopp commented Feb 29, 2024 •
edited
Loading

This would corresponds to other macOS API such as CACurrentMediaTime(). Otherwise we have a problem where DisplayLink callback gives target timestamp in based on CLOCK_UPTIME_RAW, but fml::TimePoint expects CLOCK_MONOTONIC_RAW.

Also the documentation for mach_absolute_time suggests clock_gettime_nsec_np(CLOCK_UPTIME_RAW) as suitable replacement and clock_gettime_nsec_np(CLOCK_UPTIME_RAW) is not mentioned in the prohibited boot time API list.

@rmacnak-google
@knopp knopp mentioned this issue Feb 29, 2024
Merged
8 tasks
@cbracken
Copy link
Member

@rmacnak-google
For reference, the related patch that switched from mach_absolute_time to clock_gettime_nsec_No problem! which caused the issue:
https://dart-review.googlesource.com/c/sdk/+/348044

cbracken pushed a commit to flutter/engine that referenced this issue Feb 29, 2024
@knopp
Revert "[macOS] Use CVDisplayLink to drive repaint (#49159)" (#51095)
dd3383d 
The original PR assumed that `fml::TimePoint` has same base as
`CACurrentMediaTime()`. However due to recent change in Dart SDK that's
no longer true. Dart SDK
[replaced](https://dart-review.googlesource.com/c/sdk/+/348044?tab=comments)
call to `mach_absolute_time` with
`clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW)`, while
`CACurrentMediaTime()` corresponds to `CLOCK_UPTIME_RAW`.

This needs to be fixed either in Dart SDK
dart-lang/sdk#55071 or the original PR needs
to be relanded with appropriate conversions.

This reverts commit 21474ee.

*Replace this paragraph with a description of what this PR is changing
or adding, and why. Consider including before/after screenshots.*

*List which issues are fixed by this PR. You must list at least one
issue.*

*If you had to change anything in the [flutter/tests] repo, include a
link to the migration guide as per the [breaking change policy].*

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
@rmacnak-google
Copy link
Contributor

mach_absolute_time and clock_gettime_nsec_np(CLOCK_UPTIME_RAW) access the same clock and so have the same fingerprinting privacy implications.

Dart_TimelineGetTicks/Micros and Timeline.now do not promise any particular clock, only that it is suitable for measuring duration and for use with Dart_RecordTimelineEvent. If an application needs some particular clock, it should access that clock itself instead of assuming Dart timeline APIs use the same clock.

@lrhn lrhn added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Feb 29, 2024
@knopp
Copy link
Contributor Author
knopp commented Feb 29, 2024 •
edited by cbracken
Loading

So does CACurrentMediaTime() and yet it's not mentioned in the document. I would assume there is a reason why apple singled out mach_absolute_time in the document and not mentioned either clock_gettime_nsec_np(CLOCK_UPTIME_RAW) or CACurrentMediaTime().

The issue here is that Dart_TimelineGetTicks is used in fml::TimePoint in Flutter, which right now leaks through the embedder API. I opened this issue because I think that maybe the original PR was overly cautious, but if that's not the case we'll need to handle this in the embedder.

@knopp
Copy link
Contributor Author
knopp commented Mar 1, 2024

I gave this another though - I think the correct solution is to not assume anything about the embedder API clock and always rebase the time, i.e. CACurrentMediaTime() -> FlutterEngineGetCurrentTime().

I still think that maybe CLOCK_MONOTONIC_RAW is bit too defensive since we're using CACurrentMediaTime() anyway in other places of engine and can't really get rid of it, but the engine API doesn't make any guarantees about the clock and embedder should handle that properly.

@a-siva
Copy link
Contributor
a-siva commented Mar 6, 2024

@knopp is there anything actionable on the Dart side for this ?

@a-siva a-siva added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Mar 6, 2024
@knopp
Copy link
Contributor Author
knopp commented Mar 6, 2024

I don't think so. I'll close the issue.

@knopp knopp closed this as completed Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. needs-info We need additional information from the issue author (auto-closed after 14 days if no response)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@knopp @cbracken @lrhn @rmacnak-google @a-siva