WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED INVALID
104202
Create only NodeRareDataBase when setting TreeScope
https://bugs.webkit.org/show_bug.cgi?id=104202
Summary
Create only NodeRareDataBase when setting TreeScope
Elliott Sprehn
Reported
2012-12-05 18:15:26 PST
Share ElementRareData instances in shadows
Attachments
Patch
(24.45 KB, patch)
2012-12-05 18:20 PST
,
Elliott Sprehn
no flags
Details
Formatted Diff
Diff
Patch
(13.69 KB, patch)
2012-12-06 10:01 PST
,
Elliott Sprehn
no flags
Details
Formatted Diff
Diff
Patch
(14.27 KB, patch)
2012-12-06 11:31 PST
,
Elliott Sprehn
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Elliott Sprehn
Comment 1
2012-12-05 18:20:48 PST
Created
attachment 177900
[details]
Patch
Elliott Sprehn
Comment 2
2012-12-05 18:24:31 PST
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 3
2012-12-05 18:28:28 PST
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
Comment 4
2012-12-06 10:01:14 PST
Created
attachment 178022
[details]
Patch
Elliott Sprehn
Comment 5
2012-12-06 10:03:08 PST
(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 6
2012-12-06 10:33:42 PST
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
Comment 7
2012-12-06 10:39:59 PST
(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)
Comment 8
2012-12-06 11:05:09 PST
(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)
Comment 9
2012-12-06 11:09:35 PST
UncommonNodeData?
Adam Barth
Comment 10
2012-12-06 11:25:31 PST
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
Comment 11
2012-12-06 11:31:36 PST
Created
attachment 178044
[details]
Patch
Eric Seidel (no email)
Comment 12
2012-12-06 12:04:02 PST
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 13
2012-12-06 12:17:17 PST
Comment on
attachment 178044
[details]
Patch Clearing flags on attachment: 178044 Committed
r136871
: <
http://trac.webkit.org/changeset/136871
>
WebKit Review Bot
Comment 14
2012-12-06 12:17:21 PST
All reviewed patches have been landed. Closing bug.
Elliott Sprehn
Comment 15
2012-12-06 12:42:29 PST
(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
Comment 16
2012-12-06 13:34:29 PST
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
Comment 17
2012-12-06 13:38:41 PST
Re-opened since this is blocked by
bug 104293
Elliott Sprehn
Comment 18
2012-12-06 13:45:21 PST
(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
Comment 19
2012-12-06 13:50:39 PST
(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
Comment 20
2012-12-06 13:51:51 PST
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
Comment 21
2012-12-06 14:09:39 PST
To clarify, I object to this change.
Ryosuke Niwa
Comment 22
2012-12-06 14:19:06 PST
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
Comment 23
2012-12-06 14:30:19 PST
(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
Comment 24
2012-12-06 14:31:58 PST
(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
Comment 25
2012-12-06 14:48:05 PST
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
Comment 26
2012-12-06 14:48:30 PST
(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
Comment 27
2012-12-06 15:00:09 PST
(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)
Comment 28
2012-12-06 15:32:51 PST
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
Comment 29
2012-12-06 15:44:11 PST
(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
Comment 30
2012-12-06 21:24:54 PST
So are we then going to have NodeRareDataBase2 next time? I am worried that this is not scalable.
Elliott Sprehn
Comment 31
2012-12-17 13:02:28 PST
Closing this since there doesn't seem to be support for this approach.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug