Move TreeScope pointer to ContainerNode

• Component: DOM
• Status: NEW ---

[Antti Koivisto]

We can make non-ContainerNodes (Text mostly) smaller by moving the tree scope pointer from Node to ContainerNode. Non-containers can get it from the parent (and they don't need it much anyway). This should be a significant memory win.

[Antti Koivisto]

Created attachment 182882 [details]
possible patch

[Early Warning System Bot]

Comment on attachment 182882 [details]
possible patch

Attachment 182882 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15884795

[Early Warning System Bot]

Comment on attachment 182882 [details]
possible patch

Attachment 182882 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15901195

[Ryosuke Niwa]

Comment on attachment 182882 [details]
possible patch

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

> Source/WebCore/dom/Node.h:810
> +    union {
> +        ContainerNode* m_parentOrHostNode;
> +        TreeScope* m_treeScopeForParentlessNonContainer;
> +    };

It's very unfortunate that we have to use union for such an edge case :(

[WebKit Review Bot]

Comment on attachment 182882 [details]
possible patch

Attachment 182882 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15902183

[Elliott Sprehn]

Comment on attachment 182882 [details]
possible patch

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

I object to this change, it puts a branch into all the methods that go through the document pointer in the render tree (see RenderObject.h) which we know is a perf regression from the last time moritta tried replacing the document pointer. This also seems like a lot of complexity.

> Source/WebCore/dom/ContainerNode.h:278
> +    return toContainerNode(this)->treeScope();

This is going to be a perf regression since you're inserting a branch into all node->document() access which means a branch into lots of methods in the render tree.

[Elliott Sprehn]

(In reply to comment #0)
> We can make non-ContainerNodes (Text mostly) smaller by moving the tree scope pointer from Node to ContainerNode. Non-containers can get it from the parent (and they don't need it much anyway).

Note also that "they don't need it much anyway" is not really true since document() goes through m_treeScope and CharacterData.cpp uses document() a bunch; Text.cpp also uses it in the critical path for recalcTextStyle and RenderText.cpp also uses document() in a bunch of performance sensitive places.

[Peter Beverloo (cr-android ews)]

Comment on attachment 182882 [details]
possible patch

Attachment 182882 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15904205

[EFL EWS Bot]

Comment on attachment 182882 [details]
possible patch

Attachment 182882 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15910182

[Antti Koivisto]

(In reply to comment #4)
> (From update of attachment 182882 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182882&action=review
> 
> > Source/WebCore/dom/Node.h:810
> > +    union {
> > +        ContainerNode* m_parentOrHostNode;
> > +        TreeScope* m_treeScopeForParentlessNonContainer;
> > +    };
> 
> It's very unfortunate that we have to use union for such an edge case :(

Yeah. It is double silly since both TreeScope types are actually ContainerNodes. 

Maybe the case is rare enough to shove the tree scope to a map. The case is easy enough to hit by by trying though

javascript:alert(document.createTextNode().ownerDocument)

[Antti Koivisto]

(In reply to comment #6)
> I object to this change, it puts a branch into all the methods that go through the document pointer in the render tree (see RenderObject.h) which we know is a perf regression from the last time moritta tried replacing the document pointer. This also seems like a lot of complexity.

There is no branch if you operate on ContainerNodes. Looks like the new branchiness in the render tree could be largely eliminated by simple tightening (non-RenderTexts always have Element as their node())

Note that significant memory reductions can be performance progressions too in by themselves due to improved locality. I don't know how this is going to be without testing.

[Antti Koivisto]

Actually I can probably do this cleaner by enuming with the renderer pointer. The edge case never has a renderer.

[Darin Adler]

(In reply to comment #12)
> Actually I can probably do this cleaner by enuming with the renderer pointer. The edge case never has a renderer.

Where you say "enuming" you mean "unioning", right?

[Antti Koivisto]

(In reply to comment #13)
> Where you say "enuming" you mean "unioning", right?

Yes!