Bug 88653 – Remove unneeded callRemovedLastRef function from TreeShared refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

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

[Kentaro Hara]

Created attachment 146551 [details]
Patch

[Ryosuke Niwa]

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.

[Ojan Vafai]

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?

[Kentaro Hara]

anttik, kling: http://trac.webkit.org/changeset/97137 introduced TreeShared<ContainerNode>. Would you take a look at the patch I uploaded?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Comment on attachment 146551 [details]
Patch

This looks like an overall improvement. Please update the bug titles in change logs before you land.

[Kentaro Hara]

Created attachment 146726 [details]
patch for landing

[WebKit Review Bot]

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

[Darin Adler]

Please don’t make this change!

[Darin Adler]

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?

[Darin Adler]

Comment on attachment 146551 [details]
Patch

Please do not land this change.

[Darin Adler]

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

[Kentaro Hara]

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;
  }

[Darin Adler]

I’ll take care of this.

[Darin Adler]

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

[Kentaro Hara]

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

[Kentaro Hara]

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

[Kentaro Hara]

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

[Darin Adler]

(In reply to comment #16)
> because 'class Node : TreeShared<Node>' is less hacky than 'class Node : TreeShared<ContainerNode>'

Hacky? No.

[Darin Adler]

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

[Darin Adler]

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.

[Darin Adler]

Created attachment 146740 [details]
Patch (Windows .def files not right yet)

[Darin Adler]

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

[Darin Adler]

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.

[Kentaro Hara]

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.

[Darin Adler]

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.

[Early Warning System Bot]

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

[Build Bot]

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

[Darin Adler]

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

[Darin Adler]

I may need some help finishing the patch (getting it to complete on all platforms and such).

[Kentaro Hara]

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

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Darin Adler]

Created attachment 146741 [details]
Patch

[Build Bot]

Comment on attachment 146741 [details]
Patch

Attachment 146741 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12919972

[Darin Adler]

Created attachment 146742 [details]
Patch

[Darin Adler]

Committed r119937: <http://trac.webkit.org/changeset/119937>