[iOS] Pass NSTimeInterval instead of microseconds to Vsync tracing#186190
Conversation
Fixes an issue in `FlutterVSyncClient` where platform vsync timestamps were reported incorrectly to the timeline. In flutter#185918 I introduced the Obj-C tracing API, which originally used microseconds as an int64_t. During review this was updated to use NSTimeInterval to be more idiomatic, and type-safe -- as we see here, int64_t doesn't enforce units at all. This fixes the timeline recording issue. This code will be replaced in a followup that migrates off the C++ types entirely: flutter#186166 Issue: flutter#112232
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
test-exempt: refactoring that would negate the value of any added tests is already in review |
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM
It's almost like having a bunch of important Obj-C types just being typedefs of other types isn't great for compile-time correctness...
|
The way I propose to make this testable is to support injecting a trace writer into tracing via a Test extension somewhat similar to what I did with logging here (lines 239-243): The reference patch with the logging change won't land for unrelated reasons, but you get the idea. |
|
A reason for requesting a revert of flutter/flutter/186190 could |
|
Reason for revert: the previous PR caused a G3 crash, so I have to revert this too |
|
Unable to create the revert pull request due to ProcessException: Standard out |
|
This was rolled in by cl/912159861, before the root cause PR was rolled, so no need to revert. |
Fixes an issue in
FlutterVSyncClientwhere platform vsync timestamps were reported incorrectly to the timeline.In #185918 I introduced the Obj-C tracing API, which originally used microseconds as an int64_t. During review this was updated to use NSTimeInterval to be more idiomatic and use a clear type in place of int64_t for the timestamp. The tests were updated but the callsite was not.
Authoring a test that would differentiate microseconds from fractional seconds (NSTimeInterval) is infeasible in the current state without significant refactoring, however, that refactoring is coming in followup patch:
#186166
That patch could be landed as an alternative as it also contains the fix.
Issue: #112232
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.