Bug 215673 – Make it possible to create a WeakPtr to Node and use it store assigned nodes in SlotAssignment

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

[Ryosuke Niwa]

We should be able to make WeakPtr to Node. Deploy that in SlotAssignment.

[Ryosuke Niwa]

Created attachment 406891 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/67440818>

[Geoffrey Garen]

Is it OK to make all Nodes bigger? Or should we consider putting the WeakPtrFactory in RareData?

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #3)
> Is it OK to make all Nodes bigger? Or should we consider putting the
> WeakPtrFactory in RareData?

I don't think putting in RareData is a good idea because ElementRareData is pretty big and I doubt that WeakPtr would stay rare. I'm running memory tests with this change.

I don't think the size of Node really matters these days because most of memory usage is JS heap. But if we really wanted, we can share it with NodeRareData pointer in Node. So basically, we can have a single pointer either point to WeakReference or NodeRareData/ElementRareData. When we create NodeRareData, we can move it inside.

[Ryosuke Niwa]

Created attachment 407257 [details]
Patch

[Ryosuke Niwa]

Our internal perf tests seem to show that this patch is basically perf & memory neutral.

[Darin Adler]

I’m surprised that making all text nodes bigger is memory neutral!

[Darin Adler]

Comment on attachment 407257 [details]
Patch

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

I’m going to say review+ but I am surprised that we can afford this much extra memory for every text node.

If we can afford this extra memory, then it seems like we can cut down the code size and complexity of accessibility code by taking advantage. AXObjectCache::remove has a lot of code in it to respond to the deletion of a node. Needs to remove it from each of many separate data structures have nodes as keys or values. AccessibilityNodeObject::m_node is a raw pointer that is nulled out by a hand-written WeakPtr implementation in a function named "detachRemoteParts".

> Source/WebCore/html/HTMLSlotElement.cpp:122
> +    for (const WeakPtr<Node>& nodePtr : *assignedNodes) {

auto&

> Source/WebCore/html/HTMLSlotElement.cpp:125
> +            ASSERT_NOT_REACHED();

Why can we assert this?

> Source/WebCore/html/HTMLSlotElement.cpp:157
> -    return assignedNodes->map([] (Node* node) { return makeRef(*node); });
> +
> +    Vector<Ref<Node>> nodes;
> +    nodes.reserveInitialCapacity(assignedNodes->size());
> +    for (auto& nodePtr : *assignedNodes) {
> +        auto* node = nodePtr.get();
> +        if (UNLIKELY(!node))
> +            continue;
> +        nodes.uncheckedAppend(*node);
> +    }
> +
> +    return nodes;

I’d like us to create a Vector::map variant that allows removal of the nodes during the mapping process. I included this feature in makeVector, although the design isn’t super-elegant, and have found it to often be useful.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #8)
> Comment on attachment 407257 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407257&action=review
> 
> I’m going to say review+ but I am surprised that we can afford this much
> extra memory for every text node.

I'm sure it's using slightly more memory but these days JS heap & IO surfaces account for a much larger proportion of the memory.

> If we can afford this extra memory, then it seems like we can cut down the
> code size and complexity of accessibility code by taking advantage.
> AXObjectCache::remove has a lot of code in it to respond to the deletion of
> a node. Needs to remove it from each of many separate data structures have
> nodes as keys or values. AccessibilityNodeObject::m_node is a raw pointer
> that is nulled out by a hand-written WeakPtr implementation in a function
> named "detachRemoteParts".

Yeah, there is a lot of code that could be simplified once this patch lands.

> > Source/WebCore/html/HTMLSlotElement.cpp:122
> > +    for (const WeakPtr<Node>& nodePtr : *assignedNodes) {
> 
> auto&

Hm... I explicitly wanted to communicate the fact it's a weak pointer. I'd use auto& and rename the variable to nodeWeakPtr then.

> > Source/WebCore/html/HTMLSlotElement.cpp:125
> > +            ASSERT_NOT_REACHED();
> 
> Why can we assert this?

In theory, this should never happen. This assertion being hit would mean that we have a bug somewhere, which I don't believe it exist today.

> > Source/WebCore/html/HTMLSlotElement.cpp:157
> > -    return assignedNodes->map([] (Node* node) { return makeRef(*node); });
> > +
> > +    Vector<Ref<Node>> nodes;
> > +    nodes.reserveInitialCapacity(assignedNodes->size());
> > +    for (auto& nodePtr : *assignedNodes) {
> > +        auto* node = nodePtr.get();
> > +        if (UNLIKELY(!node))
> > +            continue;
> > +        nodes.uncheckedAppend(*node);
> > +    }
> > +
> > +    return nodes;
> 
> I’d like us to create a Vector::map variant that allows removal of the nodes
> during the mapping process. I included this feature in makeVector, although
> the design isn’t super-elegant, and have found it to often be useful.

Yes, my thoughts exactly. I didn't do it in this patch because I wanted to keep it easily revertible in the case it still causes a perf regression somewhere but I'd go work on that on a separate patch.

[Ryosuke Niwa]

Committed r266212: <https://trac.webkit.org/changeset/266212>