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

Author: goderbauer

packages/flutter/lib/src/rendering/object.dart

@@ -987,8 +987,7 @@ class PipelineOwner {
     try {
       final List<RenderObject> dirtyNodes = _nodesNeedingPaint;
       _nodesNeedingPaint = <RenderObject>[];
-      // Sort the dirty nodes in reverse order (deepest first).
-      for (final RenderObject node in dirtyNodes..sort((RenderObject a, RenderObject b) => b.depth - a.depth)) {
+      for (final RenderObject node in dirtyNodes..sort((RenderObject a, RenderObject b) => a.depth - b.depth)) {
         assert(node._layerHandle.layer != null);
         if (node._needsPaint && node.owner == this) {
           if (node._layerHandle.layer!.attached) {
@@ -2365,12 +2364,8 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
       return true;
     }());
     // If we still need layout, then that means that we were skipped in the
-    // layout phase and therefore don't need painting. We might not know that
-    // 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.
+    // layout phase and therefore don't need painting. This can happen when an
+    // error occurred prior to our paint and our layer wasn't properly detached.
     if (_needsLayout)
       return;
     if (!kReleaseMode && debugProfilePaintsEnabled) {

packages/flutter/test/rendering/paint_order_test.dart

@@ -0,0 +1,84 @@
+// 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++;
+  }
+}