Fix VsyncWaiter #25551

farchond · 2021-04-12T20:39:14Z

This PR rewrites the VsyncWaiter for Fuchsia class from scratch. This change is ready for review, modulo some additional unittests I'm working on involving creating a FakeSessionConnection.

Implementation Details

A VsyncWaiter's only purposes are to wait for vsyncs, and call FireCallback() when appropriate to let the Animator know when they can produce a new frame, or do some trace bookkeeping. This PR changes how this is done on Fuchsia in some key ways, listed below.

SessionConnection integration

VsyncWaiter now has a reference to SessionConnection. SessionConnection is responsible for talking to Scenic, and SessionConnection therefore knows when vsyncs are reported back. This means that VsyncWaiter no longer needs to post tasks and guess when vsyncs happen, but rather hear back from SessionConnection directly. There are some caveats with this approach however, as there can be occasional delays in getting this signal due to the amount of thread hops that need to happen. For this reason, VsyncWaiter still schedules based on SnapToNextPhase, but receiving the vsync tick means it doesn't post tasks for future work, which significantly reduces code complexity.

The integration of SessionConnection in the VsyncWaiter also allows for backpressure from Scenic to translate up into Flutter framework.

Use of AwaitVSyncForSecondaryCallback() (#25787)

There are two flavors of waiting for vsync - one is intended for frame scheduling, and the other for miscellaneous tasks. The problem I noticed while testing is that every input event would call AwaitVsync() for trace related purposes, but VsyncWaiter() would interpret this as a frame request, which wasn't often the case. This would lead to an implementation that either lacked robustness by not implementing backpressure, or one that was too conservative in its scheduling.

By separating the two intentions, it is possible to easily perform better for both cases. AwaitVSyncForSecondaryCallback() requests do not update the last_targetted_time_, meaning that when real frame requests come in they are not delayed by the occurrence of prior input events.

Use of `GetTargetTimes()`

Similar to SnapToNextPhase() in VsyncWaiter, and CalculateNextLatchPoint() in SessionConnection, it is useful to extract all scheduling logic into an easy-to-test static function. Whereas the previous implementation had its scheduling logic scattered around the class, and via PostTask timing, now the decision of what arguments to pass into FireCallback() is easier to read and maintain. This makes it easy to solve and unittest for the performance problems I noticed, more on that below.

Motivations

Performance

This implementation solves two main problems, as well as enabling input resampling on Fuchsia.

The first problem I noticed was that we would previously often schedule N+1 frames in an N vsync interval window. This guarantees wasted effort, as one of those frames will be produced only to never be shown. This also meant that the frames that would be displayed would be produced later than necessary, due to the wasted effort.

The second issue is that even when the frames would be scheduled in a reasonable time frame, sometimes we'd have a "galloping" behavior where 2 frames would be scheduled in one vsync interval, then 0 in the following interval, then 2 in the one after, etc. This did mean N frames produced in N vsync intervals given optimal scheduling, yes, but the cadence of the frames was uneven. This hampered our ability to resample input, which led to it being disabled on Fuchsia.

I ran perfcompare tests on common workflows on Fuchsia, and saw a statistically significant increase in FPS, GPU utilization, and other key benchmarks.

Smooth input

This change will enable input resampling on Fuchsia again, as it had to be disabled to do inconsistencies in scheduling.

Testability

This implementation is much easier to test, and it has performance unittests as a result. This PR extracts all the logic of when to schedule next (aka call FireCallback()) into a single static function, GetTargetTimes(), which takes in as input all the relevant variables for when to schedule the next available frame.

This change also adds a FakeSessionConnection, which allows us to better test the VsyncWaiter without needing a real Scenic or display. Specifically, we can test backpressure limits as well as high CPU load by delaying OnVsync()s.

Robustness

This implementation takes into account Scenic backpressure, which means that it is no longer possible for Flutter to produce more GPU work than Scenic and the system can handle. This is accomplished by having the VsyncWaiter take a pointer to the SessionConnection, which knows about how many frames in flight Flutter is allowed to have.

chinmaygarde · 2021-04-15T21:52:59Z

Can you describe the patch and what its trying to do?

chinmaygarde · 2021-04-22T20:15:54Z

Ping. Is this WIP or a draft? If progress on this is not likely, lets close it for now.

farchond · 2021-04-22T20:44:15Z

Ping. Is this WIP or a draft? If progress on this is not likely, lets close it for now.

Hi Chinmay. I've been OOO recovering from a medical operation, but this PR and the associated documentation is something I'm actively working on now.

chinmaygarde · 2021-04-22T20:59:53Z

I've been OOO recovering.

Oh, take you time. No rush. Hope you feel better soon.

arbreng · 2021-04-28T19:17:19Z

shell/platform/fuchsia/flutter/platform_view.cc

    std::shared_ptr<flutter::ExternalViewEmbedder> external_view_embedder,
    fml::TimeDelta vsync_offset,
-    zx_handle_t vsync_event_handle)
+    SessionConnection* session_connection)


