From dfc7b21adefbe387887d1cca2a5c8d03d07b3e9d Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 19 Oct 2023 14:31:14 -0700 Subject: [PATCH] Reland 2 (part 2): Enforce the rule of calling `FlutterView.Render` (flutter/engine#47095) This PR relands part of https://github.com/flutter/engine/pull/45300, which was reverted in https://github.com/flutter/engine/pull/46919 due to performance regression. Due to how little and trivial production code the original PR touches, I really couldn't figure out the exact line that caused it except through experimentation, which requires changes to be officially landed on the main branch. After this PR lands, I'll immediately fire a performance test. This PR contains the render rule check performed by `PlatformDispatcher` of the original PR, the remaining changes to production code besides [the part 1](https://github.com/flutter/engine/pull/47062). Since part 1 shows no regression, the changes of this PR is highly likely to be the culprit. Therefore I made some changes: The rule enforcement is no longer performed in release mode, but only in debug mode. This will cause behavior deviation between builds, but since the developer should be able to notice violation in debug mode anyway, I think this design is acceptable. It is intentional to not contain any unit tests or other changes of the original PR. They will be landed shortly after this PR. Part of https://github.com/flutter/flutter/issues/136826. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../flutter/lib/ui/platform_dispatcher.dart | 52 +++++++++++++++++++ engine/src/flutter/lib/ui/window.dart | 13 ++++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/engine/src/flutter/lib/ui/platform_dispatcher.dart b/engine/src/flutter/lib/ui/platform_dispatcher.dart index 0bd387270ba..66e68b88be5 100644 --- a/engine/src/flutter/lib/ui/platform_dispatcher.dart +++ b/engine/src/flutter/lib/ui/platform_dispatcher.dart @@ -308,6 +308,28 @@ class PlatformDispatcher { _invoke(onMetricsChanged, _onMetricsChangedZone); } + // A debug-only variable that stores the [FlutterView]s for which + // [FlutterView.render] has already been called during the current + // [onBeginFrame]/[onDrawFrame] callback sequence. + // + // It is null outside the scope of those callbacks indicating that calls to + // [FlutterView.render] must be ignored. Furthermore, if a given [FlutterView] + // is already present in this set when its [FlutterView.render] is called + // again, that call must be ignored as a duplicate. + // + // Between [onBeginFrame] and [onDrawFrame] the properties value is + // temporarily stored in `_renderedViewsBetweenCallbacks` so that it survives + // the gap between the two callbacks. + // + // In release build, this variable is null, and therefore the calling rule is + // not enforced. This is because the check might hurt cold startup delay; + // see https://github.com/flutter/engine/pull/46919. + Set? _debugRenderedViews; + // A debug-only variable that temporarily stores the `_renderedViews` value + // between `_beginFrame` and `_drawFrame`. + // + // In release build, this variable is null. + Set? _debugRenderedViewsBetweenCallbacks; /// A callback invoked when any view begins a frame. /// @@ -329,11 +351,26 @@ class PlatformDispatcher { // Called from the engine, via hooks.dart void _beginFrame(int microseconds) { + assert(_debugRenderedViews == null); + assert(_debugRenderedViewsBetweenCallbacks == null); + assert(() { + _debugRenderedViews = {}; + return true; + }()); + _invoke1( onBeginFrame, _onBeginFrameZone, Duration(microseconds: microseconds), ); + + assert(_debugRenderedViews != null); + assert(_debugRenderedViewsBetweenCallbacks == null); + assert(() { + _debugRenderedViewsBetweenCallbacks = _debugRenderedViews; + _debugRenderedViews = null; + return true; + }()); } /// A callback that is invoked for each frame after [onBeginFrame] has @@ -351,7 +388,22 @@ class PlatformDispatcher { // Called from the engine, via hooks.dart void _drawFrame() { + assert(_debugRenderedViews == null); + assert(_debugRenderedViewsBetweenCallbacks != null); + assert(() { + _debugRenderedViews = _debugRenderedViewsBetweenCallbacks; + _debugRenderedViewsBetweenCallbacks = null; + return true; + }()); + _invoke(onDrawFrame, _onDrawFrameZone); + + assert(_debugRenderedViews != null); + assert(_debugRenderedViewsBetweenCallbacks == null); + assert(() { + _debugRenderedViews = null; + return true; + }()); } /// A callback that is invoked when pointer data is available. diff --git a/engine/src/flutter/lib/ui/window.dart b/engine/src/flutter/lib/ui/window.dart index 26a258cfa96..be761e548bb 100644 --- a/engine/src/flutter/lib/ui/window.dart +++ b/engine/src/flutter/lib/ui/window.dart @@ -353,7 +353,18 @@ class FlutterView { /// scheduling of frames. /// * [RendererBinding], the Flutter framework class which manages layout and /// painting. - void render(Scene scene) => _render(scene as _NativeScene); + void render(Scene scene) { + // Duplicated calls or calls outside of onBeginFrame/onDrawFrame (indicated + // by _debugRenderedViews being null) are ignored. See _debugRenderedViews. + bool validRender = true; + assert(() { + validRender = platformDispatcher._debugRenderedViews?.add(this) ?? false; + return true; + }()); + if (validRender) { + _render(scene as _NativeScene); + } + } @Native)>(symbol: 'PlatformConfigurationNativeApi::Render') external static void _render(_NativeScene scene);