Allow render to be called multiple times for one BeginFrame

[fzyzcjy]

1. I will finish code details, refine code, add tests, make tests pass, etc, after a code review that thinks the rough idea is acceptable. It is because, from my past experience, reviews may request changing a lot. If the general idea is to be changed, all detailed implementation efforts are wasted :)
2. The PR has an already-working counterpart, and it produces ~60FPS smooth experimental results. The benchmark results and detailed analysis is in chapter https://cjycode.com/flutter_smooth/benchmark/. All the source code is in https://github.com/fzyzcjy/engine/tree/flutter-smooth and https://github.com/fzyzcjy/flutter/tree/flutter-smooth.
3. Possibly useful as a context to this PR, there is a whole chapter discussing the internals - how flutter_smooth is implemented. (Link: https://cjycode.com/flutter_smooth/design/)

---

This PR is a part for implementing the 60fps smooth rendering (#101227).

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#101227

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

The only change is an "if" as follows (all else are just tests)

Pre-launch Checklist

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

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

By the way, the tests and code does work: If I comment out the code, the tests fail.

[fzyzcjy]

Not sure whether I should "@" some people here, maybe @dnfield @jonahwilliams @gaaclarke @flar engine experts?

May I get a code review, thanks :)

[dnfield]

The documentation on FlutterView.render specifies when it is safe/allowed to call render.

I'm not quite clear on how this will affect the pipeline - it seems like it will now be trivial for a dart:ui application to flood the pipeline if we remove guardrails around when you can call render. Today the contract is that the application can expect that it's time to call render because it got a call to onBeginFrame. In this world, the application calls render whenever it thinks it has been working too long and might want to give an update. But the application doesn't know about vsync and it will be very hard to reason about why render is getting called if we make this change.

I don't think we should make this change. It too easily allows wasted work to happen.

[fzyzcjy]

@dnfield Hi, thanks for the quick reply :)

The documentation on FlutterView.render specifies when it is safe/allowed to call render.

I will change that doc accordingly (probably after we come to a conclusion what should be done for this PR)

I'm not quite clear on how this will affect the pipeline - it seems like it will now be trivial for a dart:ui application to flood the pipeline if we remove guardrails around when you can call render. Today the contract is that the application can expect that it's time to call render because it got a call to onBeginFrame. In this world, the application calls render whenever it thinks it has been working too long and might want to give an update. But the application doesn't know about vsync and it will be very hard to reason about why render is getting called if we make this change.
I don't think we should make this change. It too easily allows wasted work to happen.

Firstly, IMHO, a normal flutter app calls window.render once per frame, so no problem at all for all existing app.

Secondly, in my proposal (Preempt for 60 FPS), I do observe vsync (using VsyncAwaiter class), and only submit one window.render per vsync. Therefore, "But the application doesn't know about vsync" seems not to be the situation, and thus "it will be very hard to reason about why render is getting called" is also no problem.

That said, I do agree that, if the rasterizer thread takes too long (e.g. takes 50ms for one rasterize), it is a waste to submit a Scene per 16.6ms (but should submit per 50ms).

If this is still a problem for you, can I change as follows: Add a flag to window.render, say, window.render({bool onlyRenderOncePerBeginFrame = true}). Then the behavior will be exactly the same, except for someone who really needs this (e.g. the Preempt proposal).

In addition, given it is a so low-level API that most people will never touch, it seems reasonable to provide some flexibility to it.

[fzyzcjy]

@dnfield Another possibility for Dart code to understand the queue is full so it do not do anything more:

Add this 4-line function:

// return: whether it is prepared successfully. If return false, it means pipeline is full, 
// and thus the user should not really compute the Scene to avoid unnecessary work.
bool Animator::PrepareExtraRender() {
  if (!producer_continuation_) {
    producer_continuation_ = layer_tree_pipeline_->Produce();
  }
  return static_cast<bool>(producer_continuation_);
}

usage:

realize_next_vsync_comes; // see the design for details https://docs.google.com/document/d/1FuNcBvAPghUyjeqQCOYxSt6lGDAQ1YxsNlOvrUx0Gko/edit
var prepared = window.prepareExtraRender();
if (prepared) {
  ui.Scene scene = compute_the_scene();
  window.render(scene);
} else {
  // do not do anything since the rasterizer queue is already so full
  // this mimic the behavior of Animator::BeginFrame, where we skip the current frame if it is full
}

