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

Thu, 04 Feb 2021 16:11:14 -0800

Title: [272394] trunk/Source/WebCore
Revision
272394
Author
rn...@webkit.org
Date
2021-02-04 16:10:30 -0800 (Thu, 04 Feb 2021)

Log Message

Avoid creating JS wrapper on a removed node when the subtree is not observable
https://bugs.webkit.org/show_bug.cgi?id=221243
<rdar://problem/73719386>

Reviewed by Geoffrey Garen.

Prior to this patch, WebKit forced the creation the JS wrapper on the root DOM node of a removed subtree
to avoid script observable deletion of nodes. This is necessary because DOM nodes are reference counted
in the C++ side, and while a DOM node keeps its child nodes alive, it won't keep its parent node alive
to avoid reference cycles (leaks). If we didn't force the creation of the JS wrapper and the root node
of the removed subtree didn't have any external reference to it in C++ side, we would happily delete it
and all its descendant nodes above any subtree with a JS wrapper or an external C++ reference.

While this turned out to be an effective strategy for implementing DOM nodes' GC semantics correctly,
it has a significant runtime and memory cost - the latter is because we don't collect the JS wrappers
of DOM nodes once they're created until they're ready to be destructed.

This patch introduces a new optimization to avoid creating these JS wrappers when the removed subtree
won't be observable by scripts in the future. The current heuristic is to check if the removed node
has any external reference to it (i.e. refCount() > 0 excluding any reference counting that happens within
our algorithm). This is sufficient because a given node should not be observable at a later time unless
it has an external reference to it. This is ~0.5% progression on Speedometer-2.0 on MacBookAir7,2.

To do this, we take advantage of the fact notifyChildNodeRemoved already traverses each removed subtree,
and check if any of them has a reference count greater than 1 (greater than 1 because notifyChildNodeRemoved
itself increments node's reference count before calling itself on child nodes). Note that we exclude the
root node of the removed subtree as these JS wrapper creation is only needed to keep the root node alive.
If the root node is already kept alive by some external reference to it, there is no need to keep it alive
again by creating a JS wrapper on it.

No new tests since existing tests such as fast/dom/gc-3.html covers it.

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::removeAllChildrenWithScriptAssertion): Call willCreatePossiblyOrphanedTreeByRemoval
if the removed subtree contains an observable node.
(WebCore::ContainerNode::removeNodeWithScriptAssertion): Ditto.
(WebCore::ContainerNode::removeSelfOrChildNodesForInsertion): Renamed from collectChildrenAndRemoveFromOldParent.
Avoid collecting the removed nodes in this function in addition to removeAllChildrenWithScriptAssertion.
(WebCore::ContainerNode::insertBefore):
(WebCore::ContainerNode::replaceChild):
(WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck):
(WebCore::dispatchChildRemovalEvents): Don't call willCreatePossiblyOrphanedTreeByRemoval here.
* dom/ContainerNode.h:
* dom/ContainerNodeAlgorithms.cpp:
(WebCore::observabilityOfRemovedNode): Added. Checks the observability of a node excluding the root node of
the removed subtree since it needs a special case in removeAllChildrenWithScriptAssertion.
As notifyNodeRemovedFromDocument and notifyNodeRemovedFromTree ref's each child node before recursing on itself,
we check refCount() > 1 here.
(WebCore::updateObservability): Added. A helper function to update RemovedSubtreeObservability.
(WebCore::notifyNodeRemovedFromDocument): Now returns RemovedSubtreeObservability,
(WebCore::notifyNodeRemovedFromTree): Ditto.
(WebCore::notifyChildNodeRemoved): Ditto.
* dom/ContainerNodeAlgorithms.h:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (272393 => 272394)


