Bug 104202 – Create only NodeRareDataBase when setting TreeScope

• Component: New Bugs
• Status: RESOLVED INVALID

[Elliott Sprehn]

Share ElementRareData instances in shadows

[Elliott Sprehn]

Created attachment 177900 [details]
Patch

[Elliott Sprehn]

Unfortunately this patch isn't really much of a win unless we expect lots of things inside a ShadowRoot to be display: none because as soon as you set a renderer we need to create a unique rare data instance since that's where I put the renderer when I got rid of the rare data map in r133372.. doh!

I posted the patch just for reference of how this might work, or in case someone has a great idea.

[Elliott Sprehn]

Comment on attachment 177900 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:489
> +    m_data.m_rareData = const_cast<NodeRareData*>(data);

There should be a m_data.m_rareData->ref() here.

> Source/WebCore/dom/Node.cpp:508
> +        newRareData->setTreeScope(rareData()->treeScope());

There should be a rareData()->deref() here.

[Elliott Sprehn]

Created attachment 178022 [details]
Patch

[Elliott Sprehn]

(In reply to comment #4)
> Created an attachment (id=178022) [details]
> Patch

This approach seems much more promising. By only allocating NodeRareDataBase I've reduced the overhead per element from 136 bytes to 32 bytes for a 76% savings, and on other nodes from 64 bytes to 32 bytes for a 50% savings!

[Dimitri Glazkov (Google)]

Comment on attachment 178022 [details]
Patch

This is good, though it seems the naming could be tuned: "Base" vs. "Basic", and communicating the notion that the NodeRareDataBase is now actually a self-sufficient less rare flavor of NodeRareData.

[Elliott Sprehn]

(In reply to comment #6)
> (From update of attachment 178022 [details])
> This is good, though it seems the naming could be tuned: "Base" vs. "Basic", and communicating the notion that the NodeRareDataBase is now actually a self-sufficient less rare flavor of NodeRareData.

Sure, Have a recommendation for a better name?

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 178022 [details] [details])
> > This is good, though it seems the naming could be tuned: "Base" vs. "Basic", and communicating the notion that the NodeRareDataBase is now actually a self-sufficient less rare flavor of NodeRareData.
> 
> Sure, Have a recommendation for a better name?

NotSoRareNodeData? MediumWellNodeData? I dunno. Eric is good at this stuff, and so's Sam.

[Eric Seidel (no email)]

UncommonNodeData?

[Adam Barth]

Comment on attachment 178022 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        When putting nodes into ShadowRoot or any descdendant we must associate

typo: descdendant

[Elliott Sprehn]

Created attachment 178044 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 178044 [details]
Patch

I'm confused how this works. How do you graduate from uncommon to rare data? Do we thrash when setting certain properties? I assume its now two mallocs to end up with rare data in some cases?  The rare data replaces the uncommon data when needed?

[WebKit Review Bot]

Comment on attachment 178044 [details]
Patch

Clearing flags on attachment: 178044

Committed r136871: <http://trac.webkit.org/changeset/136871>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Elliott Sprehn]

(In reply to comment #12)
> (From update of attachment 178044 [details])
> I'm confused how this works. How do you graduate from uncommon to rare data? Do we thrash when setting certain properties? I assume its now two mallocs to end up with rare data in some cases?  The rare data replaces the uncommon data when needed?

We graduate it whenever you would have asked for a real rare data previously, so calling ensureElementRareData() or ensureRareData() will force an allocation and a free of the UncommonNodeData if it's present. This is because hasRareData() will return false if you only have an UncommonRareData.

It would thrash if you did setTreeScope() and then called ensureRareData() right after it.

An example of causing this thrashing:

div = shadowRoot.appendChild(document.createElement('div'));
div.childNodes // causes promotion to ElementRareData from UncommonNodeData.

It would also seem that anything that sets the style bits could cause this kind of thrashing (ex. m_childrenAffectedByFirstChildRules so if there's div:first-child then we'd also thrash here).

We can move the style bit methods to UncommonNodeData and make the promotion process copy them to the NodeRareData instance when it's created which solves this... if you think it's necessary?

[Emil A Eklund]

This change caused quite a few tests to trigger an assertion, 

STDERR: ASSERTION FAILED: hasRareData()
STDERR: third_party/WebKit/Source/WebCore/dom/Node.cpp(484) : WebCore::NodeRareData* WebCore::Node::rareData() const

accessibility/aria-checkbox-checked.html
accessibility/aria-labelledby-on-input.html
accessibility/aria-roles.html
accessibility/canvas-accessibilitynodeobject.html
accessibility/canvas-fallback-content-2.html
accessibility/canvas-fallback-content.html
accessibility/container-node-delete-causes-crash.html
accessibility/disabled-controls-not-focusable.html
accessibility/file-upload-button-with-axpress.html
accessibility/input-file-causes-crash.html
accessibility/input-image-alt.html
accessibility/media-controls.html
accessibility/nochildren-elements.html
accessibility/radio-button-title-label.html
accessibility/secure-textfield-title-ui.html
accessibility/textbox-role-on-contenteditable-crash.html
accessibility/th-as-title-ui.html
compositing/geometry/clipped-video-controller.html
compositing/overflow/theme-affects-visual-overflow.html
compositing/video/video-controls-layer-creation.html
css1/box_properties/acid_test.html
css2.1/t09-c5526c-display-00-e.html
css3/flexbox/flexitem-stretch-range.html
css3/flexbox/flexitem.html
css3/selectors3/html/css3-modsel-25.html
css3/selectors3/html/css3-modsel-70.html
css3/selectors3/xhtml/css3-modsel-25.xml
css3/selectors3/xhtml/css3-modsel-70.xml
css3/selectors3/xml/css3-modsel-25.xml
css3/selectors3/xml/css3-modsel-70.xml
dom/html/level2/html/HTMLBodyElement07.html
dom/html/level2/html/HTMLBodyElement08.html
dom/html/level2/html/HTMLBodyElement09.html
dom/html/level2/html/HTMLBodyElement10.html
dom/html/level2/html/HTMLBodyElement11.html
dom/html/level2/html/HTMLBodyElement12.html
dom/html/level2/html/HTMLDocument01.html
dom/html/level2/html/HTMLDocument02.html
dom/html/level2/html/HTMLDocument03.html
dom/html/level2/html/HTMLDocument04.html
dom/html/level2/html/HTMLDocument05.html
dom/html/level2/html/HTMLDocument07.html
dom/html/level2/html/HTMLDocument08.html
dom/html/level2/html/HTMLDocument09.html
dom/xhtml/level2/html/HTMLBodyElement07.xhtml
dom/xhtml/level2/html/HTMLBodyElement08.xhtml
dom/xhtml/level2/html/HTMLBodyElement09.xhtml
dom/xhtml/level2/html/HTMLBodyElement10.xhtml
dom/xhtml/level2/html/HTMLBodyElement11.xhtml
dom/xhtml/level2/html/HTMLBodyElement12.xhtml
editing/deleting/5290534.html
editing/input/div-first-child-rule-input.html
editing/input/div-first-child-rule-textarea.html
editing/input/ime-composition-clearpreedit.html
editing/input/password-echo-passnode.html
editing/input/password-echo-passnode2.html
editing/input/password-echo-passnode3.html
editing/input/paste-linebreak-into-initially-hidden-textarea.html
editing/input/paste-text-ending-with-interchange-newline.html
http/tests/appcache/destroyed-frame.html
http/tests/appcache/video.html
http/tests/cache/post-redirect-get.php
http/tests/cache/post-with-cached-subresources.php
http/tests/cache/subresource-failover-to-network.html
http/tests/history/back-to-post.php
http/tests/loading/307-after-303-after-post.html
http/tests/loading/redirect-methods.html
http/tests/local/blob/send-hybrid-blob.html
http/tests/local/fileapi/file-last-modified-after-delete.html
http/tests/local/fileapi/file-last-modified.html
http/tests/local/fileapi/send-dragged-file.html
http/tests/local/fileapi/send-sliced-dragged-file.html
http/tests/local/formdata/form-data-with-unknown-file-extension.html
http/tests/local/formdata/send-form-data-constructed-from-form.html
http/tests/local/formdata/send-form-data-with-sliced-file.html
http/tests/local/formdata/send-form-data.html
http/tests/local/formdata/upload-events.html
http/tests/media/media-document.html
http/tests/media/reload-after-dialog.html
http/tests/navigation/anchor-basic.html
http/tests/navigation/anchor-goback.html
http/tests/navigation/anchor-subframeload.html
http/tests/navigation/dynamic-iframe-dynamic-form-back-entry.html
http/tests/navigation/history-back-across-form-submission-to-fragment.html
http/tests/navigation/javascriptlink-basic.html
http/tests/navigation/javascriptlink-frames.html
http/tests/navigation/javascriptlink-goback.html
http/tests/navigation/javascriptlink-subframeload.html
http/tests/navigation/metaredirect-basic.html
http/tests/navigation/metaredirect-frames.html
http/tests/navigation/metaredirect-goback.html
http/tests/navigation/metaredirect-subframeload.html
http/tests/navigation/new-window-redirect-history.html
http/tests/navigation/parsed-iframe-dynamic-form-back-entry.html
http/tests/navigation/post-302-response.html
http/tests/navigation/post-basic.html
http/tests/navigation/post-frames.html
http/tests/navigation/post-goback-same-url.html
http/tests/navigation/post-goback1.html
http/tests/navigation/post-goback2.html

[WebKit Review Bot]

Re-opened since this is blocked by bug 104293

[Elliott Sprehn]

(In reply to comment #17)
> Re-opened since this is blocked by bug 104293

(In reply to comment #17)
> Re-opened since this is blocked by bug 104293

Woops, code worked fine because if hasUncommonNodeData() is true then rareData()->setTreeScope will work since UncommonNodeData is a superclass of NodeRareData, but triggers the assert since we don't really have rare data.

Fix is just to call m_data.m_rareData->setTreeScope instead.

[Emil A Eklund]

(In reply to comment #18)
> Woops, code worked fine because if hasUncommonNodeData() is true then rareData()->setTreeScope will work since UncommonNodeData is a superclass of NodeRareData, but triggers the assert since we don't really have rare data.
> 
> Fix is just to call m_data.m_rareData->setTreeScope instead.

Seems like it should do the trick but you should have the original reviewer take another look once you've uploaded a new patch. Thanks.

[Ryosuke Niwa]

I don’t think this whole business about introducing yet another object is a good idea. If NodeRareData is not rare, then we need to fix that.

The whole tree-scope-being-too-common thing seems dubious to me. If we made a bad design decision in the past, then we need to revise that design decision instead of working around it by introducing more crafts.

The fact that UncommonNodeData is created for every element in the shadow tree (due to it having a tree scope) means that it’s not uncommon.

[Ryosuke Niwa]

To clarify, I object to this change.

[Ryosuke Niwa]

Here's an alternative suggestion I have:
1. Move ChildNodeList back into NodeListsNodeData
2. Move m_mutationObserverRegistry & m_transientMutationObserverRegistry into a separate object owned by NodeRareData
3. Move m_itemProp, m_itemRef, m_itemType & into a separate object owned by NodeRareData

That'll make NodeRareData substantially smaller. In fact, sizeof(NodeRareData) will be sizeof(UncommonNodeData) + sizeof(void*) once this is done.

[Elliott Sprehn]

(In reply to comment #22)
> Here's an alternative suggestion I have:
> 1. Move ChildNodeList back into NodeListsNodeData
> 2. Move m_mutationObserverRegistry & m_transientMutationObserverRegistry into a separate object owned by NodeRareData
> 3. Move m_itemProp, m_itemRef, m_itemType & into a separate object owned by NodeRareData
> 
> That'll make NodeRareData substantially smaller. In fact, sizeof(NodeRareData) will be sizeof(UncommonNodeData) + sizeof(void*) once this is done.

This is the wrong comparison because ElementRareData is massive. It doesn't matter that NodeRareData is small, we still get an ElementRareData when setting the tree scope.

[Ryosuke Niwa]

(In reply to comment #23)
> (In reply to comment #22)
> > Here's an alternative suggestion I have:
> > 1. Move ChildNodeList back into NodeListsNodeData
> > 2. Move m_mutationObserverRegistry & m_transientMutationObserverRegistry into a separate object owned by NodeRareData
> > 3. Move m_itemProp, m_itemRef, m_itemType & into a separate object owned by NodeRareData
> > 
> > That'll make NodeRareData substantially smaller. In fact, sizeof(NodeRareData) will be sizeof(UncommonNodeData) + sizeof(void*) once this is done.
> 
> This is the wrong comparison because ElementRareData is massive. It doesn't matter that NodeRareData is small, we still get an ElementRareData when setting the tree scope.

Then keep NodeRareData & ElementRareData dynamically swappable. That's still significantly better design than introducing yet another base class.

[Ryosuke Niwa]

The approach taken in https://bugs.webkit.org/show_bug.cgi?id=59816 is much saner.

Also see https://bugs.webkit.org/show_bug.cgi?id=87034 and https://bugs.webkit.org/show_bug.cgi?id=89635.

[Elliott Sprehn]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Here's an alternative suggestion I have:
> > > 1. Move ChildNodeList back into NodeListsNodeData
> > > 2. Move m_mutationObserverRegistry & m_transientMutationObserverRegistry into a separate object owned by NodeRareData
> > > 3. Move m_itemProp, m_itemRef, m_itemType & into a separate object owned by NodeRareData
> > > 
> > > That'll make NodeRareData substantially smaller. In fact, sizeof(NodeRareData) will be sizeof(UncommonNodeData) + sizeof(void*) once this is done.
> > 
> > This is the wrong comparison because ElementRareData is massive. It doesn't matter that NodeRareData is small, we still get an ElementRareData when setting the tree scope.
> 
> Then keep NodeRareData & ElementRareData dynamically swappable. That's still significantly better design than introducing yet another base class.

I don't understand how that's better, it still requires swapping logic except it's much more complex since we need to copy all the bits between the two things. It also means any patch that adds things to NodeRareData will be a memory regression on Shadow DOM using pages.

[Ryosuke Niwa]

(In reply to comment #26)
> I don't understand how that's better, it still requires swapping logic except it's much more complex since we need to copy all the bits between the two things. It also means any patch that adds things to NodeRareData will be a memory regression on Shadow DOM using pages.

Yet, shadow DOM is still rare on the Web today. I don't think we should be optimizing WebKit for content that doesn't exist yet.

[Dimitri Glazkov (Google)]

This is a fascinating discussion. It looks like Ryosuke provided some ideas, though none seem able to bear any fruit.

In the larger context, I am interested in your thoughts on a reasonable path to success. When we should start optimizing Shadow DOM performance/memory use? If we do it after it starts being used in the wild, we're too late. If we do it before then, it appears that we're too early.

[Ryosuke Niwa]

(In reply to comment #28)
> This is a fascinating discussion. It looks like Ryosuke provided some ideas, though none seem able to bear any fruit.
> 
> In the larger context, I am interested in your thoughts on a reasonable path to success. When we should start optimizing Shadow DOM performance/memory use? If we do it after it starts being used in the wild, we're too late.

Why is that too late? How do we figure out what kind of function calls & objects are common if nobody had started using them?

[Darin Adler]

So are we then going to have NodeRareDataBase2 next time? I am worried that this is not scalable.

[Elliott Sprehn]

Closing this since there doesn't seem to be support for this approach.