[fzyzcjy]

@dnfield ... be trivial for a dart:ui application to flood the pipeline ...

IMHO the pipeline seems not to be flooded - it has depth 2. In other words, even if we call window.render(scene) a million times within a frame, only the first two scenes will be in the queue, and the rest 999998 will just be thrown away (suppose rasterizer has not processed any). So we are still safe.

For a dart:ui application, if needed, it can use the window.prepareExtraRender extra call to see whether the queue is already full, to avoid generating scene etc (just like example above).

Today the contract is that the application can expect that it's time to call render because it got a call to onBeginFrame. In this world, the application calls render whenever it thinks it has been working too long and might want to give an update.

Just as mentioned above, adding a flag like window.render({bool onlyRenderOncePerBeginFrame = true}) seems to solve the "contract" problem.

[fzyzcjy]

Oops sorry @chinmaygarde and @iskakaushik I just clicked the "Icons.refresh" on dnfield and do not know why github remove review requests to you...

[fzyzcjy]

If we are worried that users may submit multiple window.render inside one vsync, another possibility: We may add some code in the C++ layer (or Dart layer), such that we check current vsync status, and only Produce() if it is a new vsync that has not been produced before.

[dnfield]

Fizzling like that would be expensive, and we'd be giving developers a button to push that actually makes things slower. We should avoid that.

[fzyzcjy]

@dnfield If speed is a concern, maybe the original PR is ok: For a normal usage, it only adds if (!producer_continuation_) (and that if will return false immediately). Given that explicit operator bool() const { return continuation_ != nullptr; }, this if will only check whether a pointer is nullptr, so I guess it is only a few CPU cycles (per 16667 microseconds, i.e. maybe 10000000 cycles). In addition, for a normal usage, the branch will always be false, so IMHO the cpu branch predictor will be quite correct about the prediction.

[fzyzcjy]

@dnfield giving developers a button to push that actually makes things slower

There seems to be another way that is not very slower:

Firstly, the onlyRenderOncePerBeginFrame should not be window.render(onlyRenderOncePerBeginFrame: true), but be window.onlyRenderOncePerBeginFrame = true; window.render(). In other words, it should be a flag that is set once. Then the code is:

...
  if (!onlyRenderOncePerBeginFrame && !producer_continuation_) {
    producer_continuation_ = layer_tree_pipeline_->Produce();
  }
...

void SetOnlyRenderOncePerBeginFrame(bool value) { this->onlyRenderOncePerBeginFrame = value; }
class Animator { ... bool onlyRenderOncePerBeginFrame; ... }

(no need for locks, since all on UI thread.)

Then there comes the concern that if (!onlyRenderOncePerBeginFrame && !producer_continuation_) can cost CPU cycles, even when onlyRenderOncePerBeginFrame is always true (for a classical user). Firstly, for a classical user, we only pay extra cost of if(boolean) (because && is short-circuited), so only a few cycles.

