New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make NotifyIdle reject close and past deadlines. #37737
Conversation
This patch also eliminates some extraneous tracing that is happening every frame. It is possible to get the same trace calls by enabling the API stream if needed. Also refactors the NotifyIdle callsites to just always work in TimeDeltas rather than converting back and forth between them and TimePoints, which I think reads more clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm w/ some questions.
// viewport event). Because of this, we hold off on calling | ||
// |OnAnimatorNotifyIdle| for a little bit, as that could cause garbage | ||
// collection to trigger at a highly undesirable time. | ||
// Wait a tad more than 3 60hz frames before reporting a big idle period. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any logic in place that prevents having multiple outstanding NotifyIdle calls scheduled? Said differently, is there logic to ensure that there is a maximum of one call at a time scheduled to run? If that property is a side-effect of something else, would it be worth making it explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old logic said if RequestFrame
and BeginFrame
have not been called again for 51ms, fire a big idle notification. The new logic says if RequestFrame
has not been called in the last 51ms and the last known vsync target time is not in the future, fire.
We could still easily end up in a situation where NotifyIdle
is called at the end of a frame and then called again ~51ms later if there are no more frames.
The situation we are still not avoiding is that we could call NotifyIdle
at the end of a very cheap frame only to immediately start pumping an animation/route transition in the very next frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And to answer your specific question: we could schedule multiple tasks, but the guards in those tasks would prevent notify idle from being called repeatedly (i.e. more than once per frame or every 51ms when no frames are pumped).
…115672) * a76ec9158 Roll Fuchsia Linux SDK from lnmSnyJi-2H07tBnV... to WdtwlLEce90PjFJ9z... (flutter/engine#37747) * 44e2f5854 [Impeller] Change texture upload pipeline in Vulkan (flutter/engine#37623) * 6a3ad3c14 Roll Skia from 345bceacd298 to 5270b1d26b5f (4 revisions) (flutter/engine#37748) * 01271891c Do not abort if a MultiFrameCodec is unable to allocate a bitmap buffer (flutter/engine#37534) * 54232a4c3 Roll Skia from 5270b1d26b5f to cf967e6b1c00 (5 revisions) (flutter/engine#37751) * 30aa3cc38 [fuchsia][a11y] Set explicit hit regions in flatland embedder (flutter/engine#37338) * da07c33d2 Make NotifyIdle reject close and past deadlines. (flutter/engine#37737) * e3844cc1e Add third_party/dart/third_party/binaryen/src as a dependency (flutter/engine#37749) * aeb2cd95b [Impeller] use SSBOs for gradients where supported (metal/vulkan) (flutter/engine#37654) * e8aa1c192 Roll Skia from cf967e6b1c00 to f1f59de17204 (2 revisions) (flutter/engine#37756) * 4311774fb [Impeller] register modern shaders on Vulkan too (flutter/engine#37757) * 8e4a718d0 Made FlutterTextField that outlive FlutterTextPlatformNode not crash (flutter/engine#37735) * 446a09dfc [macOS] Use consistent filenames for tests (flutter/engine#37755) * 7a390f97c Roll Skia from f1f59de17204 to 12f01bc5b57e (1 revision) (flutter/engine#37760)
…lutter#115672) * a76ec9158 Roll Fuchsia Linux SDK from lnmSnyJi-2H07tBnV... to WdtwlLEce90PjFJ9z... (flutter/engine#37747) * 44e2f5854 [Impeller] Change texture upload pipeline in Vulkan (flutter/engine#37623) * 6a3ad3c14 Roll Skia from 345bceacd298 to 5270b1d26b5f (4 revisions) (flutter/engine#37748) * 01271891c Do not abort if a MultiFrameCodec is unable to allocate a bitmap buffer (flutter/engine#37534) * 54232a4c3 Roll Skia from 5270b1d26b5f to cf967e6b1c00 (5 revisions) (flutter/engine#37751) * 30aa3cc38 [fuchsia][a11y] Set explicit hit regions in flatland embedder (flutter/engine#37338) * da07c33d2 Make NotifyIdle reject close and past deadlines. (flutter/engine#37737) * e3844cc1e Add third_party/dart/third_party/binaryen/src as a dependency (flutter/engine#37749) * aeb2cd95b [Impeller] use SSBOs for gradients where supported (metal/vulkan) (flutter/engine#37654) * e8aa1c192 Roll Skia from cf967e6b1c00 to f1f59de17204 (2 revisions) (flutter/engine#37756) * 4311774fb [Impeller] register modern shaders on Vulkan too (flutter/engine#37757) * 8e4a718d0 Made FlutterTextField that outlive FlutterTextPlatformNode not crash (flutter/engine#37735) * 446a09dfc [macOS] Use consistent filenames for tests (flutter/engine#37755) * 7a390f97c Roll Skia from f1f59de17204 to 12f01bc5b57e (1 revision) (flutter/engine#37760)
…lutter#115672) * a76ec9158 Roll Fuchsia Linux SDK from lnmSnyJi-2H07tBnV... to WdtwlLEce90PjFJ9z... (flutter/engine#37747) * 44e2f5854 [Impeller] Change texture upload pipeline in Vulkan (flutter/engine#37623) * 6a3ad3c14 Roll Skia from 345bceacd298 to 5270b1d26b5f (4 revisions) (flutter/engine#37748) * 01271891c Do not abort if a MultiFrameCodec is unable to allocate a bitmap buffer (flutter/engine#37534) * 54232a4c3 Roll Skia from 5270b1d26b5f to cf967e6b1c00 (5 revisions) (flutter/engine#37751) * 30aa3cc38 [fuchsia][a11y] Set explicit hit regions in flatland embedder (flutter/engine#37338) * da07c33d2 Make NotifyIdle reject close and past deadlines. (flutter/engine#37737) * e3844cc1e Add third_party/dart/third_party/binaryen/src as a dependency (flutter/engine#37749) * aeb2cd95b [Impeller] use SSBOs for gradients where supported (metal/vulkan) (flutter/engine#37654) * e8aa1c192 Roll Skia from cf967e6b1c00 to f1f59de17204 (2 revisions) (flutter/engine#37756) * 4311774fb [Impeller] register modern shaders on Vulkan too (flutter/engine#37757) * 8e4a718d0 Made FlutterTextField that outlive FlutterTextPlatformNode not crash (flutter/engine#37735) * 446a09dfc [macOS] Use consistent filenames for tests (flutter/engine#37755) * 7a390f97c Roll Skia from f1f59de17204 to 12f01bc5b57e (1 revision) (flutter/engine#37760)
This patch also eliminates some extraneous tracing that is happening every frame. It is possible to get the same trace calls by enabling the API stream if needed.
Also refactors the NotifyIdle callsites to just always work in TimeDeltas rather than converting back and forth between them and TimePoints, which I think reads more clearly.
Also refactors the way the delayed task gets posted to hopefully be a little more safe/clear.
This patch does not "solve" the problem of when we call notify idle and then immediately start pumping frames, e.g. when we get a super cheap frame and then follow it up with a transition.