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.
Created attachment 182882 [details] possible patch
Comment on attachment 182882 [details] possible patch Attachment 182882 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15884795
Comment on attachment 182882 [details] possible patch Attachment 182882 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15901195
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 :(
Comment on attachment 182882 [details] possible patch Attachment 182882 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15902183
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.
(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.
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
Comment on attachment 182882 [details] possible patch Attachment 182882 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15910182
(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)
(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.
Actually I can probably do this cleaner by enuming with the renderer pointer. The edge case never has a renderer.
(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?
(In reply to comment #13) > Where you say "enuming" you mean "unioning", right? Yes!