Do not call Animator::Delegate::OnAnimatorNotifyIdle until at least one frame has been rendered. #29015
Conversation
dnfield
requested review from
chinmaygarde,
iskakaushik,
zanderso and
jason-simmons
|
The new test is segfaulting. |
|
Ah. I only ran it locally with debug unoptimized. I'll check profile out. |
|
Issue was that the animator was sometimes getting deleted before the vsync waiter was trying to check a callback the animator ends up deleting in its dtor. The engine seems to handle this correctly. I refactored the test to more carefully control vsync pulses, found a flake where I was sometimes losing a race to the delayed task for notify idle in BeginFrame, and made sure to stop/flush the animator before deleting it. |
|
This is now ready for review. |
|
And big thanks to @bdero for getting asan builds working for this - without that it would've been really hard to figure out what's going on here. |
|
Do we have a benchmark that runs on our CI that will show a benefit from this change? (And which more importantly will regress if the effects of this change are broken). |
|
I don't think so. Looking at the traces for flutter_gallery__start_up, it doesn't typically run into this problem because it tends to very quickly get to We could add a test to our CI that does something like try to talk to some plugins before runApp, or just does an |
It creates a bit of disruption and makes customers unhappy when we don't discover regressions until downstream post-submit benchmarks run. If there's anything we can do to prevent that situation, then I think it's worth the trouble. |
|
Performance test has landed downstream. Anyone up for reviewing this? :) |
| @@ -147,7 +147,7 @@ void Animator::BeginFrame( | |||
| delegate_.OnAnimatorBeginFrame(frame_target_time, frame_number); | |||
| } | |||
|
|
|||
| if (!frame_scheduled_) { | |||
| if (!frame_scheduled_ && has_rendered_) { | |||
There was a problem hiding this comment.
Do you know what the magic number 100000 down on line 172 is for? Sorry it isn't part of your PR, but if you're in the neighborhood and you know what it is and why it has that value, could you make a constant for it and document it?
There was a problem hiding this comment.
AFAIK this was done by some Fuchsia folks to try to avoid kicking off a GC when we're about to animate. I'm not sure where it came from though, and it's not well tested.
There was a problem hiding this comment.
maybe @nathanrogersgoogle
There was a problem hiding this comment.
There was a problem hiding this comment.
I suppose it's to make up for the fact that we just delayed 51ms but I'm really not sure. I'm going to land this as-is, and if Nathan knows what's up maybe we can add more docs there.
There was a problem hiding this comment.
The magic number "100ms" was meant to be enough time to run the compactor but not so much time that things wouldn't be awful if we've incorrectly detected idle time. 100ms probably isn't enough time for slower devices or larger heaps. This logic should probably be moved to the engine's task runners / scheduler to properly detect idle time, and invoke Dart_NotifyIdle with a much further deadline.
… least one frame has been rendered. (flutter/engine#29015)
… least one frame has been rendered. (flutter/engine#29015) (#91431)
… least one frame has been rendered. (flutter/engine#29015) (flutter#91431)
Do not call Animator::Delegate::OnAnimatorNotifyIdle until at least one frame has been rendered. (flutter#29015)
Do not call Animator::Delegate::OnAnimatorNotifyIdle until at least one frame has been rendered. (flutter#29015) flutter/flutter#91209
Otherwise, the animator may get started by a number of events (e.g. getting an onscreen surface created) and notify Dart that we're idle when we are in fact working dutifully to get to the first frame.
Fixes flutter/flutter#91209
This significantly improves customer: money's start up time by avoiding GC work before the first frame. Per @zanderso , we may also want some way to tell dart to size the heap appropriately to avoid unnecessary work to grow it early in the application as well.