[webkit-changes] [291416] trunk/Source/WebCore

Thu, 17 Mar 2022 09:54:54 -0700

Title: [291416] trunk/Source/WebCore
Revision
291416
Author
cdu...@apple.com
Date
2022-03-17 09:54:03 -0700 (Thu, 17 Mar 2022)

Log Message

Stop returning NodeVector from functions
https://bugs.webkit.org/show_bug.cgi?id=237988

Reviewed by Darin Adler.

Stop returning NodeVector from functions and use a out-parameter instead. While this doesn't look
as modern, this is actually more efficient. This is because NodeVector has a fairly large inline
buffer and is thus not that cheap to "move".

This was causing functions like `ContainerNode::parserAppendChild(Node&)` to spend unnecessary
time under:
`VectorBuffer<Ref<Node, RawPtrTraits<Node>>, 11, FastMalloc>::swap(VectorBuffer<Ref<Node, RawPtrTraits<Node>>, 11, FastMalloc>&, unsigned long, unsigned long)`

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::removeAllChildrenWithScriptAssertion):
(WebCore::executeNodeInsertionWithScriptAssertion):
(WebCore::ContainerNode::removeSelfOrChildNodesForInsertion):
(WebCore::ContainerNode::takeAllChildrenFrom):
(WebCore::ContainerNode::replaceAll):
(WebCore::ContainerNode::removeChildren):
(WebCore::ContainerNode::replaceChildren):
* dom/ContainerNode.h:
(WebCore::collectChildNodes):
* dom/ContainerNodeAlgorithms.cpp:
(WebCore::notifyChildNodeInserted):
* dom/ContainerNodeAlgorithms.h:
* dom/Element.cpp:
(WebCore::Element::addShadowRoot):
(WebCore::Element::insertAdjacentHTML):
* editing/ApplyStyleCommand.cpp:
(WebCore::ApplyStyleCommand::pushDownInlineStyleAroundNode):
* editing/ReplaceNodeWithSpanCommand.cpp:
(WebCore::swapInNodePreservingAttributesAndChildren):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (291415 => 291416)


--- trunk/Source/WebCore/ChangeLog	2022-03-17 16:52:37 UTC (rev 291415)
+++ trunk/Source/WebCore/ChangeLog	2022-03-17 16:54:03 UTC (rev 291416)
@@ -1,3 +1,39 @@
+2022-03-17  Chris Dumez  <cdu...@apple.com>
+
+        Stop returning NodeVector from functions
+        https://bugs.webkit.org/show_bug.cgi?id=237988
+
+        Reviewed by Darin Adler.
+
+        Stop returning NodeVector from functions and use a out-parameter instead. While this doesn't look
+        as modern, this is actually more efficient. This is because NodeVector has a fairly large inline
+        buffer and is thus not that cheap to "move".
+
+        This was causing functions like `ContainerNode::parserAppendChild(Node&)` to spend unnecessary
+        time under:
+        `VectorBuffer<Ref<Node, RawPtrTraits<Node>>, 11, FastMalloc>::swap(VectorBuffer<Ref<Node, RawPtrTraits<Node>>, 11, FastMalloc>&, unsigned long, unsigned long)`
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeAllChildrenWithScriptAssertion):
+        (WebCore::executeNodeInsertionWithScriptAssertion):
+        (WebCore::ContainerNode::removeSelfOrChildNodesForInsertion):
+        (WebCore::ContainerNode::takeAllChildrenFrom):
+        (WebCore::ContainerNode::replaceAll):
+        (WebCore::ContainerNode::removeChildren):
+        (WebCore::ContainerNode::replaceChildren):
+        * dom/ContainerNode.h:
+        (WebCore::collectChildNodes):
+        * dom/ContainerNodeAlgorithms.cpp:
+        (WebCore::notifyChildNodeInserted):
+        * dom/ContainerNodeAlgorithms.h:
+        * dom/Element.cpp:
+        (WebCore::Element::addShadowRoot):
+        (WebCore::Element::insertAdjacentHTML):
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::pushDownInlineStyleAroundNode):
+        * editing/ReplaceNodeWithSpanCommand.cpp:
+        (WebCore::swapInNodePreservingAttributesAndChildren):
+
 2022-03-17  Ben Nham  <n...@apple.com>
 
         Only show notification permission prompt on user gesture

Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (291415 => 291416)