Secondly, seems we can use the [[likely]] (c++20), or LIKELY (a lot of c++ library write their own version for that, e.g. see [how linux])(https://stackoverflow.com/questions/109710/how-do-the-likely-unlikely-macros-in-the-linux-kernel-work-and-what-is-their-ben) does that), to further hint compiler about this case to speed up.

[fzyzcjy]

@dnfield And for zero speed decrease if you like:

The Animator::PrepareExtraRender proposal seems to cause zero speed loss for a classical user. Because a classical user never calls that function, and only call Animator::Render. But Animator::Render is not modified in this proposal. For a smooth user, there does exist overhead, because need to call one extra C++ function - the PrepareExtraRender.

[dnfield]

You're discounting the time it takes to actually make a native call from Dart.

On top of that, we should not expose an API that might or might not do something and developers have no good way to reason about whether they're really supposed to call it or not.

This proposal is fundamentally changing the invariants around render/onBeginFrame, but it's not providing any way for developers to know if they're using the new invariants correctly or not. Even if the new potentially useless API is relatively cheap, it adds up when developers (and packages they use) start doing it multiple times per frame. And those developers/packages will have no way to know whether they're doing it correctly or not, so it will definitely get misused.

Why, for example, shouldn't the framework just call render and schedule a new frame when its hit its potential limit?

[fzyzcjy]

@dnfield Hi thanks for the reply.

You're discounting the time it takes to actually make a native call from Dart.

Originally I thought that is small just like a normal function call... Ok now I learn it.

On top of that, we should not expose an API that might or might not do something and developers have no good way to reason about whether they're really supposed to call it or not.

Indeed they have a way to reason: look at time or vsync info. They should not submit twice inside one vsync interval.

This proposal is fundamentally changing the invariants around render/onBeginFrame, but it's not providing any way for developers to know if they're using the new invariants correctly or not.

I am not sure, if I expose the vsync info and ensure only one call is made per vsync interval (16.67ms), does this satisfy your requirement?

Even if the new potentially useless API is relatively cheap, it adds up when developers (and packages they use) start doing it multiple times per frame.

Again, as mentioned above, dev should not call it multiple times per frame. A naive dev may use DateTime.now() - last_vsync_time > 15ms etc to check, and a more sophisticated way may be read the vsync info (exposed from engine) to really ensure we never call twice per vsync interval.

And those developers/packages will have no way to know whether they're doing it correctly or not, so it will definitely get misused.

Then maybe we should return bool to indicate whether it is really scheduled. If they see a lot of false they are doing it wrong (call too many times that are useless).

Why, for example, shouldn't the framework just call render and schedule a new frame when its hit its potential limit?

Because of the fundamental design of the preempt proposal (https://docs.google.com/document/d/1FuNcBvAPghUyjeqQCOYxSt6lGDAQ1YxsNlOvrUx0Gko/edit), mainly "The flow chart" section.

Indeed, window.render is called per vsync interval (16.67ms). The main difference from classical code is that, it may be called multiple times per onBeginFrame (when onBeginFrame is super slow).

[dnfield]

Devices do not always have 60fps vsync - sometimes it's 90 or 120 or more or less (at one point we had a customer looking at 240hz devices, and it's likely there are customers out there looking at 30hz use cases). There is no way currently in dart:ui to know what the current refresh rate is, and on some platforms it's not even possible to implement because the vendors don't provide an API for it (e.g. some Android vendors), and it can change from frame to frame.

In other words, a developer must not assume that 16.67ms is the right interval for a frame in all circumstances. And the query of DateTime.now is almost certain to not match the actual vsync start time, so if you assume you have roughly 16ms from onBeginFrame you might actually overshoot vsync.

[fzyzcjy]

Devices do not always have 60fps vsync - sometimes it's 90 or 120 or more or less (at one point we had a customer looking at 240hz devices, and it's likely there are customers out there looking at 30hz use cases). There is no way currently in dart:ui to know what the current refresh rate is, and on some platforms it's not even possible to implement because the vendors don't provide an API for it (e.g. some Android vendors), and it can change from frame to frame.

Definitely! That's why I also propose to expose vsync-related information to the dev. Last week you asked me to describe it and it was at "Get last vsync time information" section of google doc.

And the query of DateTime.now is almost certain to not match the actual vsync start time, so if you assume you have roughly 16ms from onBeginFrame you might actually overshoot vsync.

Totally agree. Indeed in my (previous) implementation, I let the C++ side expose the timeStamp (i.e. vsync target time we provide to dart tonBeginFrame) both a "Duration" and a "DateTime-compatible time". If you like I can add that back (deleted it b/c want to make PR small).

[fzyzcjy]

P.S. I am also considering relaxing when to start a onBeginFrame which seems to reduce unnecessary idle time and improve performance. That may be related to the big picture you are concerned - how vsync and code are interacted. I will add it to design doc and reply here maybe in an hour.

[fzyzcjy]

@dnfield Here it goes: "Relax onBeginFrame starting criterion" section in https://docs.google.com/document/d/1FuNcBvAPghUyjeqQCOYxSt6lGDAQ1YxsNlOvrUx0Gko/edit

[dnfield]

I think it would be easier to start with a patch that exposes more about vsync timings to the developer, because that will be critical to whether this one makes sense.

[fzyzcjy]

Thanks, I will do that.

[fzyzcjy]

@dnfield Hi, PR is here: #36607

Only the API is there currently, because IMHO the implementation details is unrelated to thoughts about this issue, and the API (and therefore implementations) are subject to changes.

[dnfield]

Let's close this one out to get it off review queues until we have more consensus around the approach.

[fzyzcjy]

All right...