Share ElementRareData instances in shadows
Created attachment 177900 [details] Patch
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.
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.
Created attachment 178022 [details] Patch
(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!
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.
(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?
(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.
UncommonNodeData?
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
Created attachment 178044 [details] Patch
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?
Comment on attachment 178044 [details] Patch Clearing flags on attachment: 178044 Committed r136871: <http://trac.webkit.org/changeset/136871>
All reviewed patches have been landed. Closing bug.
(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?
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
Re-opened since this is blocked by bug 104293
(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.
(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.
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.
To clarify, I object to this change.
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.
(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.
(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.
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.
(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.
(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.
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.
(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?
So are we then going to have NodeRareDataBase2 next time? I am worried that this is not scalable.
Closing this since there doesn't seem to be support for this approach.