Because the Node class inherits TreeShared<ContainerNode> instead of TreeShared<Node>, TreeShared.h tends to be dirty. For example, we cannot write static_cast<T*>(this) in TreeShared.h. It cannot be compiled. Consequently, r119802 had to add callRemovedLastRef(ContainerNode* self) to ContainerNode.cpp just to call static_cast<T*>(self). See around https://bugs.webkit.org/show_bug.cgi?id=88528#c21 for more details. We should change 'class Node : public TreeShared<ContainerNode>' to 'class Node : public TreeShared<Node>'.
Created attachment 146551 [details] Patch
Comment on attachment 146551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146551&action=review > Source/WebCore/ChangeLog:4 > + Change 'class Node : public TreeShared<ContainerNode>' > + to 'class Node : public TreeShared<Node>' We don't normally line-wrap in the middle of the bug title. I've shortened the bug title for you.
Comment on attachment 146551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146551&action=review > Source/WebCore/ChangeLog:10 > + Because the Node class inherits TreeShared<ContainerNode> instead > + of TreeShared<Node>, TreeShared.h tends to be dirty. For example, Did you look at the svn history to see if it's always been like this? Maybe CC the person who first made the change?
anttik, kling: http://trac.webkit.org/changeset/97137 introduced TreeShared<ContainerNode>. Would you take a look at the patch I uploaded?
(In reply to comment #3) > (From update of attachment 146551 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146551&action=review > > > Source/WebCore/ChangeLog:10 > > + Because the Node class inherits TreeShared<ContainerNode> instead > > + of TreeShared<Node>, TreeShared.h tends to be dirty. For example, > > Did you look at the svn history to see if it's always been like this? Maybe CC the person who first made the change? It was done in http://trac.webkit.org/changeset/65852.
Comment on attachment 146551 [details] Patch This looks like an overall improvement. Please update the bug titles in change logs before you land.
Created attachment 146726 [details] patch for landing
Comment on attachment 146726 [details] patch for landing Rejecting attachment 146726 [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: er patching file Source/WebKit2/mac/WebKit2.order patching file Source/WebKit2/win/WebKit2.def patching file Source/WebKit2/win/WebKit2CFLite.def Hunk #1 FAILED at 108. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/win/WebKit2CFLite.def.rej patching file Source/autotools/symbols.filter patching file ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12914926
Please don’t make this change!
Having the parent of a Node be a ContainerNode can lead to more efficient code. We can call firstChild on a ContainerNode without checking the “is container node” bit, and do the types correctly. Why is this a good change?
Comment on attachment 146551 [details] Patch Please do not land this change.
(In reply to comment #0) > Consequently, r119802 had to add callRemovedLastRef(ContainerNode* self) to ContainerNode.cpp just to call static_cast<T*>(self). See around https://bugs.webkit.org/show_bug.cgi?id=88528#c21 for more details. That was indeed a bad idea. We can come up with a much better way to do this. But please don’t change the type of parent from ContainerNode* to Node* to solve the problem that was introduced with the poorly written code in r119802.
darin: Thanks for the clarification. We won't land it. (In reply to comment #12) > > Consequently, r119802 had to add callRemovedLastRef(ContainerNode* self) to ContainerNode.cpp just to call static_cast<T*>(self). See around https://bugs.webkit.org/show_bug.cgi?id=88528#c21 for more details. > > That was indeed a bad idea. We can come up with a much better way to do this. But please don’t change the type of parent from ContainerNode* to Node* to solve the problem that was introduced with the poorly written code in r119802. There are other reasons. As I posted to webkit-dev@, I am planning to change the reference counting algorithm so that "If Node X is reachable, all Nodes in the DOM tree which X belongs to are kept alive" is guaranteed. I have an idea to achieve the algorithm (hopefully) with no overhead and with no additional member variables, and the implementation requires to access TreeShared<T*> methods of other TreeShared<T*> objects, like this: template<typename T> class TreeShared { void rootRef() { m_refCount++; } void ref() { m_root->rootRef(); // This cannot be compiled here and there if Node inherits TreeShared<ContainerNode>. If Node inherited TreeShared<Node>, this works fine. } T* m_parent; T* m_root; // I do not intend to add this variable, this is just for explanation of what I want to do. int m_refCount; }
I’ll take care of this.
(In reply to comment #13) > I am planning to change the reference counting algorithm so that "If Node X is reachable, all Nodes in the DOM tree which X belongs to are kept alive" is guaranteed. I look forward to seeing what you do for this. From your description I do not yet understand your plans or why they will be efficient. Adding additional cost to every ref/deref on a Node seems likely to slow down many operations in WebKit.
(In reply to comment #10) > Why is this a good change? I thought it would be good because 'class Node : TreeShared<Node>' is less hacky than 'class Node : TreeShared<ContainerNode>', and the change seemed to solve our problems (i.e. static_cast<T*>(this) and m_root->rootRef()) straightforwardly. > Having the parent of a Node be a ContainerNode can lead to more efficient code. We can call firstChild on a ContainerNode without checking the “is container node” bit, and do the types correctly. Does it really affect JavaScript performance? Bindings/first-child.html is the most micro benchmark for firstChild. (I could not test it until tomorrow morning due to blackout of buildings...)
(In reply to comment #15) > (In reply to comment #13) > > I am planning to change the reference counting algorithm so that "If Node X is reachable, all Nodes in the DOM tree which X belongs to are kept alive" is guaranteed. > > I look forward to seeing what you do for this. From your description I do not yet understand your plans or why they will be efficient. Adding additional cost to every ref/deref on a Node seems likely to slow down many operations in WebKit. I'll post a design document within this week.
(In reply to comment #16) > > Having the parent of a Node be a ContainerNode can lead to more efficient code. We can call firstChild on a ContainerNode without checking the “is container node” bit, and do the types correctly. > > Does it really affect JavaScript performance? Bindings/first-child.html is the most micro benchmark for firstChild. (I could not test it until tomorrow morning due to blackout of buildings...) Maybe Parser/query-selector-last.html might be the benchmark where we can observe the performance difference clearly. querySelector("#id") for an element that appears at the tail of the document requires to traverse a lot of DOM nodes using ContainerNode::firstChild() and ContainerNode::nextSiblings() without being interrupted by JavaScript bindings. (In case of Bindings/first-child.html, each ContainerNode::firstChild() is interrupted by JavaScript bindings, which might hide the overhead of ContainerNode::firstChild().) Anyway let me test it tomorrow.
(In reply to comment #16) > because 'class Node : TreeShared<Node>' is less hacky than 'class Node : TreeShared<ContainerNode>' Hacky? No.
(In reply to comment #16) > > Having the parent of a Node be a ContainerNode can lead to more efficient code. We can call firstChild on a ContainerNode without checking the “is container node” bit, and do the types correctly. > > Does it really affect JavaScript performance? I didn’t say JavaScript performance.
Continuing to use the specific type for ContainerNode is the right thing to do. Changing the design is unnecessary and you only had to do it because you rushed. I have a patch ready now.
Created attachment 146740 [details] Patch (Windows .def files not right yet)
(In reply to comment #16) > Bindings/first-child.html is the most micro benchmark for firstChild. JavaScript firstChild won’t be affected. It’s DOM manipulation in C++ that will be affected, not JavaScript. The ContainerNode class does not exist in JavaScript bindings.
These changes have gotten off on a strange tangent. It’s OK to remove the virtual functions from TreeShared to get rid of the extra overhead added when we added EventTarget. That’s a good idea. But these other changes are mostly just based on misunderstanding of TreeShared. There’s no need to change the parent pointer from TreeShared into a Node* instead of the correct type, ContainerNode*. And there’s no need for all the mess of callRemovedLastRef. It’s just because we rushed.
Comment on attachment 146740 [details] Patch (Windows .def files not right yet) View in context: https://bugs.webkit.org/attachment.cgi?id=146740&action=review I am sorry for the original patch that messed things up. Your change makes much sense to me. > Source/WebCore/dom/Node.h:107 > -class Node : public EventTarget, public ScriptWrappable, public TreeShared<ContainerNode> { > +class Node : public EventTarget, public ScriptWrappable, public TreeShared<Node, ContainerNode> { Shall we add a comment about why we use TreeShared<Node, ContainerNode> instead of TreeShared<Node>? I discussed it with 4 folks in IRC last week, but no one could not think up of the reason.
Oh, I see you renamed this back; I thought that was just bugs.webkit.org malfunctioning. Does that mean you still want to change TreeShared so it doesn’t know the parent node type? That’s not a good idea. We should land my patch instead. Sorry, I didn’t mean to argue with you about the bug title. If you want to change this back and have me use a different bug report, that’s OK. But I strongly disagree with changing to TreeShared<Node> just because you think it’s “hacky”. The parent node type should use the correct type, ContainerNode, unless there is some concrete reason not to.
Comment on attachment 146740 [details] Patch (Windows .def files not right yet) Attachment 146740 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12937388
Comment on attachment 146740 [details] Patch (Windows .def files not right yet) Attachment 146740 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12939132
(In reply to comment #25) > Shall we add a comment about why we use TreeShared<Node, ContainerNode> instead of TreeShared<Node>? I discussed it with 4 folks in IRC last week, but no one could not think up of the reason. We could, but I think a fine way to clear that up is the template argument names, NodeType and ParentNodeType, in TreeShared.h. If the template argument name was ParentNodeType last week when you asked, I think they would have quickly figured it out. I think the problem is that people are familiar with RefCounted and assume that the argument type to TreeShared is the same as RefCounted. So I think the problem from last week is solved.
I may need some help finishing the patch (getting it to complete on all platforms and such).
(In reply to comment #26) > Oh, I see you renamed this back; I thought that was just bugs.webkit.org malfunctioning. That's just bugs.webkit.org malfunctioning. > But I strongly disagree with changing to TreeShared<Node> just because you think it’s “hacky”. The parent node type should use the correct type, ContainerNode, unless there is some concrete reason not to. Now I understood your change is much much better:)
Comment on attachment 146740 [details] Patch (Windows .def files not right yet) Attachment 146740 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12914995
Comment on attachment 146740 [details] Patch (Windows .def files not right yet) Attachment 146740 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12925742
Comment on attachment 146740 [details] Patch (Windows .def files not right yet) Attachment 146740 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12941074
Created attachment 146741 [details] Patch
Comment on attachment 146741 [details] Patch Attachment 146741 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12919972
Created attachment 146742 [details] Patch
Committed r119937: <http://trac.webkit.org/changeset/119937>