firka/flutter
firka/flutter
4
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Revert "[performance] Process dirty nodes from top to bottom during paint to avoid unnecessary layer tree walks" (#98520)

Browse Source
This commit is contained in:
Michael Goderbauer 2022-02-15 13:30:13 -08:00 committed by GitHub
parent db4c98fcb0
commit a454da0220
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 8 additions and 87 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
packages/flutter/lib/src/rendering/object.dart
View File

@ -987,7 +987,8 @@ class PipelineOwner {
try { try {
final List<RenderObject> dirtyNodes = _nodesNeedingPaint; final List<RenderObject> dirtyNodes = _nodesNeedingPaint;
_nodesNeedingPaint = <RenderObject>[]; _nodesNeedingPaint = <RenderObject>[];
for (final RenderObject node in dirtyNodes..sort((RenderObject a, RenderObject b) => a.depth - b.depth)) { // Sort the dirty nodes in reverse order (deepest first).
for (final RenderObject node in dirtyNodes..sort((RenderObject a, RenderObject b) => b.depth - a.depth)) {
assert(node._layerHandle.layer != null); assert(node._layerHandle.layer != null);
if (node._needsPaint && node.owner == this) { if (node._needsPaint && node.owner == this) {
if (node._layerHandle.layer!.attached) { if (node._layerHandle.layer!.attached) {
@ -2364,8 +2365,12 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
return true; return true;
}()); }());
// If we still need layout, then that means that we were skipped in the // If we still need layout, then that means that we were skipped in the
// layout phase and therefore don't need painting. This can happen when an // layout phase and therefore don't need painting. We might not know that
// error occurred prior to our paint and our layer wasn't properly detached. // yet (that is, our layer might not have been detached yet), because the
// same node that skipped us in layout is above us in the tree (obviously)
// and therefore may not have had a chance to paint yet (since the tree
// paints in reverse order). In particular this will happen if they have
// a different layer, because there's a repaint boundary between us.
if (_needsLayout) if (_needsLayout)
return; return;
if (!kReleaseMode && debugProfilePaintsEnabled) { if (!kReleaseMode && debugProfilePaintsEnabled) {

84
packages/flutter/test/rendering/paint_order_test.dart
View File

@ -1,84 +0,0 @@
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
import 'package:flutter/rendering.dart';
import 'package:flutter_test/flutter_test.dart';
import 'rendering_tester.dart';
void main() {
TestRenderingFlutterBinding.ensureInitialized();
test('paint RenderObjects from top to bottom to avoid duplicated layer tree walks', () {
// Historic Background: The order in which dirty nodes are processed for paint
// in [PipelineOwner.flushPaint] was changed from shallowest first to
// deepest first in August 2015, see https://github.com/flutter/flutter/commit/654fc7346eb79780aadeb9c2883ea9938d5f0bd3#diff-d06e00032e4d722205a2189ffbab26c1d8f5e13652efebc849583d0a1359fec9R612.
// The reasons for this change are lost in history. In February 2022 it
// was determined that deepest first actually caused additional unnecessary
// walks of the layer tree. To avoid those, the processing order was changed
// back to deepest first (which is also the order used by all other flush-methods on
// PipelineOwner). The test below encodes that the framework is not doing
// these unnecessary layer tree walks during paint that would occur during
// deepest first processing.
late RenderPositionedBox outer, inner;
late TestRenderBox testBox;
outer = RenderPositionedBox(
child: RenderRepaintBoundary(
child: inner = RenderPositionedBox(
child: testBox = TestRenderBox(),
),
),
);
// Paint the tree for the first time; Our TestLayer is attached exactly once.
expect((testBox.debugLayer! as TestLayer).attachCount, 0);
expect((testBox.debugLayer! as TestLayer).detachCount, 0);
layout(outer, phase: EnginePhase.paint);
expect((testBox.debugLayer! as TestLayer).attachCount, 1);
expect((testBox.debugLayer! as TestLayer).detachCount, 0);
// Repaint RenderObjects outside and inside the RepaintBoundary.
outer.markNeedsPaint();
inner.markNeedsPaint();
expect((testBox.debugLayer! as TestLayer).attachCount, 1);
expect((testBox.debugLayer! as TestLayer).detachCount, 0);
pumpFrame(phase: EnginePhase.paint);
// The TestLayer should be detached and reattached exactly once during the
// paint process. More attach/detach would indicate unnecessary
// additional walks of the layer tree.
expect((testBox.debugLayer! as TestLayer).attachCount, 2);
expect((testBox.debugLayer! as TestLayer).detachCount, 1);
});
}
class TestRenderBox extends RenderProxyBoxWithHitTestBehavior {
TestRenderBox() {
layer = TestLayer();
}
@override
void paint(PaintingContext context, Offset offset) {
context.addLayer(layer!);
}
}
class TestLayer extends OffsetLayer {
int attachCount = 0;
int detachCount = 0;
@override
void attach(Object owner) {
super.attach(owner);
attachCount++;
}
@override
void detach() {
super.detach();
detachCount++;
}
}