Bug 59816

Summary:

[Refactoring] Replace Node's Document pointer with a TreeScope pointer

Product:

WebKit

Reporter:

Roland Steiner <rolandsteiner>

Component:

DOM

Assignee:

Elliott Sprehn <esprehn>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, barraclough, cmarcelo, darin, dglazkov, dominicc, esprehn, ggaren, gustavo, mifenton, morrita, ojan.autocc, philn, rakuco, rniwa, rolandsteiner, simon.fraser, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

73800, 74067, 75240, 75241, 75290, 92048, 92049, 104859

Bug Blocks:

72352, 87034, 90913

Attachments:

Description	Flags
Patch	none
Patch	none
Just for examining ews	none
Patch	none
Patch	none
Updated to ToT	none
WIP: Naive version	none
WIP: Naive version	none
WIP: with a few peephole optimizations	none
WIP: Almost working	none
Yet another trial with a tag bit	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
WIP	none
WIP	none
Patch	none
Patch	none
Patch for landing	none
Patch	none
Patch	none
Patch	none

Roland Steiner

Reported 2011-04-29 11:33:31 PDT

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.

Attachments
Patch (16.85 KB, patch) 2012-01-10 01:20 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (17.84 KB, patch) 2012-01-10 02:52 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Just for examining ews (18.37 KB, patch) 2012-01-10 21:03 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (26.57 KB, patch) 2012-02-12 20:39 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (24.42 KB, patch) 2012-03-29 04:46 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Updated to ToT (24.45 KB, patch) 2012-06-13 00:24 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
WIP: Naive version (12.14 KB, patch) 2012-06-27 01:57 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
WIP: Naive version (13.57 KB, patch) 2012-06-27 02:12 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
WIP: with a few peephole optimizations (26.58 KB, patch) 2012-06-29 00:46 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
WIP: Almost working (28.24 KB, patch) 2012-06-29 04:34 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Yet another trial with a tag bit (21.94 KB, patch) 2012-07-04 21:54 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (29.33 KB, patch) 2012-07-04 23:14 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (26.91 KB, patch) 2012-07-09 19:08 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (22.31 KB, patch) 2012-07-10 20:31 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (22.87 KB, patch) 2012-07-10 21:00 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (22.89 KB, patch) 2012-07-10 21:52 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (23.00 KB, patch) 2012-07-17 01:26 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (23.29 KB, patch) 2012-07-17 03:58 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch for landing (24.57 KB, patch) 2012-07-19 22:10 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
WIP (26.08 KB, patch) 2012-08-08 04:06 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
WIP (26.08 KB, patch) 2012-08-08 04:12 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (20.10 KB, patch) 2012-12-06 19:12 PST, Elliott Sprehn	no flags	Details Formatted Diff Diff
Patch (19.52 KB, patch) 2012-12-06 21:16 PST, Elliott Sprehn	no flags	Details Formatted Diff Diff
Patch for landing (18.16 KB, patch) 2012-12-12 12:24 PST, Elliott Sprehn	no flags	Details Formatted Diff Diff
Patch (19.50 KB, patch) 2012-12-12 14:36 PST, Elliott Sprehn	no flags	Details Formatted Diff Diff
Patch (20.36 KB, patch) 2012-12-12 18:20 PST, Elliott Sprehn	no flags	Details Formatted Diff Diff
Patch (20.60 KB, patch) 2013-01-02 18:30 PST, Elliott Sprehn	no flags	Details Formatted Diff Diff
Show Obsolete (26) View All Add attachment proposed patch, testcase, etc.

Dominic Cooney

Comment 1 2011-11-20 18:27:16 PST

This is not specific to the content element.

Hajime Morrita

Comment 2 2011-12-04 22:49:10 PST

Starting to work on this.

Hajime Morrita

Comment 3 2012-01-10 01:20:33 PST

Created attachment 121810 [details] Patch

Early Warning System Bot

Comment 4 2012-01-10 01:40:58 PST

Comment on attachment 121810 [details] Patch Attachment 121810 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11166919

Gyuyoung Kim

Comment 5 2012-01-10 01:47:00 PST

Comment on attachment 121810 [details] Patch Attachment 121810 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11172381

Roland Steiner

Comment 6 2012-01-10 01:53:50 PST

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?

Gustavo Noronha (kov)

Comment 7 2012-01-10 02:06:44 PST

Comment on attachment 121810 [details] Patch Attachment 121810 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11183917

Hajime Morrita

Comment 8 2012-01-10 02:52:28 PST

Created attachment 121818 [details] Patch

Hajime Morrita

Comment 9 2012-01-10 02:54:54 PST

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.

Early Warning System Bot

Comment 10 2012-01-10 03:18:05 PST

Comment on attachment 121818 [details] Patch Attachment 121818 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11186933

Gyuyoung Kim

Comment 11 2012-01-10 03:30:22 PST

Comment on attachment 121818 [details] Patch Attachment 121818 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11186937

Gustavo Noronha (kov)

Comment 12 2012-01-10 03:47:41 PST

