Skip to content

Commit

Permalink
Skip invalid renders in Dart (flutter#47323)
Browse files Browse the repository at this point in the history
With this PR, invalid renders are skipped in Dart in both release build and debug build.

Part of flutter/flutter#137073 and part of relanding flutter#45555.

Unit tests will be relanded in a following PR once this PR is confirmed performance-safe.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
  • Loading branch information
dkwingsmt committed Oct 26, 2023
1 parent 87f384c commit bedc49e
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 55 deletions.
58 changes: 19 additions & 39 deletions lib/ui/platform_dispatcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,8 @@ 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.
// 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]
Expand All @@ -320,16 +319,9 @@ class PlatformDispatcher {
// 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<FlutterView>? _debugRenderedViews;
// A debug-only variable that temporarily stores the `_renderedViews` value
// between `_beginFrame` and `_drawFrame`.
//
// In release build, this variable is null.
Set<FlutterView>? _debugRenderedViewsBetweenCallbacks;
Set<FlutterView>? _renderedViews;
// The `_renderedViews` value between `_beginFrame` and `_drawFrame`.
Set<FlutterView>? _renderedViewsBetweenCallbacks;

/// A callback invoked when any view begins a frame.
///
Expand All @@ -351,26 +343,20 @@ class PlatformDispatcher {

// Called from the engine, via hooks.dart
void _beginFrame(int microseconds) {
assert(_debugRenderedViews == null);
assert(_debugRenderedViewsBetweenCallbacks == null);
assert(() {
_debugRenderedViews = <FlutterView>{};
return true;
}());
assert(_renderedViews == null);
assert(_renderedViewsBetweenCallbacks == null);
_renderedViews = <FlutterView>{};

_invoke1<Duration>(
onBeginFrame,
_onBeginFrameZone,
Duration(microseconds: microseconds),
);

assert(_debugRenderedViews != null);
assert(_debugRenderedViewsBetweenCallbacks == null);
assert(() {
_debugRenderedViewsBetweenCallbacks = _debugRenderedViews;
_debugRenderedViews = null;
return true;
}());
assert(_renderedViews != null);
assert(_renderedViewsBetweenCallbacks == null);
_renderedViewsBetweenCallbacks = _renderedViews;
_renderedViews = null;
}

/// A callback that is invoked for each frame after [onBeginFrame] has
Expand All @@ -388,22 +374,16 @@ class PlatformDispatcher {

// Called from the engine, via hooks.dart
void _drawFrame() {
assert(_debugRenderedViews == null);
assert(_debugRenderedViewsBetweenCallbacks != null);
assert(() {
_debugRenderedViews = _debugRenderedViewsBetweenCallbacks;
_debugRenderedViewsBetweenCallbacks = null;
return true;
}());
assert(_renderedViews == null);
assert(_renderedViewsBetweenCallbacks != null);
_renderedViews = _renderedViewsBetweenCallbacks;
_renderedViewsBetweenCallbacks = null;

_invoke(onDrawFrame, _onDrawFrameZone);

assert(_debugRenderedViews != null);
assert(_debugRenderedViewsBetweenCallbacks == null);
assert(() {
_debugRenderedViews = null;
return true;
}());
assert(_renderedViews != null);
assert(_renderedViewsBetweenCallbacks == null);
_renderedViews = null;
}

/// A callback that is invoked when pointer data is available.
Expand Down
8 changes: 2 additions & 6 deletions lib/ui/window.dart
Original file line number Diff line number Diff line change
Expand Up @@ -355,14 +355,10 @@ class FlutterView {
/// painting.
void render(Scene scene) {
// Duplicated calls or calls outside of onBeginFrame/onDrawFrame (indicated
// by _debugRenderedViews being null) are ignored. See _debugRenderedViews.
// by _renderedViews being null) are ignored. See _renderedViews.
// TODO(dkwingsmt): We should change this skip into an assertion.
// https://github.com/flutter/flutter/issues/137073
bool validRender = true;
assert(() {
validRender = platformDispatcher._debugRenderedViews?.add(this) ?? false;
return true;
}());
final bool validRender = platformDispatcher._renderedViews?.add(this) ?? false;
if (validRender) {
_render(viewId, scene as _NativeScene);
}
Expand Down
10 changes: 1 addition & 9 deletions shell/common/animator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -179,15 +179,7 @@ void Animator::EndFrame() {
void Animator::Render(int64_t view_id,
std::unique_ptr<flutter::LayerTree> layer_tree,
float device_pixel_ratio) {
// Animator::Render should be called between BeginFrame and EndFrame,
// which is indicated by frame_timings_recorder_ being non-null.
// This might happen on release build, and is guarded by PlatformDispatcher on
// debug build.
// TODO(dkwingsmt): We should change this skip into an assertion.
// https://github.com/flutter/flutter/issues/137073
if (frame_timings_recorder_ == nullptr) {
return;
}
FML_CHECK(frame_timings_recorder_ != nullptr);

has_rendered_ = true;

Expand Down
3 changes: 2 additions & 1 deletion shell/common/animator.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ class Animator final {
///
/// This method must be called during a vsync callback, or
/// technically, between Animator::BeginFrame and Animator::EndFrame
/// (both private methods). Otherwise, this call will be ignored.
/// (both private methods). Otherwise, an assertion will be
/// triggered.
///
void Render(int64_t view_id,
std::unique_ptr<flutter::LayerTree> layer_tree,
Expand Down

0 comments on commit bedc49e

Please sign in to comment.