--- trunk/Source/WebCore/dom/ContainerNode.cpp	2022-03-17 16:52:37 UTC (rev 291415)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp	2022-03-17 16:54:03 UTC (rev 291416)
@@ -79,9 +79,10 @@
 ScriptDisallowedScope::EventAllowedScope* ScriptDisallowedScope::EventAllowedScope::s_currentScope = nullptr;
 #endif
 
-ALWAYS_INLINE NodeVector ContainerNode::removeAllChildrenWithScriptAssertion(ChildChange::Source source, DeferChildrenChanged deferChildrenChanged)
+ALWAYS_INLINE void ContainerNode::removeAllChildrenWithScriptAssertion(ChildChange::Source source, NodeVector& children, DeferChildrenChanged deferChildrenChanged)
 {
-    auto children = collectChildNodes(*this);
+    ASSERT(children.isEmpty());
+    collectChildNodes(*this, children);
 
     if (UNLIKELY(isDocumentFragmentForInnerOuterHTML())) {
         ScriptDisallowedScope::InMainThread scriptDisallowedScope;
@@ -90,7 +91,7 @@
         while (RefPtr<Node> child = m_firstChild)
             removeBetween(nullptr, child->nextSibling(), *child);
         document().incDOMTreeVersion();
-        return children;
+        return;
     }
 
     if (source == ChildChange::Source::API) {
@@ -136,8 +137,6 @@
 
     if (deferChildrenChanged == DeferChildrenChanged::No)
         childrenChanged(childChange);
-
-    return children;
 }
 
 static ContainerNode::ChildChange makeChildChangeForRemoval(Node& childToRemove, ContainerNode::ChildChange::Source source)
@@ -262,7 +261,7 @@
 
         doNodeInsertion();
         ChildListMutationScope(containerNode).childAdded(child);
-        postInsertionNotificationTargets = notifyChildNodeInserted(containerNode, child);
+        notifyChildNodeInserted(containerNode, child, postInsertionNotificationTargets);
     }
 
     // FIXME: Move childrenChanged into ScriptDisallowedScope block.
@@ -282,8 +281,8 @@
         if (!fragment->hasChildNodes())
             return { };
 
-        auto removedChildNodes = fragment->removeAllChildrenWithScriptAssertion(ContainerNode::ChildChange::Source::API);
-        nodesForInsertion.swap(removedChildNodes);
+        ASSERT(nodesForInsertion.isEmpty());
+        fragment->removeAllChildrenWithScriptAssertion(ContainerNode::ChildChange::Source::API, nodesForInsertion);
 
         fragment->rebuildSVGExtensionsElementsIfNecessary();
         fragment->dispatchSubtreeModifiedEvent();