Comment on attachment 121818 [details] Patch Attachment 121818 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11197001

WebKit Review Bot

Comment 13 2012-01-10 03:53:41 PST

Comment on attachment 121818 [details] Patch Attachment 121818 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11184994

Dimitri Glazkov (Google)

Comment 14 2012-01-10 08:50:41 PST

Comment on attachment 121818 [details] Patch This patch may have performance implications, right? If so, it needs to be tested for perf regressions.

Hajime Morrita

Comment 15 2012-01-10 17:29:09 PST

(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...

Hajime Morrita

Comment 16 2012-01-10 21:03:23 PST

Created attachment 121969 [details] Just for examining ews

Darin Adler

Comment 17 2012-01-11 11:01:33 PST

Comment on attachment 121969 [details] Just for examining ews Since Morrita says this is for EWS only, clearing review flag.

Adam Barth

Comment 18 2012-01-11 11:04:19 PST

@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.

Hajime Morrita

Comment 19 2012-01-11 17:08:17 PST

@darin, @abarth. Ah, sure. I should have done so. Thanks for your advice!

Hajime Morrita

Comment 20 2012-02-12 20:39:54 PST

Created attachment 126707 [details] Patch

Hajime Morrita

Comment 21 2012-03-29 04:46:02 PDT

Created attachment 134547 [details] Patch

Hajime Morrita

Comment 22 2012-03-29 04:47:43 PDT

I couldn't resolve performance regression and give this up. Will try another idea.

Hajime Morrita

Comment 23 2012-06-13 00:24:20 PDT

Reopening to attach new patch.

Hajime Morrita

Comment 24 2012-06-13 00:24:24 PDT

Created attachment 147249 [details] Updated to ToT

Hajime Morrita

Comment 25 2012-06-27 01:57:01 PDT

Created attachment 149707 [details] WIP: Naive version

Hajime Morrita

Comment 26 2012-06-27 02:12:35 PDT

Created attachment 149709 [details] WIP: Naive version

Hajime Morrita

Comment 27 2012-06-29 00:46:48 PDT

Created attachment 150106 [details] WIP: with a few peephole optimizations

Hajime Morrita

Comment 28 2012-06-29 04:34:05 PDT

Created attachment 150138 [details] WIP: Almost working

Hajime Morrita

Comment 29 2012-07-04 21:54:22 PDT

Created attachment 150872 [details] Yet another trial with a tag bit

Hajime Morrita

Comment 30 2012-07-04 23:14:42 PDT

Created attachment 150879 [details] Patch

Build Bot

Comment 31 2012-07-04 23:45:54 PDT

Comment on attachment 150879 [details] Patch Attachment 150879 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13139736

Gustavo Noronha (kov)

Comment 32 2012-07-05 00:39:40 PDT

Comment on attachment 150879 [details] Patch Attachment 150879 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13144040

Build Bot

Comment 33 2012-07-05 03:00:09 PDT

Comment on attachment 150879 [details] Patch Attachment 150879 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13141113

Build Bot

Comment 34 2012-07-05 03:04:55 PDT

Comment on attachment 150879 [details] Patch Attachment 150879 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13151004

Hajime Morrita

Comment 35 2012-07-09 19:08:19 PDT

Created attachment 151386 [details] Patch

Roland Steiner

Comment 36 2012-07-09 20:24:49 PDT

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.

Hajime Morrita

Comment 37 2012-07-09 20:42:34 PDT

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.

Simon Fraser (smfr)

Comment 38 2012-07-10 18:14:26 PDT

Comment on attachment 151386 [details] Patch r-. We don't want bits stuffed into pointers. This breaks lots of tools.

Ryosuke Niwa

Comment 39 2012-07-10 18:27:01 PDT

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.

Hajime Morrita

Comment 40 2012-07-10 20:31:15 PDT

Created attachment 151583 [details] Patch

Ryosuke Niwa

Comment 41 2012-07-10 20:44:44 PDT

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?

Early Warning System Bot

Comment 42 2012-07-10 20:53:54 PDT

Comment on attachment 151583 [details] Patch Attachment 151583 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13215010

Hajime Morrita

Comment 43 2012-07-10 21:00:20 PDT

Created attachment 151585 [details] Patch

WebKit Review Bot

Comment 44 2012-07-10 21:01:59 PDT

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.

Hajime Morrita

Comment 45 2012-07-10 21:52:24 PDT

Created attachment 151593 [details] Patch

Hajime Morrita

Comment 46 2012-07-17 01:26:42 PDT

Created attachment 152715 [details] Patch

Roland Steiner

Comment 47 2012-07-17 02:24:55 PDT

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?

Hajime Morrita

Comment 48 2012-07-17 03:31:21 PDT

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()

Hajime Morrita

Comment 49 2012-07-17 03:58:04 PDT

Created attachment 152727 [details] Patch

Ryosuke Niwa

Comment 50 2012-07-18 17:50:51 PDT

Comment on attachment 152727 [details] Patch This looks reasonable to me. Hopefully, I'm not failing on my review again here...

Hajime Morrita

Comment 51 2012-07-18 17:54:38 PDT

> (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.

Hajime Morrita

Comment 52 2012-07-19 22:10:44 PDT

Created attachment 153407 [details] Patch for landing

WebKit Review Bot

Comment 53 2012-07-19 23:34:07 PDT

Comment on attachment 153407 [details] Patch for landing Clearing flags on attachment: 153407 Committed r123184: <http://trac.webkit.org/changeset/123184>

WebKit Review Bot

Comment 54 2012-07-19 23:34:16 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 55 2012-07-23 17:39:09 PDT

Re-opened since this is blocked by 92048

Hajime Morrita

Comment 56 2012-08-08 04:06:20 PDT

Created attachment 157171 [details] WIP

Hajime Morrita

Comment 57 2012-08-08 04:12:14 PDT

Created attachment 157174 [details] WIP

Hajime Morrita

Comment 58 2012-08-10 02:33:11 PDT

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.

Hajime Morrita

Comment 59 2012-09-20 18:06:46 PDT

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.

Elliott Sprehn

Comment 60 2012-12-06 18:47:32 PST

Per rniwa's request lets try this again.

Elliott Sprehn

Comment 61 2012-12-06 19:12:10 PST

Created attachment 178136 [details] Patch

Elliott Sprehn

Comment 62 2012-12-06 19:26:47 PST

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?

Ryosuke Niwa

Comment 63 2012-12-06 19:45:36 PST

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.

Ryosuke Niwa

Comment 64 2012-12-06 19:46:26 PST

(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().

Hajime Morrita

Comment 65 2012-12-06 20:13:43 PST

Here is a bug which reported the perf regression https://crbug.com/138410

Elliott Sprehn

Comment 66 2012-12-06 20:23:01 PST

(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?

Ryosuke Niwa

Comment 67 2012-12-06 20:23:22 PST

(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.

Ryosuke Niwa

Comment 68 2012-12-06 20:24:14 PST

(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.

Elliott Sprehn

Comment 69 2012-12-06 20:35:36 PST

(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.

Ryosuke Niwa

Comment 70 2012-12-06 20:41:51 PST

(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.

Elliott Sprehn

Comment 71 2012-12-06 20:45:24 PST

(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.

Ryosuke Niwa

Comment 72 2012-12-06 20:48:18 PST

(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.

Elliott Sprehn

Comment 73 2012-12-06 21:04:41 PST

(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.

Elliott Sprehn

Comment 74 2012-12-06 21:16:33 PST

Created attachment 178154 [details] Patch Never end up in a situation where a TreeScope is detached. Also addressed rniwa's other comments.

Ojan Vafai

Comment 75 2012-12-06 21:29:49 PST

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.

Ryosuke Niwa

Comment 76 2012-12-06 21:32:06 PST

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().

Ryosuke Niwa

Comment 77 2012-12-10 21:30:12 PST

Comment on attachment 178154 [details] Patch Alright, why don't we try this one since it looks promising.

Elliott Sprehn

Comment 78 2012-12-12 11:53:55 PST

(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.

WebKit Review Bot

Comment 79 2012-12-12 12:06:33 PST

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

Elliott Sprehn

Comment 80 2012-12-12 12:24:31 PST

Created attachment 179102 [details] Patch for landing

WebKit Review Bot

Comment 81 2012-12-12 14:27:53 PST

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

Elliott Sprehn

Comment 82 2012-12-12 14:36:20 PST

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

WebKit Review Bot

Comment 83 2012-12-12 15:21:23 PST

Comment on attachment 179129 [details] Patch Clearing flags on attachment: 179129 Committed r137524: <http://trac.webkit.org/changeset/137524>

WebKit Review Bot

Comment 84 2012-12-12 15:21:33 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 85 2012-12-12 16:16:00 PST

Re-opened since this is blocked by bug 104859

Elliott Sprehn

Comment 86 2012-12-12 18:20:18 PST

Created attachment 179174 [details] Patch

Simon Fraser (smfr)

Comment 87 2012-12-12 18:21:04 PST

Did you run all the tests in Debug mode?

Elliott Sprehn

Comment 88 2012-12-12 18:27:26 PST

(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.

Elliott Sprehn

Comment 89 2012-12-12 18:48:45 PST

(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.

Ryosuke Niwa

Comment 90 2012-12-12 20:04:14 PST

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).

Elliott Sprehn

Comment 91 2013-01-02 18:30:18 PST

Created attachment 181128 [details] Patch

Elliott Sprehn

Comment 92 2013-01-02 18:31:25 PST

(In reply to comment #91) > Created an attachment (id=181128) [details] > Patch I ran all 27k tests in debug and there were no crashes.

WebKit Review Bot

Comment 93 2013-01-03 13:40:30 PST

Comment on attachment 181128 [details] Patch Clearing flags on attachment: 181128 Committed r138735: <http://trac.webkit.org/changeset/138735>

WebKit Review Bot

Comment 94 2013-01-03 13:40:42 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.