Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

FrameTiming build start timestamp fix and add vsync start timestamp #20229

Merged
merged 8 commits into from
Aug 8, 2020
Merged

FrameTiming build start timestamp fix and add vsync start timestamp #20229

merged 8 commits into from
Aug 8, 2020

Conversation

CareF
Copy link
Contributor

@CareF CareF commented Aug 4, 2020

Description

See flutter/flutter#62689
The original build_start time stamp is now vsync_start, and the current build time closer to what we get from timeline events.

Related Issues

fixes flutter/flutter#62689

Tests

I modify the 'FrameTiming.toString has the correct format' test in testing/dart/window_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@CareF CareF requested review from iskakaushik and liyuqian August 4, 2020 17:20
@auto-assign auto-assign bot requested a review from gw280 August 4, 2020 17:22
@@ -133,7 +134,8 @@ void Animator::BeginFrame(fml::TimePoint frame_start_time,
// to service potential frame.
FML_DCHECK(producer_continuation_);

last_frame_begin_time_ = frame_start_time;
last_frame_begin_time_ = fml::TimePoint::Now();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we post a trace event for VsyncSchedulingOverhead we also use fml::TimePoint::Now(). Can we unify both these and maybe post a trace event here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure I understand but I made a commit moving the TaceEventAsyncComplete line. PTAL.

@CareF CareF requested a review from iskakaushik August 4, 2020 18:28
@CareF
Copy link
Contributor Author

CareF commented Aug 5, 2020

I realized that I need also to update the framework code as in flutter/flutter#62933 . How should this work? @liyuqian
Especially packages/flutter/test/scheduler/scheduler_test.dart because this test depends on the FrameTiming data length which we modify here. This will block the CI test on the framework.

@liyuqian
Copy link
Contributor

liyuqian commented Aug 5, 2020

I realized that I need also to update the framework code as in flutter/flutter#62933 . How should this work? @liyuqian
Especially packages/flutter/test/scheduler/scheduler_test.dart because this test depends on the FrameTiming data length which we modify here. This will block the CI test on the framework.

I'd suggest adding a new FrameTiming constructor that takes named parameter for unit tests, and document that the List<int> constructor should only be used in the engine as FramePhase could change. Here are some specific steps:

  1. Introduce that named constructor in an engine PR
  2. Modify the unit tests to use the named constructor in a framework PR
  3. Land this PR by extending the named constructor without breaking the old unit tests
  4. Update the unit test by giving the new named parameter in another framework PR (likely [engine] update for frameTiming (flutter/engine#20229) flutter#62933)

Additionally, we can make the List<int> constructor private as FrameTimng._(List<int>). The hooks.dart can still access those private constructors, but I think framework won't be able to.

@liyuqian
Copy link
Contributor

liyuqian commented Aug 6, 2020

Should this PR also update FrameTiming.fromTimeStamps, and make the old List<int> constructor private?

@CareF
Copy link
Contributor Author

CareF commented Aug 6, 2020

Should this PR also update FrameTiming.fromTimeStamps, and make the old List<int> constructor private?

I just duplicated fromTimeStamps to default constructor and will remove fromTimeStamps after everything lands. @liyuqian

And I'm guessing there may be google internal tests that's calling the old constructor, but I need confirm.

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I didn't find any reference to the FrameTiming constructors when I searched \bFrameTiming\b -file:third_party file:.dart inside the Googler internal code search tool.

@CareF CareF added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 8, 2020
@fluttergithubbot fluttergithubbot merged commit 409a5e5 into flutter:master Aug 8, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Engine] FrameTiming and TimelineSummary's frame build time are not consistent
5 participants