Bug 221243 – Avoid creating JS wrapper on a removed node when the subtree is not observable

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

We should avoid force creating JS wrappers for the root node of a removed subtree
when the subtree is not otherwise observable.

[Ryosuke Niwa]

Created attachment 418955 [details]
WIP

[Ryosuke Niwa]

<rdar://problem/73719386>

[Ryosuke Niwa]

Created attachment 419109 [details]
Patch

[Geoffrey Garen]

Comment on attachment 419109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419109&action=review

> Source/WebCore/ChangeLog:18
> +        it has a significant runtime and memory cost - the latter is because we never collect the JS wrappers of
> +        DOM nodes once they're created.

Surely it's not *never*?

> Source/WebCore/ChangeLog:21
> +        won't be observable by scripts in the future. The current heuristics is to check if the removed node

heuristic

> Source/WebCore/ChangeLog:24
> +        into existence, which increments the reference counnt. The two notable exceptions are StaticRange and

count

I think the explanation for why this approach is sufficient need not be tied to any statement about JS wrappers. The explanation is simply that "if there is no reference count, then there is no external reference" -- because those are the rules of reference counting.

> Source/WebCore/ChangeLog:39
> +        condition is refCount() > 2 because this function collects the list of child nodes to be removed in
> +        Vector<Ref<Node>> and the removeBetween loop itself stores it again in a RefPtr, the latter of which is

The refCount() > 2 comment probably belongs inside the function. That's a little subtle.

> Source/WebCore/dom/ContainerNode.cpp:116
> +    bool hasChildListChanged = false;
>      while (RefPtr<Node> child = m_firstChild) {
> +        if (i >= children.size() || children[i++].ptr() != child)
> +            hasChildListChanged = true;

I don't understand why we care if children have changed.

If a child has been removed, that is not an external reference to this subtree.

If a child has been added, that is not an external reference to this subtree unless the child has an external reference, which the refcount check will cover.

If a child has been moved, that is just a removal followed by an addition, so it is covered by the logic above.

[Geoffrey Garen]

(If we do have some reason to care about children changing, then we need to think a little bit about the ABA case where children change, and then change back. Our hasChildListChanged would not be able to detect that case.)

[Geoffrey Garen]

Comment on attachment 419109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419109&action=review

>> Source/WebCore/dom/ContainerNode.cpp:116
>> +            hasChildListChanged = true;
> 
> I don't understand why we care if children have changed.
> 
> If a child has been removed, that is not an external reference to this subtree.
> 
> If a child has been added, that is not an external reference to this subtree unless the child has an external reference, which the refcount check will cover.
> 
> If a child has been moved, that is just a removal followed by an addition, so it is covered by the logic above.

I guess the issue here is that, if an event fired above, and then added a node to the subtree, we don't know what refCount check to do. Nodes in the children set should be checked for > 2, and other nodes should be checked for > 1, and we don't have an obvious way to tell the difference.

[Geoffrey Garen]

Comment on attachment 419109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419109&action=review

r=me

>>> Source/WebCore/dom/ContainerNode.cpp:116
>>> +            hasChildListChanged = true;
>> 
>> I don't understand why we care if children have changed.
>> 
>> If a child has been removed, that is not an external reference to this subtree.
>> 
>> If a child has been added, that is not an external reference to this subtree unless the child has an external reference, which the refcount check will cover.
>> 
>> If a child has been moved, that is just a removal followed by an addition, so it is covered by the logic above.
> 
> I guess the issue here is that, if an event fired above, and then added a node to the subtree, we don't know what refCount check to do. Nodes in the children set should be checked for > 2, and other nodes should be checked for > 1, and we don't have an obvious way to tell the difference.

^ I guess this is worth a comment next to hasChildListChanged; but I can't think of an obviously better alternative.

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 419109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419109&action=review
> 
> r=me
> 
> >>> Source/WebCore/dom/ContainerNode.cpp:116
> >>> +            hasChildListChanged = true;
> >> 
> >> I don't understand why we care if children have changed.
> >> 
> >> If a child has been removed, that is not an external reference to this subtree.
> >> 
> >> If a child has been added, that is not an external reference to this subtree unless the child has an external reference, which the refcount check will cover.
> >> 
> >> If a child has been moved, that is just a removal followed by an addition, so it is covered by the logic above.
> > 
> > I guess the issue here is that, if an event fired above, and then added a node to the subtree, we don't know what refCount check to do. Nodes in the children set should be checked for > 2, and other nodes should be checked for > 1, and we don't have an obvious way to tell the difference.
> 
> ^ I guess this is worth a comment next to hasChildListChanged; but I can't
> think of an obviously better alternative.

Turns out that we don't actually need this check after all.

[Ryosuke Niwa]

Created attachment 419226 [details]
Patch

[Ryosuke Niwa]

Created attachment 419228 [details]
Patch

[Geoffrey Garen]

Comment on attachment 419228 [details]
Patch

So much better! :P

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 419228 [details]
> Patch
> 
> So much better! :P

Indeed!

[EWS]

commit-queue failed to commit attachment 419228 [details] to WebKit repository. To retry, please set cq+ flag again.

[Ryosuke Niwa]

Comment on attachment 419228 [details]
Patch

Clearing flags on attachment: 419228

Committed r272394: <https://trac.webkit.org/changeset/272394>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.