--- trunk/Source/WebCore/ChangeLog	2021-02-04 23:59:53 UTC (rev 272393)
+++ trunk/Source/WebCore/ChangeLog	2021-02-05 00:10:30 UTC (rev 272394)
@@ -1,3 +1,59 @@
+2021-02-04  Ryosuke Niwa  <rn...@webkit.org>
+
+        Avoid creating JS wrapper on a removed node when the subtree is not observable
+        https://bugs.webkit.org/show_bug.cgi?id=221243
+        <rdar://problem/73719386>
+
+        Reviewed by Geoffrey Garen.
+
+        Prior to this patch, WebKit forced the creation the JS wrapper on the root DOM node of a removed subtree
+        to avoid script observable deletion of nodes. This is necessary because DOM nodes are reference counted
+        in the C++ side, and while a DOM node keeps its child nodes alive, it won't keep its parent node alive
+        to avoid reference cycles (leaks). If we didn't force the creation of the JS wrapper and the root node
+        of the removed subtree didn't have any external reference to it in C++ side, we would happily delete it
+        and all its descendant nodes above any subtree with a JS wrapper or an external C++ reference.
+
+        While this turned out to be an effective strategy for implementing DOM nodes' GC semantics correctly,
+        it has a significant runtime and memory cost - the latter is because we don't collect the JS wrappers
+        of DOM nodes once they're created until they're ready to be destructed.
+
+        This patch introduces a new optimization to avoid creating these JS wrappers when the removed subtree
+        won't be observable by scripts in the future. The current heuristic is to check if the removed node
+        has any external reference to it (i.e. refCount() > 0 excluding any reference counting that happens within
+        our algorithm). This is sufficient because a given node should not be observable at a later time unless
+        it has an external reference to it. This is ~0.5% progression on Speedometer-2.0 on MacBookAir7,2.
+
+        To do this, we take advantage of the fact notifyChildNodeRemoved already traverses each removed subtree,
+        and check if any of them has a reference count greater than 1 (greater than 1 because notifyChildNodeRemoved
+        itself increments node's reference count before calling itself on child nodes). Note that we exclude the
+        root node of the removed subtree as these JS wrapper creation is only needed to keep the root node alive.
+        If the root node is already kept alive by some external reference to it, there is no need to keep it alive
+        again by creating a JS wrapper on it.
+
+        No new tests since existing tests such as fast/dom/gc-3.html covers it.
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeAllChildrenWithScriptAssertion): Call willCreatePossiblyOrphanedTreeByRemoval
+        if the removed subtree contains an observable node.
+        (WebCore::ContainerNode::removeNodeWithScriptAssertion): Ditto.
+        (WebCore::ContainerNode::removeSelfOrChildNodesForInsertion): Renamed from collectChildrenAndRemoveFromOldParent.
+        Avoid collecting the removed nodes in this function in addition to removeAllChildrenWithScriptAssertion.
+        (WebCore::ContainerNode::insertBefore):
+        (WebCore::ContainerNode::replaceChild):
+        (WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck):
+        (WebCore::dispatchChildRemovalEvents): Don't call willCreatePossiblyOrphanedTreeByRemoval here.
+        * dom/ContainerNode.h:
+        * dom/ContainerNodeAlgorithms.cpp:
+        (WebCore::observabilityOfRemovedNode): Added. Checks the observability of a node excluding the root node of
+        the removed subtree since it needs a special case in removeAllChildrenWithScriptAssertion.
+        As notifyNodeRemovedFromDocument and notifyNodeRemovedFromTree ref's each child node before recursing on itself,
+        we check refCount() > 1 here.
+        (WebCore::updateObservability): Added. A helper function to update RemovedSubtreeObservability.
+        (WebCore::notifyNodeRemovedFromDocument): Now returns RemovedSubtreeObservability,
+        (WebCore::notifyNodeRemovedFromTree): Ditto.
+        (WebCore::notifyChildNodeRemoved): Ditto.
+        * dom/ContainerNodeAlgorithms.h:
+
 2021-02-04  Chris Dumez  <cdu...@apple.com>
 
         RELEASE_ASSERT(bigInt) in VM constructor when constructing a WorkletGlobalScope

Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (272393 => 272394)


--- trunk/Source/WebCore/dom/ContainerNode.cpp	2021-02-04 23:59:53 UTC (rev 272393)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp	2021-02-05 00:10:30 UTC (rev 272394)
@@ -111,7 +111,9 @@
 
     while (RefPtr<Node> child = m_firstChild) {
         removeBetween(nullptr, child->nextSibling(), *child);
-        notifyChildNodeRemoved(*this, *child);
+        auto subtreeObservability = notifyChildNodeRemoved(*this, *child);
+        if (source == ChildChange::Source::API && subtreeObservability == RemovedSubtreeObservability::MaybeObservableByRefPtr)
+            willCreatePossiblyOrphanedTreeByRemoval(child.get());
     }
 
     if (deferChildrenChanged == DeferChildrenChanged::No)
@@ -149,6 +151,7 @@
         return false;
 
     ChildChange change;
+    RemovedSubtreeObservability subtreeObservability;
     {
         WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
         ScriptDisallowedScope::InMainThread scriptDisallowedScope;
@@ -164,7 +167,7 @@
         RefPtr<Node> previousSibling = childToRemove.previousSibling();
         RefPtr<Node> nextSibling = childToRemove.nextSibling();
         removeBetween(previousSibling.get(), nextSibling.get(), childToRemove);
-        notifyChildNodeRemoved(*this, childToRemove);
+        subtreeObservability = notifyChildNodeRemoved(*this, childToRemove);
 
         change.type = is<Element>(childToRemove) ?
             ChildChange::Type::ElementRemoved :
@@ -176,6 +179,9 @@
         change.source = source;
     }
 
+    if (source == ChildChange::Source::API && subtreeObservability == RemovedSubtreeObservability::MaybeObservableByRefPtr)
+        willCreatePossiblyOrphanedTreeByRemoval(&childToRemove);
+
     // FIXME: Move childrenChanged into ScriptDisallowedScope block.
     childrenChanged(change);
 
@@ -224,18 +230,26 @@
         dispatchChildInsertionEvents(child);
 }
 
-static ExceptionOr<void> collectChildrenAndRemoveFromOldParent(Node& node, NodeVector& nodes)
+ExceptionOr<void> ContainerNode::removeSelfOrChildNodesForInsertion(Node& child, NodeVector& nodesForInsertion)
 {
-    if (!is<DocumentFragment>(node)) {
-        nodes.append(node);
-        auto* oldParent = node.parentNode();
+    if (!is<DocumentFragment>(child)) {
+        nodesForInsertion.append(child);
+        auto oldParent = makeRefPtr(child.parentNode());
         if (!oldParent)
             return { };
-        return oldParent->removeChild(node);
+        return oldParent->removeChild(child);
     }
 
-    nodes = collectChildNodes(node);
-    downcast<DocumentFragment>(node).removeChildren();
+    auto& fragment = downcast<DocumentFragment>(child);
+    if (!fragment.hasChildNodes())
+        return { };
+
+    auto removedChildNodes = fragment.removeAllChildrenWithScriptAssertion(ContainerNode::ChildChange::Source::API);
+    nodesForInsertion.swap(removedChildNodes);
+
+    fragment.rebuildSVGExtensionsElementsIfNecessary();
+    fragment.dispatchSubtreeModifiedEvent();
+
     return { };
 }
 
@@ -393,13 +407,13 @@
     Ref<Node> next(*refChild);
 
     NodeVector targets;
-    auto removeResult = collectChildrenAndRemoveFromOldParent(newChild, targets);
+    auto removeResult = removeSelfOrChildNodesForInsertion(newChild, targets);
     if (removeResult.hasException())
         return removeResult.releaseException();
     if (targets.isEmpty())
         return { };
 
-    // We need this extra check because collectChildrenAndRemoveFromOldParent() can fire mutation events.
+    // We need this extra check because removeSelfOrChildNodesForInsertion() can fire mutation events.
     for (auto& child : targets) {
         auto checkAcceptResult = checkAcceptChildGuaranteedNodeTypes(*this, child);
         if (checkAcceptResult.hasException())
@@ -508,12 +522,12 @@
     NodeVector targets;
     {
         ChildListMutationScope mutation(*this);
-        auto collectResult = collectChildrenAndRemoveFromOldParent(newChild, targets);
+        auto collectResult = removeSelfOrChildNodesForInsertion(newChild, targets);
         if (collectResult.hasException())
             return collectResult.releaseException();
     }
 
-    // Do this one more time because collectChildrenAndRemoveFromOldParent() fires a MutationEvent.
+    // Do this one more time because removeSelfOrChildNodesForInsertion() fires a MutationEvent.
     for (auto& child : targets) {
         validityResult = checkPreReplacementValidity(*this, child, oldChild, ShouldValidateChildParent::No);
         if (validityResult.hasException())
@@ -707,7 +721,7 @@
     Ref<ContainerNode> protectedThis(*this);
 
     NodeVector targets;
-    auto removeResult = collectChildrenAndRemoveFromOldParent(newChild, targets);
+    auto removeResult = removeSelfOrChildNodesForInsertion(newChild, targets);
     if (removeResult.hasException())
         return removeResult.releaseException();
 
@@ -714,7 +728,7 @@
     if (targets.isEmpty())
         return { };
 
-    // We need this extra check because collectChildrenAndRemoveFromOldParent() can fire mutation events.
+    // We need this extra check because removeSelfOrChildNodesForInsertion() can fire mutation events.
     for (auto& child : targets) {
         auto nodeTypeResult = checkAcceptChildGuaranteedNodeTypes(*this, child);
         if (nodeTypeResult.hasException())
@@ -857,10 +871,6 @@
     if (child->isInShadowTree())
         return;
 
-    // FIXME: This doesn't belong in dispatchChildRemovalEvents.
-    // FIXME: Nodes removed from a shadow tree should also be kept alive.
-    willCreatePossiblyOrphanedTreeByRemoval(child.ptr());
-
     Ref<Document> document = child->document();
 
     // dispatch pre-removal mutation events

Modified: trunk/Source/WebCore/dom/ContainerNode.h (272393 => 272394)


--- trunk/Source/WebCore/dom/ContainerNode.h	2021-02-04 23:59:53 UTC (rev 272393)
+++ trunk/Source/WebCore/dom/ContainerNode.h	2021-02-05 00:10:30 UTC (rev 272394)
@@ -150,6 +150,7 @@
     enum class DeferChildrenChanged { Yes, No };
     NodeVector removeAllChildrenWithScriptAssertion(ChildChange::Source, DeferChildrenChanged = DeferChildrenChanged::No);
     bool removeNodeWithScriptAssertion(Node&, ChildChange::Source);
+    ExceptionOr<void> removeSelfOrChildNodesForInsertion(Node&, NodeVector&);
 
     void removeBetween(Node* previousChild, Node* nextChild, Node& oldChild);
     ExceptionOr<void> appendChildWithoutPreInsertionValidityCheck(Node&);

Modified: trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp (272393 => 272394)


--- trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp	2021-02-04 23:59:53 UTC (rev 272393)
+++ trunk/Source/WebCore/dom/ContainerNodeAlgorithms.cpp	2021-02-05 00:10:30 UTC (rev 272394)
@@ -107,49 +107,66 @@
     return postInsertionNotificationTargets;
 }
 
-static void notifyNodeRemovedFromDocument(ContainerNode& oldParentOfRemovedTree, TreeScopeChange treeScopeChange, Node& node)
+inline RemovedSubtreeObservability observabilityOfRemovedNode(Node& node)
 {
+    bool isRootOfRemovedTree = !node.parentNode();
+    return node.refCount() > 1 && !isRootOfRemovedTree ? RemovedSubtreeObservability::MaybeObservableByRefPtr : RemovedSubtreeObservability::NotObservable;
+}
+
+inline void updateObservability(RemovedSubtreeObservability& currentObservability, RemovedSubtreeObservability newStatus)
+{
+    if (newStatus == RemovedSubtreeObservability::MaybeObservableByRefPtr)
+        currentObservability = newStatus;
+}
+
+static RemovedSubtreeObservability notifyNodeRemovedFromDocument(ContainerNode& oldParentOfRemovedTree, TreeScopeChange treeScopeChange, Node& node)
+{
     ASSERT(oldParentOfRemovedTree.isConnected());
     ASSERT(node.isConnected());
     node.removedFromAncestor(Node::RemovalType { /* disconnectedFromDocument */ true, treeScopeChange == TreeScopeChange::Changed }, oldParentOfRemovedTree);
 
+    auto observability = observabilityOfRemovedNode(node);
     if (!is<ContainerNode>(node))
-        return;
+        return observability;
 
     for (RefPtr<Node> child = downcast<ContainerNode>(node).firstChild(); child; child = child->nextSibling()) {
         RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!node.isConnected() && child->parentNode() == &node);
-        notifyNodeRemovedFromDocument(oldParentOfRemovedTree, treeScopeChange, *child.get());
+        updateObservability(observability, notifyNodeRemovedFromDocument(oldParentOfRemovedTree, treeScopeChange, *child.get()));
     }
 
     if (!is<Element>(node))
-        return;
+        return observability;
 
     if (RefPtr<ShadowRoot> root = downcast<Element>(node).shadowRoot()) {
         RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!node.isConnected() && root->host() == &node);
-        notifyNodeRemovedFromDocument(oldParentOfRemovedTree, TreeScopeChange::DidNotChange, *root.get());
+        updateObservability(observability, notifyNodeRemovedFromDocument(oldParentOfRemovedTree, TreeScopeChange::DidNotChange, *root.get()));
     }
+    return observability;
 }
 
-static void notifyNodeRemovedFromTree(ContainerNode& oldParentOfRemovedTree, TreeScopeChange treeScopeChange, Node& node)
+static RemovedSubtreeObservability notifyNodeRemovedFromTree(ContainerNode& oldParentOfRemovedTree, TreeScopeChange treeScopeChange, Node& node)
 {
     ASSERT(!oldParentOfRemovedTree.isConnected());
 
     node.removedFromAncestor(Node::RemovalType { /* disconnectedFromDocument */ false, treeScopeChange == TreeScopeChange::Changed }, oldParentOfRemovedTree);
 
+    auto observability = observabilityOfRemovedNode(node);
     if (!is<ContainerNode>(node))
-        return;
+        return observability;
 
     for (RefPtr<Node> child = downcast<ContainerNode>(node).firstChild(); child; child = child->nextSibling())
-        notifyNodeRemovedFromTree(oldParentOfRemovedTree, treeScopeChange, *child);
+        updateObservability(observability, notifyNodeRemovedFromTree(oldParentOfRemovedTree, treeScopeChange, *child));
 
     if (!is<Element>(node))
-        return;
+        return observability;
 
     if (RefPtr<ShadowRoot> root = downcast<Element>(node).shadowRoot())
-        notifyNodeRemovedFromTree(oldParentOfRemovedTree, TreeScopeChange::DidNotChange, *root);
+        updateObservability(observability, notifyNodeRemovedFromTree(oldParentOfRemovedTree, TreeScopeChange::DidNotChange, *root));
+
+    return observability;
 }
 
-void notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node& child)
+RemovedSubtreeObservability notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node& child)
 {
     // Assert that the caller of this function has an instance of ScriptDisallowedScope.
     ASSERT(!isMainThread() || ScriptDisallowedScope::InMainThread::hasDisallowedScope());
@@ -158,9 +175,8 @@
     // Tree scope has changed if the container node from which "node" is removed is in a document or a shadow root.
     auto treeScopeChange = oldParentOfRemovedTree.isInTreeScope() ? TreeScopeChange::Changed : TreeScopeChange::DidNotChange;
     if (child.isConnected())
-        notifyNodeRemovedFromDocument(oldParentOfRemovedTree, treeScopeChange, child);
-    else
-        notifyNodeRemovedFromTree(oldParentOfRemovedTree, treeScopeChange, child);
+        return notifyNodeRemovedFromDocument(oldParentOfRemovedTree, treeScopeChange, child);
+    return notifyNodeRemovedFromTree(oldParentOfRemovedTree, treeScopeChange, child);
 }
 
 void addChildNodesToDeletionQueue(Node*& head, Node*& tail, ContainerNode& container)

Modified: trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h (272393 => 272394)


--- trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h	2021-02-04 23:59:53 UTC (rev 272393)
+++ trunk/Source/WebCore/dom/ContainerNodeAlgorithms.h	2021-02-05 00:10:30 UTC (rev 272394)
@@ -62,7 +62,11 @@
 #endif // not ASSERT_ENABLED
 
 NodeVector notifyChildNodeInserted(ContainerNode& parentOfInsertedTree, Node&);
-void notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node&);
+enum class RemovedSubtreeObservability {
+    NotObservable,
+    MaybeObservableByRefPtr,
+};
+RemovedSubtreeObservability notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node&);
 void removeDetachedChildrenInContainer(ContainerNode&);
 
 enum SubframeDisconnectPolicy {

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

Reply via email to