Lets be good about tracking lifetimes and make this a shared_ptr

Yeah, I should've marked this as TODO.

I left it as a raw pointer until I have a FakeSessionConnection class available to use for platform_view_unittests.

arbreng · 2021-04-28T19:17:39Z

shell/platform/fuchsia/flutter/platform_view.h

  std::set<std::string /* channel */> unregistered_channels_;

  fml::TimeDelta vsync_offset_;
-  zx_handle_t vsync_event_handle_ = 0;


Hallelujah!

arbreng · 2021-04-28T19:20:17Z

shell/platform/fuchsia/flutter/vsync_waiter.h

-
-  VsyncWaiter(std::string debug_label,
-              zx_handle_t session_present_handle,
+  VsyncWaiter(SessionConnection* session_connection,


Nit: please move constructor below static methods (google style guide dictates this)

arbreng · 2021-04-28T19:21:28Z

shell/platform/fuchsia/flutter/vsync_waiter.cc

+}
+
+VsyncWaiter::~VsyncWaiter() {
+  FML_LOG(WARNING) << "~VsyncWaiter()";


Nit: remove

arbreng · 2021-04-28T19:44:05Z

shell/platform/fuchsia/flutter/vsync_waiter.h

+
+  FrameTargetTimes GetTargetTimesHelper(bool secondary_callback);
+
+  SessionConnection* session_connection_;


Use shared_ptr please (not a nit)

arbreng · 2021-04-28T19:45:06Z

shell/platform/fuchsia/flutter/engine.cc

       max_frames_in_flight = product_config.get_max_frames_in_flight(),
-       vsync_handle = vsync_event_.get(), &view_embedder_latch]() mutable {
+       &view_embedder_latch]() mutable {
        session_connection_.emplace(


At this point, this should be a shared_ptr too

arbreng · 2021-04-28T19:51:44Z

shell/platform/fuchsia/flutter/vsync_waiter.cc

+  // Initialize the callback that will run every time the SessionConnection
+  // receives one or more frame presented events on Vsync.
+  session_connection_->InitializeVsyncWaiterCallback([this]() {
+    task_runners_.GetUITaskRunner()->PostTask([&weak_factory_ui =


It is not neccesary to rethread this callback to the UI thread.

This callback bottoms out in VsyncWaiter::FireCallback. If you read the implementation of it, it protects variables w/ a mutex, and also performs the necesary re-threading internally.

So, 3 things:

Can you update the common VsyncWaiter documentation to indicate that FireCallback may be called from any thread safely?

Session callbacks happen on the raster thread. So change "weak_factory_ui" to "weak_factory_raster" + initialize and destroy it on the raster thread instead of UI. I plan to move the scenic::Session to the platform thread soon (the thread that weak_factory_ is on), and then the 2nd weak factory will vanish entirely.

Since all of this code now runs on the raster thread, and the presentation callbacks + Present also run on the raster thread, VsyncRecorder has no more reason to exist. Everything happens on one thread. See my other comment on vsync_recorder.h

As we discussed, this is not currently possible due to decreased performance. I filed flutter/flutter#81984 to track this issue.

arbreng · 2021-04-28T19:54:55Z

shell/platform/fuchsia/flutter/vsync_recorder.h

  // information will be saved (in order to handle edge cases involving
-  // multiple Scenic sessions in the same process).  This function is safe to
+  // multiple Scenic sessions in the same process). This function is safe to
  // call from any thread.


If you follow my other suggestion below, then this class no longer has a reason to exist. Everything happens on one thread now, and mostly inside of the SessionConnection.

At that point, you can just collapse this class into SessionConnection without violating any sort of encapsulation.

This also is not possible now unfortunately, so I filed flutter/flutter#81984

mikaelpessa

LGTM around the scheduling logic.

mikaelpessa · 2021-04-28T20:20:12Z

shell/platform/fuchsia/flutter/vsync_waiter.h

+// (frame_start_time, frame_target_time)
+using FrameTargetTimes = std::pair<fml::TimePoint, fml::TimePoint>;
+// (latch point time, vsync time)
+using LatchPointVsyncPair = std::pair<fml::TimePoint, fml::TimePoint>;


nit: making these structs would be more readable and make the comments unnecessary.

In preparation for flutter#25551, rename SessionConnection to DefaultSessionConnection. The plan is to have SessionConnection be an interface we can create a fake of for testing.

In preparation for #25551, rename SessionConnection to DefaultSessionConnection. The plan is to have SessionConnection be an interface we can create a fake of for testing purposes.

nathanrogersgoogle

+1

nathanrogersgoogle · 2021-05-14T18:26:30Z

shell/platform/fuchsia/flutter/vsync_waiter.h

+  // FireCallback(). This value should be strictly increasing in order to
+  // guarantee that animation code that relies on target vsyncs works correctly,
+  // and that Flutter is not producing multiple frames in a small interval.
+  fml::TimePoint last_targetted_vsync_;


Nit, but FWIW I think "targeted" is a more common spelling vs "targetted".

unittests are deactivated

remove session_connection.h remove vsync_waitr_unittests

arbreng · 2021-05-20T01:42:56Z

shell/platform/fuchsia/flutter/default_session_connection.h

+
+  // VsyncRecorder logic.
+
+  VsyncInfo GetCurrentVsyncInfo() const;


Nit: Move this above all of the fields, with PresentSession and friends

arbreng · 2021-05-20T01:43:28Z

shell/platform/fuchsia/flutter/default_session_connection.h

+  VsyncInfo GetCurrentVsyncInfo() const;

-  static void ToggleSignal(zx_handle_t handle, bool raise);
+  fuchsia::scenic::scheduling::PresentationInfo next_presentation_info_;


Are these also guarded by the mutex? Its not clear because the GetCurrentVsyncInfo breaks things up

flutter-dashboard bot added the platform-fuchsia label Apr 12, 2021

google-cla bot added the cla: yes label Apr 12, 2021

farchond mentioned this pull request Apr 27, 2021

[vsync_waiter] add AwaitVSyncForSecondaryCallback() #25787

Merged

farchond force-pushed the vsync_waiter branch from 86e992c to 857ae54 Compare April 28, 2021 00:14

farchond requested review from arbreng, chinmaygarde and nathanrogersgoogle April 28, 2021 18:26

arbreng reviewed Apr 28, 2021

View reviewed changes

mikaelpessa reviewed Apr 28, 2021

View reviewed changes

farchond force-pushed the vsync_waiter branch from 084da3d to 0e84d44 Compare May 11, 2021 19:38

farchond mentioned this pull request May 12, 2021

[fuchsia] rename SessionConnection to DefaultSessionConnection #26104

Merged

farchond force-pushed the vsync_waiter branch from 0e84d44 to ff25f35 Compare May 12, 2021 19:47

farchond force-pushed the vsync_waiter branch from a52fb20 to 5aeaba7 Compare May 13, 2021 20:03

nathanrogersgoogle approved these changes May 14, 2021

View reviewed changes

farchond force-pushed the vsync_waiter branch from 811780e to a293b06 Compare May 14, 2021 19:43

farchond added 11 commits May 18, 2021 14:09

re-implement VsyncWaiter - all tests and rebased

e02e209

moved to pure callback only mode

21b89ee

unittests are deactivated

add traces and clean up dead code

a4e0f5e

add mutex and trace

7c67c73

made locking easier to prove correct

401a086

fix BUILD.gn tests

7fe1efd

remove session_connection.h remove vsync_waitr_unittests

fix BUILD.gn formatting and DSC method order

7eef759

cleanup licenses, animator, vsync_waiter

32c2096

delete vsync_waiter_unittests

682b744

add defaultsessionconnection tests

d3f740e

remove vsync_recorder

99166f2

farchond force-pushed the vsync_waiter branch from a293b06 to 99166f2 Compare May 19, 2021 20:25

farchond added 4 commits May 19, 2021 16:33

fix vsync_recorder licenses

9a33228

finish vsync_recorder tests

2d9d00c

remove old vsync_recorder_unittests

08bf0fb

s/targetted/targeted

05c50d8

arbreng reviewed May 20, 2021

View reviewed changes

arbreng approved these changes May 20, 2021

View reviewed changes

clean up session_connection.h

fadb7dc

farchond merged commit b60dcf3 into flutter:master May 20, 2021

engine-flutter-autoroll mentioned this pull request May 20, 2021

Roll Engine from c4bd450b628e to 95b6fbeb6be1 (3 revisions) flutter/flutter#83060

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 20, 2021

b60dcf3 Fix VsyncWaiter (flutter/engine#25551)

9ece6c5

chinmaygarde mentioned this pull request Jun 28, 2021

[fuchsia] move VsyncWaiter out of the UI thread; delete VsyncRecorder flutter/flutter#81984

Closed


		FrameTargetTimes GetTargetTimesHelper(bool secondary_callback);

		SessionConnection* session_connection_;


		// VsyncRecorder logic.

		VsyncInfo GetCurrentVsyncInfo() const;

Fix VsyncWaiter #25551

Fix VsyncWaiter #25551

Uh oh!

Conversation

farchond commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation Details

SessionConnection integration

Use of AwaitVSyncForSecondaryCallback() (#25787)

Use of GetTargetTimes()

Motivations

Performance

Smooth input

Testability

Robustness

Uh oh!

chinmaygarde commented Apr 15, 2021

Uh oh!

chinmaygarde commented Apr 22, 2021

Uh oh!

farchond commented Apr 22, 2021

Uh oh!

chinmaygarde commented Apr 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelpessa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanrogersgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

farchond commented Apr 12, 2021 •

edited

Loading

Use of `GetTargetTimes()`