Replace the Document* in Node with a TreeScope*. This removes the need for NodeRareData in all shadow tree nodes, while access to the Document is still O(1) via each node's TreeScope. This does, however, require that nodes that are associated with a document but not in the tree are put in a special TreeScope. Likewise DocumentFragments would need to become TreeScopes.
This is not specific to the content element.
Starting to work on this.
Created attachment 121810 [details] Patch
Comment on attachment 121810 [details] Patch Attachment 121810 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11166919
Comment on attachment 121810 [details] Patch Attachment 121810 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11172381
View in context: https://bugs.webkit.org/attachment.cgi?id=121810&action=review I like this patch very much - two big thumbs up! Just some minor nits inline. > Source/WebCore/dom/Document.h:1490 > +inline bool TreeScope::isDocumentScope() const Can't this be moved to TreeScope.h? > Source/WebCore/dom/TreeScope.h:42 > + Document* rootTreeScope() const { return m_rootTreeScope; } Why not simply name it 'document()' and 'm_document', respectively?
Comment on attachment 121810 [details] Patch Attachment 121810 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11183917
Created attachment 121818 [details] Patch
Hi Roland, thanks for taking a look and supporting this! I just updated the patch to address your points. > > Source/WebCore/dom/Document.h:1490 > > +inline bool TreeScope::isDocumentScope() const > > Can't this be moved to TreeScope.h? Sure. Done. > > > Source/WebCore/dom/TreeScope.h:42 > > + Document* rootTreeScope() const { return m_rootTreeScope; } > > Why not simply name it 'document()' and 'm_document', respectively? Sounds reasonable. Initially I thought it hide Node::document() but actually there is no harm in practice.
Comment on attachment 121818 [details] Patch Attachment 121818 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11186933
Comment on attachment 121818 [details] Patch Attachment 121818 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11186937
Comment on attachment 121818 [details] Patch Attachment 121818 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11197001
Comment on attachment 121818 [details] Patch Attachment 121818 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11184994
Comment on attachment 121818 [details] Patch This patch may have performance implications, right? If so, it needs to be tested for perf regressions.
(In reply to comment #14) > (From update of attachment 121818 [details]) > This patch may have performance implications, right? If so, it needs to be tested for perf regressions. Fair enough. I'll try - and I should update the patch to make the bots happy anyway...
Created attachment 121969 [details] Just for examining ews
Comment on attachment 121969 [details] Just for examining ews Since Morrita says this is for EWS only, clearing review flag.
@morita: You can send a patch to the EWS without marking it for review. If you don't mark the patch for review, there's a button you can click (where the green and red bubbles usually are) that sends it to the EWS.
@darin, @abarth. Ah, sure. I should have done so. Thanks for your advice!
Created attachment 126707 [details] Patch
Created attachment 134547 [details] Patch
I couldn't resolve performance regression and give this up. Will try another idea.
Reopening to attach new patch.
Created attachment 147249 [details] Updated to ToT
Created attachment 149707 [details] WIP: Naive version
Created attachment 149709 [details] WIP: Naive version
Created attachment 150106 [details] WIP: with a few peephole optimizations
Created attachment 150138 [details] WIP: Almost working
Created attachment 150872 [details] Yet another trial with a tag bit
Created attachment 150879 [details] Patch
Comment on attachment 150879 [details] Patch Attachment 150879 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13139736
Comment on attachment 150879 [details] Patch Attachment 150879 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13144040
Comment on attachment 150879 [details] Patch Attachment 150879 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13141113
Comment on attachment 150879 [details] Patch Attachment 150879 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13151004
Created attachment 151386 [details] Patch
Comment on attachment 151386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151386&action=review Nice patch! :) Added some general comments on TaggedPtr. > Source/WTF/wtf/TaggedPtr.h:26 > + Given this is in a general library, I think this needs introductory documentation on what it does and why it's useful. > Source/WTF/wtf/TaggedPtr.h:39 > + explicit TaggedPtr(T* ptr, bool tag = false) { set(ptr, tag); } With Tag = 1, this pointer cannot point to odd addresses - that means, it cannot be used for chars at all, nor for other stuff that is byte-aligned (e.g., #pragma pack) Now, I don't think this is necessarily a fatal issue, but it should be documented and/or - even better - prevented by template magic. Additionally, this needs to answer the question whether NULL can be tagged. If not, it should be documented and prevented. If yes, then IMHO this class should really provide operator! and conversion to UnspecifiedBoolType (see RefPtr.h) that takes the tag into consideration. (Personally I'd add the latter methods in any case, since it's generally useful.) > Source/WTF/wtf/TaggedPtr.h:68 > + Adding setTagged/setUntagged versions might be nice, to remove the check when the tag state is known a-priori.
Comment on attachment 151386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151386&action=review Thanks for the feedback, Roland! >> Source/WTF/wtf/TaggedPtr.h:26 >> + > > Given this is in a general library, I think this needs introductory documentation on what it does and why it's useful. Well, I don't want people to use this more. This is necessary evil without which we cannot avoid the performance regression in this specific change. It prevent debugger to inspect the pointer. So this unfriendliness is kinda intentional. >> Source/WTF/wtf/TaggedPtr.h:39 >> + explicit TaggedPtr(T* ptr, bool tag = false) { set(ptr, tag); } > > With Tag = 1, this pointer cannot point to odd addresses - that means, it cannot be used for chars at all, nor for other stuff that is byte-aligned (e.g., #pragma pack) Now, I don't think this is necessarily a fatal issue, but it should be documented and/or - even better - prevented by template magic. > > Additionally, this needs to answer the question whether NULL can be tagged. If not, it should be documented and prevented. If yes, then IMHO this class should really provide operator! and conversion to UnspecifiedBoolType (see RefPtr.h) that takes the tag into consideration. (Personally I'd add the latter methods in any case, since it's generally useful.) The alignment of the address is checked in ASSERT() in set(). I have no idea how to catch it in the compilation time. The value allows to be NULL. Since client cannot refer the raw value, additional operator seems a bit overkill. We can just use get(). Again, I don't want this to be used broadly. And helping easier use doesn't match that intention. >> Source/WTF/wtf/TaggedPtr.h:68 >> + > > Adding setTagged/setUntagged versions might be nice, to remove the check when the tag state is known a-priori. Currently no one needs it. So it can be done later. Maybe I should remove the default parameter. It's misleading.
Comment on attachment 151386 [details] Patch r-. We don't want bits stuffed into pointers. This breaks lots of tools.
Comment on attachment 151386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151386&action=review > Source/WebCore/ChangeLog:12 > + - In shadow tree, Each node has its tree scope in ElementRareData, Nit: Each should not be capitalized. > Source/WebCore/ChangeLog:15 > + This change get rid of ElementRareData::m_treeScope by replacing Nit: gets rid of. > Source/WebCore/ChangeLog:16 > + Node::m_document with Node::m_treeScope. And retrieves the Nit: And what retrieves the document? > Source/WebCore/ChangeLog:28 > + - It makes Node::m_treeScope a tagged pointer, whose tag implies > + that m_treeScope refers ShadowRoot, not Document. If the pointer > + isn't tagged, we can assume that m_treeScope is actually a > + Document and can return it as a Document rather than invoking > + TreeScope::roootDocument(). This eliminates an extra dereference > + in many case. Per IRC discussion, we should try node flag first. > Source/WebCore/dom/Document.cpp:496 > + setRootDocument(this); Why do we need to set the document again here? Isn't the call to TreeScope's constructor sufficient? In fact, I don't think we need setRootDocument.
Created attachment 151583 [details] Patch
Comment on attachment 151583 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151583&action=review > Source/WebCore/ChangeLog:24 > + that means m_treeScope is actually a document an can be returned as the result. Nit: s/an /and /. > Source/WebCore/ChangeLog:25 > + this eliminates an extract dereference. What are you referring to by "an extract dereference"? > Source/WebCore/ChangeLog:36 > + * dom/Document.cpp: Took care of connectio betwen TreeScope. Nit: s/connectio/connection/ > Source/WebCore/dom/Document.h:1537 > +inline Document* Node::documentInternal() const "Internal" is rather a vague adjective. I'd prefer using a more descriptive name like documentWithoutNodeTypeCheck. > Source/WebCore/dom/Document.h:1565 > + , m_treeScope(0) Do we need to initialize it if we're calling setTreeScope? It seems redundant. > Source/WebCore/dom/Node.h:731 > + Document* documentInternal() const; This should certainly be private, no?
Comment on attachment 151583 [details] Patch Attachment 151583 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13215010
Created attachment 151585 [details] Patch
Attachment 151585 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/inspector/PageConsoleAgent.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 151593 [details] Patch
Created attachment 152715 [details] Patch
Comment on attachment 152715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152715&action=review Just a few drive-by remarks. :) > Source/WebCore/dom/Document.h:1544 > + return m_treeScope == TreeScope::nullInstance() ? 0 : m_treeScope; I'm wondering if it wouldn't be OK to just return the nullInstance (?). This would remove yet another condition here as well as below. (Perhaps have a TreeScopePointer wrapper class that implements operator! et al) > Source/WebCore/dom/Document.h:1548 > +{ If the nullInstance is returned and used throughout, one could just ASSERT(scope) here and take the scope without further condition (?). > Source/WebCore/dom/Document.h:1576 > + return treeScope() != document(); Couldn't you just 'return getFlag(InShadowTree);'? > Source/WebCore/dom/TreeScope.cpp:58 > + , m_parentTreeScope(rootNode == rootDocument ? 0 : rootDocument) Given the above, could use nullInstance instead of 0 here? > Source/WebCore/dom/TreeScope.cpp:67 > + , m_parentTreeScope(0) Given the above, could initialize m_parentTreeScope to nullInstance here?
Comment on attachment 152715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152715&action=review Hi Roland, thanks for your feedback! I'll update the patch shortly. >> Source/WebCore/dom/Document.h:1544 >> + return m_treeScope == TreeScope::nullInstance() ? 0 : m_treeScope; > > I'm wondering if it wouldn't be OK to just return the nullInstance (?). This would remove yet another condition here as well as below. (Perhaps have a TreeScopePointer wrapper class that implements operator! et al) Let callers take care of nullInstance() doen't look good idea for me. I'd like to keep it an implementation detail considering that - the instance is mutable. - the instance violates some invariant like the m_rootDocument never be null. >> Source/WebCore/dom/Document.h:1548 >> +{ > > If the nullInstance is returned and used throughout, one could just ASSERT(scope) here and take the scope without further condition (?). As I mentioned above, I don't want use nullInstance() widely. >> Source/WebCore/dom/Document.h:1576 >> + return treeScope() != document(); > > Couldn't you just 'return getFlag(InShadowTree);'? Good catch! Will fix. >> Source/WebCore/dom/TreeScope.cpp:67 >> + , m_parentTreeScope(0) > > Given the above, could initialize m_parentTreeScope to nullInstance here? We cannot do that because this constructor is exactly for nullInstance()
Created attachment 152727 [details] Patch
Comment on attachment 152727 [details] Patch This looks reasonable to me. Hopefully, I'm not failing on my review again here...
> (From update of attachment 152727 [details]) > This looks reasonable to me. Hopefully, I'm not failing on my review again here... I hope so too. Will wait one more day before landing and have a time to hear voices from veterans like Darin.
Created attachment 153407 [details] Patch for landing
Comment on attachment 153407 [details] Patch for landing Clearing flags on attachment: 153407 Committed r123184: <http://trac.webkit.org/changeset/123184>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 92048
Created attachment 157171 [details] WIP
Created attachment 157174 [details] WIP
A profiler told me that Node::document() is hit not only from DOM side but also from rendering side through RenderObject::document(). This is why Dromaeo couln't catch the slow down of the page cycler.
I have no idea how we can do it. Closing for now. Feel free to reopen and attack this if you come up with any possible ideas.
Per rniwa's request lets try this again.
Created attachment 178136 [details] Patch
There's no justification for not doing a pointer deref inside document in the original patch so I didn't do that as it makes this simpler. Morrita do you have a benchmark where you saw the pointer deref being a perf regression? Also what page cycler did you see the regression on?
Comment on attachment 178136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review This looks right. I’m hoping that you can give us some perf. test results before landing it so that we have some confidence in this new patch. I’ll wait for other reviewers (particularly morrita & darin) to comment. > Source/WebCore/dom/Document.h:1564 > + m_treeScope = TreeScope::noDocumentInstance(); We might want to consider inlining this function butI guess it’s rare to instantiate a node without a document that it doesn’t matter? > Source/WebCore/dom/TreeScope.h:113 > + TreeScope(); Why do we need this? > Source/WebCore/dom/TreeScopeAdopter.cpp:-100 > > - node->setDocument(newDocument); > - I think it deserves a change log comment saying that this node can’t be tree scope here.
(In reply to comment #63) > > Source/WebCore/dom/TreeScope.h:113 > > + TreeScope(); > > Why do we need this? Forget about this comment. It was added before I saw noDocumentInstance().
Here is a bug which reported the perf regression https://crbug.com/138410
(In reply to comment #63) > (From update of attachment 178136 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review > ... > > > Source/WebCore/dom/TreeScopeAdopter.cpp:-100 > > > > - node->setDocument(newDocument); > > - > > I think it deserves a change log comment saying that this node can’t be tree scope here. I don't understand what you mean, what do you want me to explain?
(In reply to comment #65) > Here is a bug which reported the perf regression https://crbug.com/138410 Hm… the regression is 2%. That might be too small of a difference to detect on a non-bot environment.
(In reply to comment #66) > (In reply to comment #63) > > (From update of attachment 178136 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review > > ... > > > > > Source/WebCore/dom/TreeScopeAdopter.cpp:-100 > > > > > > - node->setDocument(newDocument); > > > - > > > > I think it deserves a change log comment saying that this node can’t be tree scope here. > > I don't understand what you mean, what do you want me to explain? We don’t have to call setDocument here since node can’t be a TreeScope but that isn’t obvious from the code change. It’ll be good to point that out in the change log.
(In reply to comment #68) > > > We don’t have to call setDocument here since node can’t be a TreeScope but that isn’t obvious from the code change. It’ll be good to point that out in the change log. node can be a TreeScope here, we call this method for every ShadowRoot as well. The reason for not needing to call setDocument is because doing setDocumentScope implicitly changes the document for every descendant in the same scope.
(In reply to comment #69) > (In reply to comment #68) > > > > We don’t have to call setDocument here since node can’t be a TreeScope but that isn’t obvious from the code change. It’ll be good to point that out in the change log. > > node can be a TreeScope here, we call this method for every ShadowRoot as well. The reason for not needing to call setDocument is because doing setDocumentScope implicitly changes the document for every descendant in the same scope. I’m talking about the case when moveNodeToNewDocument is called directly from moveTreeToNewScope at line 60-61. On my second thought, this could be a bug.
(In reply to comment #70) > (In reply to comment #69) > > (In reply to comment #68) > > > > > > We don’t have to call setDocument here since node can’t be a TreeScope but that isn’t obvious from the code change. It’ll be good to point that out in the change log. > > > > node can be a TreeScope here, we call this method for every ShadowRoot as well. The reason for not needing to call setDocument is because doing setDocumentScope implicitly changes the document for every descendant in the same scope. > > I’m talking about the case when moveNodeToNewDocument is called directly from moveTreeToNewScope at line 60-61. On my second thought, this could be a bug. We have to call it to maintain the guard refs and update the node lists after setting the tree scope and then to notify the node with didMoveToNewDocument. If the document changed we need to call moveNodeToNewDocument on every node in the top level tree, and then on every node all the way down through every shadow root.
(In reply to comment #71) > We have to call it to maintain the guard refs and update the node lists after setting the tree scope and then to notify the node with didMoveToNewDocument. If the document changed we need to call moveNodeToNewDocument on every node in the top level tree, and then on every node all the way down through every shadow root. I know we have to call moveNodeToNewDocument there. What’s not obvious is why we don’t have to call either setDocument or setDocumentScope.
(In reply to comment #72) > (In reply to comment #71) > > We have to call it to maintain the guard refs and update the node lists after setting the tree scope and then to notify the node with didMoveToNewDocument. If the document changed we need to call moveNodeToNewDocument on every node in the top level tree, and then on every node all the way down through every shadow root. > > I know we have to call moveNodeToNewDocument there. What’s not obvious is why we don’t have to call either setDocument or setDocumentScope. Hmm, it seems we do attempt to adopt ShadowRoot's during removeAllShadowRoots and we notify of their removal which is somewhat concerning because it means during that notification shadowRoot->treeScope() is not shadowRoot, but actually the document. This seems wrong since normally shadowRoot->treeScope() == shadowRoot but it's always been this way. That might be a bug, but I don't know how you observe the behavior. That's the only time node can ever be a TreeScope.
Created attachment 178154 [details] Patch Never end up in a situation where a TreeScope is detached. Also addressed rniwa's other comments.
Comment on attachment 178136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review I like it! > Source/WebCore/dom/TreeScope.h:63 > + Document* documentScope() const { return m_documentScope; } Can we s/documentScope/document here? I think that would read a lot better in the calling locations.
Comment on attachment 178136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review >> Source/WebCore/dom/TreeScope.h:63 >> + Document* documentScope() const { return m_documentScope; } > > Can we s/documentScope/document here? I think that would read a lot better in the calling locations. I don’t think we want do that given Document & ShdowRoot each of which inherits from TreeScope also has document().
Comment on attachment 178154 [details] Patch Alright, why don't we try this one since it looks promising.
(In reply to comment #76) > (From update of attachment 178136 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review > > ... > > Source/WebCore/dom/TreeScope.h:63 > > + Document* documentScope() const { return m_documentScope; } > > Can we s/documentScope/document here? I think that would read a lot better in the calling locations. Would you prefer rootScope? Like rniwa said, I didn't do that since we already have a document method on Node.
Comment on attachment 178154 [details] Patch Rejecting attachment 178154 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: #1 succeeded at 55 (offset 1 line). Hunk #2 succeeded at 112 (offset 1 line). patching file Source/WebCore/dom/TreeScope.h patching file Source/WebCore/dom/TreeScopeAdopter.cpp Hunk #1 FAILED at 42. Hunk #2 succeeded at 97 (offset 1 line). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/dom/TreeScopeAdopter.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Ryosuke Ni..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/15282622
Created attachment 179102 [details] Patch for landing
Comment on attachment 179102 [details] Patch for landing Rejecting attachment 179102 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ataBase(WebCore::Document*&)' Source/WebCore/dom/Node.h:120: note: candidates are: WebCore::NodeRareDataBase::NodeRareDataBase() Source/WebCore/dom/Node.h:113: note: WebCore::NodeRareDataBase::NodeRareDataBase(const WebCore::NodeRareDataBase&) make: *** [out/Release/obj.target/webcore_dom/Source/WebCore/dom/ClassNodeList.o] Error 1 Failed to run "['Tools/Scripts/build-webkit', '--release', '--chromium', '--update-chromium']" exit_code: 2 rce/WebCore/dom/ClassNodeList.o] Error 1 Full output: http://queues.webkit.org/results/15281669
Created attachment 179129 [details] Patch Fix the merge for real this time. It seems gits automerge put the Node::setDocument method back in Node.cpp when I synced last time
Comment on attachment 179129 [details] Patch Clearing flags on attachment: 179129 Committed r137524: <http://trac.webkit.org/changeset/137524>
Re-opened since this is blocked by bug 104859
Created attachment 179174 [details] Patch
Did you run all the tests in Debug mode?
(In reply to comment #87) > Did you run all the tests in Debug mode? I didn't request a commit because I haven't run all the tests yet, I did run fast/mutation and fast/dom though and this fixes it. Right now I can't get it to build because r137539 broke the chromium build, so I'll run them later tonight/tomorrow and update.
(In reply to comment #88) > (In reply to comment #87) > > Did you run all the tests in Debug mode? > > I didn't request a commit because I haven't run all the tests yet, I did run fast/mutation and fast/dom though and this fixes it. Right now I can't get it to build because r137539 broke the chromium build, so I'll run them later tonight/tomorrow and update. run-webkit-tests --chromium --debug --no-pixel-tests LayoutTests/fast/ 9502 tests executed and I didn't see any crashes.
Comment on attachment 179174 [details] Patch (In reply to comment #89) > (In reply to comment #88) > > (In reply to comment #87) > > > Did you run all the tests in Debug mode? > > > > I didn't request a commit because I haven't run all the tests yet, I did run fast/mutation and fast/dom though and this fixes it. Right now I can't get it to build because r137539 broke the chromium build, so I'll run them later tonight/tomorrow and update. > > run-webkit-tests --chromium --debug --no-pixel-tests LayoutTests/fast/ > > 9502 tests executed and I didn't see any crashes. For the change of this nature that could affect literally everything in DOM, it’s not acceptable to run just a subset of tests. WebKit contribution policy that contributors run all layout tests prior to landing the patch: http://www.webkit.org/quality/testing.html "Before patches can land in any of the frameworks in the repository, the layout regression tests must pass. To run these tests, execute the run-webkit-tests script." In practice, you may get away with it by running a subset if you know that the patch only affects a subset of tests in advance but we can’t necessarily do that all the time. Please run the entire layout test suite and report the result (I typically start run-webkit-tests before I go to lunch or go home).
Created attachment 181128 [details] Patch
(In reply to comment #91) > Created an attachment (id=181128) [details] > Patch I ran all 27k tests in debug and there were no crashes.
Comment on attachment 181128 [details] Patch Clearing flags on attachment: 181128 Committed r138735: <http://trac.webkit.org/changeset/138735>