@@ -328,7 +327,8 @@
 {
     ASSERT(oldParent);
 
-    auto children = oldParent->removeAllChildrenWithScriptAssertion(ChildChange::Source::Parser);
+    NodeVector children;
+    oldParent->removeAllChildrenWithScriptAssertion(ChildChange::Source::Parser, children);
 
     // FIXME: assert that we don't dispatch events here since this container node is still disconnected.
     for (auto& child : children) {
@@ -710,7 +710,8 @@
 
     Ref<ContainerNode> protectedThis(*this);
     ChildListMutationScope mutation(*this);
-    removeAllChildrenWithScriptAssertion(ChildChange::Source::API, DeferChildrenChanged::Yes);
+    NodeVector removedChildren;
+    removeAllChildrenWithScriptAssertion(ChildChange::Source::API, removedChildren, DeferChildrenChanged::Yes);
 
     executeNodeInsertionWithScriptAssertion(*this, *node, nullptr, ChildChange::Source::API, ReplacedAllChildren::Yes, [&] {
         InspectorInstrumentation::willInsertDOMNode(document(), *this);
@@ -742,7 +743,8 @@
         return;
 
     Ref<ContainerNode> protectedThis(*this);
-    removeAllChildrenWithScriptAssertion(ChildChange::Source::API);
+    NodeVector removedChildren;
+    removeAllChildrenWithScriptAssertion(ChildChange::Source::API, removedChildren);
 
     rebuildSVGExtensionsElementsIfNecessary();
     dispatchSubtreeModifiedEvent();
@@ -1026,7 +1028,8 @@
     // step 3
     Ref protectedThis { *this };
     ChildListMutationScope mutation(*this);
-    removeAllChildrenWithScriptAssertion(ChildChange::Source::API, DeferChildrenChanged::No);
+    NodeVector removedChildren;
+    removeAllChildrenWithScriptAssertion(ChildChange::Source::API, removedChildren, DeferChildrenChanged::No);
 
     if (node) {
         if (auto appendResult = appendChildWithoutPreInsertionValidityCheck(*node); appendResult.hasException())

Modified: trunk/Source/WebCore/dom/ContainerNode.h (291415 => 291416)


--- trunk/Source/WebCore/dom/ContainerNode.h	2022-03-17 16:52:37 UTC (rev 291415)
+++ trunk/Source/WebCore/dom/ContainerNode.h	2022-03-17 16:54:03 UTC (rev 291416)
@@ -170,7 +170,7 @@
 private:
     void executePreparedChildrenRemoval();
     enum class DeferChildrenChanged { Yes, No };
-    NodeVector removeAllChildrenWithScriptAssertion(ChildChange::Source, DeferChildrenChanged = DeferChildrenChanged::No);
+    void removeAllChildrenWithScriptAssertion(ChildChange::Source, NodeVector& children, DeferChildrenChanged = DeferChildrenChanged::No);
     bool removeNodeWithScriptAssertion(Node&, ChildChange::Source);
     ExceptionOr<void> removeSelfOrChildNodesForInsertion(Node&, NodeVector&);
 
@@ -223,12 +223,10 @@
     return traverseToRootNode();
 }
 
-inline NodeVector collectChildNodes(Node& node)
+inline void collectChildNodes(Node& node, NodeVector& children)
 {
-    NodeVector children;
     for (Node* child = node.firstChild(); child; child = child->nextSibling())
         children.append(*child);
-    return children;
 }
 
 class ChildNodesLazySnapshot {

Modified: trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp (291415 => 291416)


--- trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp	2022-03-17 16:52:37 UTC (rev 291415)
+++ trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp	2022-03-17 16:54:03 UTC (rev 291416)
@@ -86,7 +86,9 @@
         notifyNodeInsertedIntoTree(parentOfInsertedTree, *root, TreeScopeChange::DidNotChange, postInsertionNotificationTargets);
 }
 
-NodeVector notifyChildNodeInserted(ContainerNode& parentOfInsertedTree, Node& node)
+// We intentionally use an out-parameter for postInsertionNotificationTargets instead of returning the vector. This is because
+// NodeVector has a large inline buffer and is thus not cheap to move.
+void notifyChildNodeInserted(ContainerNode& parentOfInsertedTree, Node& node, NodeVector& postInsertionNotificationTargets)
 {
     ASSERT(ScriptDisallowedScope::InMainThread::hasDisallowedScope());
 
@@ -95,8 +97,6 @@
     Ref<Document> protectDocument(node.document());
     Ref<Node> protectNode(node);
 
-    NodeVector postInsertionNotificationTargets;
-
     // Tree scope has changed if the container node into which "node" is inserted is in a document or a shadow root.
     auto treeScopeChange = parentOfInsertedTree.isInTreeScope() ? TreeScopeChange::Changed : TreeScopeChange::DidNotChange;
     if (parentOfInsertedTree.isConnected())
@@ -103,8 +103,6 @@
         notifyNodeInsertedIntoDocument(parentOfInsertedTree, node, treeScopeChange, postInsertionNotificationTargets);
     else
         notifyNodeInsertedIntoTree(parentOfInsertedTree, node, treeScopeChange, postInsertionNotificationTargets);
-
-    return postInsertionNotificationTargets;
 }
 
 inline RemovedSubtreeObservability observabilityOfRemovedNode(Node& node)

Modified: trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h (291415 => 291416)


--- trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h	2022-03-17 16:52:37 UTC (rev 291415)
+++ trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h	2022-03-17 16:54:03 UTC (rev 291416)
@@ -61,7 +61,8 @@
 };
 #endif // not ASSERT_ENABLED
 
-NodeVector notifyChildNodeInserted(ContainerNode& parentOfInsertedTree, Node&);
+void notifyChildNodeInserted(ContainerNode& parentOfInsertedTree, Node&, NodeVector& postInsertionNotificationTargets);
+
 enum class RemovedSubtreeObservability {
     NotObservable,
     MaybeObservableByRefPtr,

Modified: trunk/Source/WebCore/dom/Element.cpp (291415 => 291416)


--- trunk/Source/WebCore/dom/Element.cpp	2022-03-17 16:52:37 UTC (rev 291415)
+++ trunk/Source/WebCore/dom/Element.cpp	2022-03-17 16:54:03 UTC (rev 291416)
@@ -2458,11 +2458,9 @@
         shadowRoot.setHost(*this);
         shadowRoot.setParentTreeScope(treeScope());
 
-#if ASSERT_ENABLED
-        ASSERT(notifyChildNodeInserted(*this, shadowRoot).isEmpty());
-#else
-        notifyChildNodeInserted(*this, shadowRoot);
-#endif
+        NodeVector postInsertionNotificationTargets;
+        notifyChildNodeInserted(*this, shadowRoot, postInsertionNotificationTargets);
+        ASSERT_UNUSED(postInsertionNotificationTargets, postInsertionNotificationTargets.isEmpty());
 
         InspectorInstrumentation::didPushShadowRoot(*this, shadowRoot);
 
@@ -4640,7 +4638,7 @@
     if (UNLIKELY(addedNodes)) {
         // Must be called before insertAdjacent, as otherwise the children of fragment will be moved
         // to their new parent and will be harder to keep track of.
-        *addedNodes = collectChildNodes(fragment.returnValue());
+        collectChildNodes(fragment.returnValue(), *addedNodes);
     }
 
     // Step 4.

Modified: trunk/Source/WebCore/editing/ApplyStyleCommand.cpp (291415 => 291416)


--- trunk/Source/WebCore/editing/ApplyStyleCommand.cpp	2022-03-17 16:52:37 UTC (rev 291415)
+++ trunk/Source/WebCore/editing/ApplyStyleCommand.cpp	2022-03-17 16:54:03 UTC (rev 291416)
@@ -1046,7 +1046,8 @@
     // Each child of the removed element, exclusing ancestors of targetNode, is then wrapped by clones of elements in elementsToPushDown.
     Vector<Ref<Element>> elementsToPushDown;
     while (current && current != targetNode && current->contains(targetNode)) {
-        auto currentChildren = collectChildNodes(*current);
+        NodeVector currentChildren;
+        collectChildNodes(*current, currentChildren);
 
         RefPtr<StyledElement> styledElement;
         if (is<StyledElement>(*current) && isStyledInlineElementToRemove(downcast<Element>(current.get()))) {

Modified: trunk/Source/WebCore/editing/ReplaceNodeWithSpanCommand.cpp (291415 => 291416)


--- trunk/Source/WebCore/editing/ReplaceNodeWithSpanCommand.cpp	2022-03-17 16:52:37 UTC (rev 291415)
+++ trunk/Source/WebCore/editing/ReplaceNodeWithSpanCommand.cpp	2022-03-17 16:54:03 UTC (rev 291416)
@@ -49,7 +49,8 @@
 
     // FIXME: Fix this to send the proper MutationRecords when MutationObservers are present.
     newNode.cloneDataFromElement(nodeToReplace);
-    auto children = collectChildNodes(nodeToReplace);
+    NodeVector children;
+    collectChildNodes(nodeToReplace, children);
     for (auto& child : children)
         newNode.appendChild(child);
 

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to