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

Conversation

farchond
Copy link
Contributor

@farchond farchond commented Feb 5, 2020

To give more flexibiltiy in scheduling, we change the number of frames
in flight we can have at one time to 3. We also introduce an offset from
VSync that Flutter can use to begin its work at. It is currently set at
0ms, signalling no change from previous behavior.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

I am not sure about the context behind updating kMaxFramesInFlight but the other change looks fine (barring the minor nit).

fml::TimePoint next_vsync = SnapToNextPhase(now, last_presentation_time,
vsync_info.presentation_interval);

auto vsync_start_time = next_vsync - vsync_offset;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: next_vsync_start_time might make more sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// outstanding at any time. This is equivalent to how many times it has
// called Present2() before receiving an OnFramePresented() event.
static constexpr int kMaxFramesInFlight = 2;
static constexpr int kMaxFramesInFlight = 3;
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about the nature of scheduling flexibility offered by this change. But, in the past, we have found that increasing the pipeline depth has adverse effects when it comes to render frames with inconsistent rasterizer times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's important to note that this doesn't actually impact the Flutter pipeline as you normally think of it, but rather how Flutter Present()s content to Scenic.

Right now Flutter only has two frames in flight, which means that if we are effectively "double buffered." If we then miss frame A Flutter can't submit frame C, because B is also in flight. By changing this to 3, we can miss frame A and still have frames B and C in flight too, ensuring that one frame miss won't result in another one.

If we were targeting <=16ms Scenic+Flutter frame build times then this wouldn't matter. But by increasing how many frames in flight we can have we can target 32ms frame build times and not suffer a double penalty for missing a frame.

Copy link
Member

Choose a reason for hiding this comment

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

It's important to note that this doesn't actually impact the Flutter pipeline as you normally think of it, but rather how Flutter Present()s content to Scenic.

I realize that. Just wanted to know the reasoning behind this change. Thanks for the clarifications.

To give more flexibiltiy in scheduling, we change the number of frames
in flight we can have at one time to 3. We also introduce an offset from
VSync that Flutter can use to begin its work at. It is currently set at
0ms, signalling no change from previous behavior.
@farchond farchond merged commit f25d325 into flutter:master Feb 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020
dnfield pushed a commit to flutter/flutter that referenced this pull request Feb 7, 2020
* d5442b8 Roll src/third_party/skia bc3307c395e2..ebc00f97fab1 (21 commits) (flutter/engine#16429)

* 76b291a Added a plugin method that gets called when the engine is about to be deleted (flutter/engine#16336)

* 07f25c5 fix bad reference to maxDiffRatePercent (flutter/engine#16440)

* 41d50c2 Reland #16206: "[web] Correct getPositionForOffset for multi-line paragraphs" (flutter/engine#16365)

* f25d325 [fuchsia] change kMaxFramesInFlight to 3 (flutter/engine#16425)

* 473f559 Suppress some deprecation warnings on Windows (flutter/engine#16416)

* 2e34ad6 Roll fuchsia/sdk/core/mac-amd64 from ubThi... to fvWgE... (flutter/engine#16454)

* 47c02e6 Roll src/third_party/skia ebc00f97fab1..cbf79b95c2d4 (4 commits) (flutter/engine#16456)

* 3d1b112 Roll buildroot (flutter/engine#16419)

* 28e6637 Add explicit casts to printing of function pointers (flutter/engine#16370)

* 9ad81da Wrap strdup to use compliant name on Windows (flutter/engine#16372)

* 9708e52 Roll rapidjson (flutter/engine#16347)

* f06ebba Include <memory> in hb_wrapper.h because unique_ptr is used. (flutter/engine#16442)

* e530376 Roll fuchsia/sdk/core/linux-amd64 from VJv0H... to A9STP... (flutter/engine#16457)

* 4cc41ae Roll src/third_party/skia cbf79b95c2d4..4721e067812f (1 commits) (flutter/engine#16459)

* 2f233ed Roll src/third_party/skia 4721e067812f..f6e3eaf05150 (1 commits) (flutter/engine#16461)

* b0b0ed8 Roll src/third_party/skia f6e3eaf05150..cc21d0c1f2ce (1 commits) (flutter/engine#16463)

* 7fea936 Roll src/third_party/skia cc21d0c1f2ce..116b33e8ab21 (3 commits) (flutter/engine#16466)

* 001b3a0 Roll src/third_party/skia 116b33e8ab21..7f36405ea3ec (3 commits) (flutter/engine#16471)

* f3ce90e Reset width/height before deallocation for Safari allocation bug. (flutter/engine#16469)
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
To give more flexibility in scheduling, we change the number of frames
in flight we can have at one time to 3. We also introduce an offset from
VSync that Flutter can use to begin its work at. It is currently set at
0ms, matching previous behavior.
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants