From b0a1649e48f357a9dad80b4bfae2cae9b1dff52f Mon Sep 17 00:00:00 2001 From: Hixie Date: Wed, 19 Aug 2015 12:16:41 -0700 Subject: [PATCH] Rewrite the MultiChildRenderObjectWrapper syncing algorithm. This also changes the way we insert nodes into a MultiChildRenderObjectWrapper's renderObject, which fixes issue #626. Now, instead of the slot being a renderObject, it's the Widget that currently uses that renderObject. That way when the Widget changes which renderObject to use, the siblings of that Widget in the same child list don't have to be notified of the change. I tested performance of the new algorithm vs the old algorithm using the stocks demo at idle and the stocks demo scrolling steadily. The data suggests the algorithms are roughly equivalent in performance. --- sky/packages/sky/lib/widgets/framework.dart | 244 +++++++++++--------- sky/tests/widgets/syncs2-expected.txt | 36 +++ sky/tests/widgets/syncs2.dart | 57 +++++ 3 files changed, 224 insertions(+), 113 deletions(-) create mode 100644 sky/tests/widgets/syncs2-expected.txt create mode 100644 sky/tests/widgets/syncs2.dart diff --git a/sky/packages/sky/lib/widgets/framework.dart b/sky/packages/sky/lib/widgets/framework.dart index 9c886ff1bf3d8..e1c78c55059be 100644 --- a/sky/packages/sky/lib/widgets/framework.dart +++ b/sky/packages/sky/lib/widgets/framework.dart @@ -19,7 +19,7 @@ export 'package:sky/rendering/box.dart' show BoxConstraints, BoxDecoration, Bord export 'package:sky/rendering/flex.dart' show FlexDirection; export 'package:sky/rendering/object.dart' show Point, Offset, Size, Rect, Color, Paint, Path; -final bool _shouldLogRenderDuration = false; +final bool _shouldLogRenderDuration = false; // see also 'enableProfilingLoop' argument to runApp() typedef Widget Builder(); typedef void WidgetTreeWalker(Widget); @@ -324,7 +324,7 @@ abstract class Widget { } if (oldNode != null) { - if (oldNode.runtimeType == newNode.runtimeType && oldNode.key == newNode.key) { + if (_canSync(newNode, oldNode)) { if (oldNode.retainStatefulNodeIfPossible(newNode)) { assert(oldNode.mounted); assert(!newNode.mounted); @@ -398,6 +398,10 @@ abstract class Widget { } } +bool _canSync(Widget a, Widget b) { + return a.runtimeType == b.runtimeType && a.key == b.key; +} + // Descendants of TagNode provide a way to tag RenderObjectWrapper and // Component nodes with annotations, such as event listeners, @@ -707,8 +711,7 @@ abstract class StatefulComponent extends Component { bool retainStatefulNodeIfPossible(StatefulComponent newNode) { assert(!_disqualifiedFromEverAppearingAgain); assert(newNode != null); - assert(runtimeType == newNode.runtimeType); - assert(key == newNode.key); + assert(_canSync(this, newNode); assert(_child != null); newNode._disqualifiedFromEverAppearingAgain = true; @@ -894,7 +897,7 @@ abstract class RenderObjectWrapper extends Widget { if (_ancestor is RenderObjectWrapper) _ancestor.insertChildRenderObject(this, slot); } else { - assert(old.runtimeType == runtimeType); + assert(_canSync(this, old)); _renderObject = old.renderObject; _ancestor = old._ancestor; assert(_renderObject != null); @@ -1007,8 +1010,9 @@ abstract class OneChildRenderObjectWrapper extends RenderObjectWrapper { abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper { - // In MultiChildRenderObjectWrapper subclasses, slots are RenderObject nodes - // to use as the "insert before" sibling in ContainerRenderObjectMixin.add() calls + // In MultiChildRenderObjectWrapper subclasses, slots are the Widget + // nodes whose RenderObjects are to be used as the "insert before" + // sibling in ContainerRenderObjectMixin.add() calls MultiChildRenderObjectWrapper({ Key key, List children }) : this.children = children == null ? const [] : children, @@ -1023,11 +1027,12 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper { walker(child); } - void insertChildRenderObject(RenderObjectWrapper child, dynamic slot) { + void insertChildRenderObject(RenderObjectWrapper child, Widget slot) { final renderObject = this.renderObject; // TODO(ianh): Remove this once the analyzer is cleverer - assert(slot == null || slot is RenderObject); + RenderObject nextSibling = slot != null ? slot.renderObject : null; + assert(nextSibling == null || nextSibling is RenderObject); assert(renderObject is ContainerRenderObjectMixin); - renderObject.add(child.renderObject, before: slot); + renderObject.add(child.renderObject, before: nextSibling); assert(renderObject == this.renderObject); // TODO(ianh): Remove this once the analyzer is cleverer } @@ -1060,121 +1065,134 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper { assert(renderObject is ContainerRenderObjectMixin); assert(old == null || old.renderObject == renderObject); - var startIndex = 0; - var endIndex = children.length; - - var oldChildren = old == null ? [] : old.children; - var oldStartIndex = 0; - var oldEndIndex = oldChildren.length; - - RenderObject nextSibling = null; - Widget currentNode = null; - Widget oldNode = null; - - void sync(int atIndex) { - children[atIndex] = syncChild(currentNode, oldNode, nextSibling); - assert(children[atIndex] != null); - assert(children[atIndex].parent == this); - if (atIndex > 0) - children[atIndex-1].updateSlot(children[atIndex].renderObject); - } - - // Scan backwards from end of list while nodes can be directly synced - // without reordering. - while (endIndex > startIndex && oldEndIndex > oldStartIndex) { - currentNode = children[endIndex - 1]; - oldNode = oldChildren[oldEndIndex - 1]; - - if (currentNode.runtimeType != oldNode.runtimeType || currentNode.key != oldNode.key) { + // This attempts to diff the new child list (this.children) with + // the old child list (old.children), and update our renderObject + // accordingly. + + // The cases it tries to optimise for are: + // - the old list is empty + // - the lists are identical + // - there is an insertion or removal of one or more widgets in + // only one place in the list + // If a widget with a key is in both lists, it will be synced. + // Widgets without keys might be synced but there is no guarantee. + + // The general approach is to sync the entire new list backwards, as follows: + // 1. Walk the lists from the top until you no longer have + // matching nodes. We don't sync these yet, but we now know to + // skip them below. We do this because at each sync we need to + // pass the pointer to the new next widget as the slot, which + // we can't do until we've synced the next child. + // 2. Walk the lists from the bottom, syncing nodes, until you no + // longer have matching nodes. + // At this point we narrowed the old and new lists to the point + // where the nodes no longer match. + // 3. Walk the narrowed part of the old list to get the list of + // keys and sync null with non-keyed items. + // 4. Walk the narrowed part of the new list backwards: + // * Sync unkeyed items with null + // * Sync keyed items with the source if it exists, else with null. + // 5. Walk the top list again but backwards, syncing the nodes. + // 6. Sync null with any items in the list of keys that are still + // mounted. + + final List newChildren = children; + final List oldChildren = old == null ? const [] : old.children; + int childrenTop = 0; + int newChildrenBottom = newChildren.length-1; + int oldChildrenBottom = oldChildren.length-1; + + // top of the lists + while ((childrenTop <= oldChildrenBottom) && (childrenTop <= newChildrenBottom)) { + Widget oldChild = oldChildren[childrenTop]; + assert(oldChild.mounted); + Widget newChild = newChildren[childrenTop]; + assert(newChild == oldChild || !newChild.mounted); + if (!_canSync(oldChild, newChild)) break; - } - - endIndex--; - oldEndIndex--; - sync(endIndex); - nextSibling = children[endIndex].renderObject; - } - - HashMap oldNodeIdMap = null; - - bool oldNodeReordered(Key key) { - return oldNodeIdMap != null && - oldNodeIdMap.containsKey(key) && - oldNodeIdMap[key] == null; + childrenTop += 1; } - void advanceOldStartIndex() { - oldStartIndex++; - while (oldStartIndex < oldEndIndex && - oldNodeReordered(oldChildren[oldStartIndex].key)) { - oldStartIndex++; - } - } + Widget nextSibling; - void ensureOldIdMap() { - if (oldNodeIdMap != null) - return; - - oldNodeIdMap = new HashMap(); - for (int i = oldStartIndex; i < oldEndIndex; i++) { - var node = oldChildren[i]; - if (node.key != null) - oldNodeIdMap.putIfAbsent(node.key, () => node); - } + // bottom of the lists + while ((childrenTop <= oldChildrenBottom) && (childrenTop <= newChildrenBottom)) { + Widget oldChild = oldChildren[oldChildrenBottom]; + assert(oldChild.mounted); + Widget newChild = newChildren[newChildrenBottom]; + assert(newChild == oldChild || !newChild.mounted); + if (!_canSync(oldChild, newChild)) + break; + newChild = syncChild(newChild, oldChild, nextSibling); + assert(newChild.mounted); + assert(oldChild == newChild || !oldChild.mounted); + newChildren[newChildrenBottom] = newChild; + nextSibling = newChild; + oldChildrenBottom -= 1; + newChildrenBottom -= 1; } - void searchForOldNode() { - if (currentNode.key == null) - return; // never re-order these nodes - - ensureOldIdMap(); - oldNode = oldNodeIdMap[currentNode.key]; - if (oldNode == null) - return; - - oldNodeIdMap[currentNode.key] = null; // mark it reordered - assert(renderObject is ContainerRenderObjectMixin); - assert(old.renderObject is ContainerRenderObjectMixin); - assert(oldNode.renderObject != null); - - renderObject.move(oldNode.renderObject, before: nextSibling); + // middle of the lists - old list + bool haveOldNodes = childrenTop <= oldChildrenBottom; + Map oldKeyedChildren; + if (haveOldNodes) { + oldKeyedChildren = new Map(); + while (childrenTop <= oldChildrenBottom) { + Widget oldChild = oldChildren[oldChildrenBottom]; + assert(oldChild.mounted); + if (oldChild.key != null) { + oldKeyedChildren[oldChild.key] = oldChild; + } else { + syncChild(null, oldChild, null); + } + oldChildrenBottom -= 1; + } } - // Scan forwards, this time we may re-order; - nextSibling = renderObject.firstChild; - while (startIndex < endIndex && oldStartIndex < oldEndIndex) { - currentNode = children[startIndex]; - oldNode = oldChildren[oldStartIndex]; - - if (currentNode.runtimeType == oldNode.runtimeType && currentNode.key == oldNode.key) { - nextSibling = renderObject.childAfter(nextSibling); - sync(startIndex); - startIndex++; - advanceOldStartIndex(); - continue; + // middle of the lists - new list + while (childrenTop <= newChildrenBottom) { + Widget oldChild; + Widget newChild = newChildren[newChildrenBottom]; + if (haveOldNodes) { + Key key = newChild.key; + if (key != null) { + oldChild = oldKeyedChildren[newChild.key]; + if (oldChild != null) { + if (oldChild.runtimeType != newChild.runtimeType) + oldChild = null; + oldKeyedChildren.remove(key); + } + } } - - oldNode = null; - searchForOldNode(); - sync(startIndex); - startIndex++; + assert(newChild == oldChild || !newChild.mounted); + newChild = syncChild(newChild, oldChild, nextSibling); + assert(newChild.mounted); + assert(oldChild == newChild || oldChild == null || !oldChild.mounted); + newChildren[newChildrenBottom] = newChild; + nextSibling = newChild; + newChildrenBottom -= 1; } - - // New insertions - oldNode = null; - while (startIndex < endIndex) { - currentNode = children[startIndex]; - sync(startIndex); - startIndex++; + assert(oldChildrenBottom == newChildrenBottom); + assert(childrenTop == newChildrenBottom+1); + + // now sync the top of the list + while (childrenTop > 0) { + childrenTop -= 1; + Widget oldChild = oldChildren[childrenTop]; + assert(oldChild.mounted); + Widget newChild = newChildren[childrenTop]; + assert(newChild == oldChild || !newChild.mounted); + assert(_canSync(oldChild, newChild)); + newChild = syncChild(newChild, oldChild, nextSibling); + assert(newChild.mounted); + assert(oldChild == newChild || oldChild == null || !oldChild.mounted); + newChildren[childrenTop] = newChild; + nextSibling = newChild; } - // Removals - currentNode = null; - while (oldStartIndex < oldEndIndex) { - oldNode = oldChildren[oldStartIndex]; - syncChild(null, oldNode, null); - assert(oldNode.parent == null); - advanceOldStartIndex(); + if (haveOldNodes && !oldKeyedChildren.isEmpty) { + for (Widget oldChild in oldKeyedChildren.values) + syncChild(null, oldChild, null); } assert(renderObject == this.renderObject); // TODO(ianh): Remove this once the analyzer is cleverer diff --git a/sky/tests/widgets/syncs2-expected.txt b/sky/tests/widgets/syncs2-expected.txt new file mode 100644 index 0000000000000..5073ba70dba0e --- /dev/null +++ b/sky/tests/widgets/syncs2-expected.txt @@ -0,0 +1,36 @@ +TestRenderView enabled + +PAINT FOR FRAME #1 ---------------------------------------------- +1 | TestPaintingCanvas() constructor: 800.0 x 600.0 +------------------------------------------------------------------------ + +PAINT FOR FRAME #2 ---------------------------------------------- +2 | TestPaintingCanvas() constructor: 800.0 x 600.0 +2 | paintChild RenderFlex at Point(0.0, 0.0) +2 | | TestPaintingCanvas() constructor: 800.0 x 600.0 +2 | | paintChild RenderConstrainedBox at Point(350.0, 0.0) +2 | | | TestPaintingCanvas() constructor: 800.0 x 600.0 +2 | | paintChild RenderConstrainedBox at Point(350.0, 100.0) +2 | | | TestPaintingCanvas() constructor: 800.0 x 600.0 +------------------------------------------------------------------------ + +PAINT FOR FRAME #3 ---------------------------------------------- +3 | TestPaintingCanvas() constructor: 800.0 x 600.0 +3 | paintChild RenderFlex at Point(0.0, 0.0) +3 | | TestPaintingCanvas() constructor: 800.0 x 600.0 +3 | | paintChild RenderConstrainedBox at Point(350.0, 0.0) +3 | | | TestPaintingCanvas() constructor: 800.0 x 600.0 +3 | | paintChild RenderPadding at Point(390.0, 100.0) +3 | | | TestPaintingCanvas() constructor: 800.0 x 600.0 +------------------------------------------------------------------------ + +PAINT FOR FRAME #4 ---------------------------------------------- +4 | TestPaintingCanvas() constructor: 800.0 x 600.0 +4 | paintChild RenderFlex at Point(0.0, 0.0) +4 | | TestPaintingCanvas() constructor: 800.0 x 600.0 +4 | | paintChild RenderPadding at Point(390.0, 0.0) +4 | | | TestPaintingCanvas() constructor: 800.0 x 600.0 +4 | | paintChild RenderPadding at Point(390.0, 20.0) +4 | | | TestPaintingCanvas() constructor: 800.0 x 600.0 +------------------------------------------------------------------------ +PAINTED 4 FRAMES diff --git a/sky/tests/widgets/syncs2.dart b/sky/tests/widgets/syncs2.dart new file mode 100644 index 0000000000000..ddc0c722844c6 --- /dev/null +++ b/sky/tests/widgets/syncs2.dart @@ -0,0 +1,57 @@ +// Copyright 2015 The Chromium 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:sky/widgets.dart'; + +import '../resources/display_list.dart'; + +// see issue 626 + +class ProblemComponent extends StatefulComponent { + + void syncFields(ProblemComponent source) { } + + bool _flag = false; + + void flip() { + setState(() { + _flag = true; + }); + } + + Widget build() { + if (_flag) + return new Padding(padding: const EdgeDims.all(10.0)); + return new SizedBox(width: 100.0, height: 100.0); + } +} + +ProblemComponent a; +ProblemComponent b; + +class TestApp extends App { + Widget build() { + return new Flex([ + a = new ProblemComponent(), + b = new ProblemComponent() + ], + direction: FlexDirection.vertical); + } +} + +main() async { + try { + TestRenderView renderViewOverride = new TestRenderView(); + TestApp app = new TestApp(); + runApp(app, renderViewOverride: renderViewOverride); + await renderViewOverride.checkFrame(); + b.flip(); + await renderViewOverride.checkFrame(); + a.flip(); + await renderViewOverride.checkFrame(); + renderViewOverride.endTest(); + } catch (e, s) { + print("Exception: $e\nStack:\n$s\n"); + } +}