Bug 80706

Created attachment 131056 [details] Patch

Comment on attachment 131056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131056&action=review > Source/WebCore/dom/ContainerNode.cpp:68 > + size_t newSize = nodes.size(); > + for (Node* child = node->firstChild(); child; child = child->nextSibling()) > + newSize++; There's already a function that does. We might need to change the type of |node| to ContainerNode to see it.

Comment on attachment 131056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131056&action=review >> Source/WebCore/dom/ContainerNode.cpp:68 >> + newSize++; > > There's already a function that does. We might need to change the type of |node| to ContainerNode to see it. childNodeCount

(In reply to comment #2) > (From update of attachment 131056 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131056&action=review > > > Source/WebCore/dom/ContainerNode.cpp:68 > > + size_t newSize = nodes.size(); > > + for (Node* child = node->firstChild(); child; child = child->nextSibling()) > > + newSize++; > > There's already a function that does. We might need to change the type of |node| to ContainerNode to see it. Unfortunately, that requires changing collectTargetNodes(), and a number of call sites pass in a plain ol' Node to collectTargetNodes() (which can't be blindly downcast, I presume). I could move childNodeCount() up to Node, but considering this is just a speculative fix for which I'd like to get some data off the perf bots, maybe we could try it as-is?

Comment on attachment 131056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131056&action=review >>>> Source/WebCore/dom/ContainerNode.cpp:68 >>>> + newSize++; >>> >>> There's already a function that does. We might need to change the type of |node| to ContainerNode to see it. >> >> childNodeCount > > Unfortunately, that requires changing collectTargetNodes(), and a number of call sites pass in a plain ol' Node to collectTargetNodes() (which can't be blindly downcast, I presume). > > I could move childNodeCount() up to Node, but considering this is just a speculative fix for which I'd like to get some data off the perf bots, maybe we could try it as-is? All nodes that have children are ContainerNodes so caller can either downcast the pointer or never call this function depending on the type of the node.

Created attachment 131065 [details] Patch

Comment on attachment 131065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131065&action=review > Source/WebCore/dom/ContainerNode.cpp:78 > - collectNodes(node, nodes); > + if (!node->isContainerNode()) > + return; node is a DocumentFragment (which inherits from ContainerNode) at this point so there's no need to call isContainerNode.

As I've privately told you, bumping up the initial capacity will probably buy us more performance than allocating the entire buffer up front.

Created attachment 131081 [details] Patch

Comment on attachment 131081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131081&action=review > Source/WebCore/ChangeLog:9 > + Covered by existing tests. You need to explain what tests this improves performance on. > Source/WebCore/dom/ContainerNode.cpp:66 > + nodes.reserveCapacity(nodes.size() + node->childNodeCount()); I believe childNodeCount() walks all the node pointers... but I could imagine avoiding multiple mallocs would be faster. > Source/WebCore/dom/ContainerNode.cpp:77 > - collectNodes(node, nodes); > + collectNodes(static_cast<ContainerNode*>(node), nodes); This doesn't seem safe.

Comment on attachment 131081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131081&action=review >> Source/WebCore/dom/ContainerNode.cpp:77 >> + collectNodes(static_cast<ContainerNode*>(node), nodes); > > This doesn't seem safe. This is okay because node is a DocumentFragment at this point. See line 73 :) But it might be better to cast it to DocumentFragment for clarity.

Comment on attachment 131081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131081&action=review >>> Source/WebCore/dom/ContainerNode.cpp:77 >>> + collectNodes(static_cast<ContainerNode*>(node), nodes); >> >> This doesn't seem safe. > > This is okay because node is a DocumentFragment at this point. See line 73 :) > But it might be better to cast it to DocumentFragment for clarity. Ah yes, I had read the above as the DocumentFramgment-only special case, missed the !=. You're right though, casing to DocumentFragment would probably be more clear.

Comment on attachment 131081 [details] Patch Please update the ChangeLog before landing.

Created attachment 131118 [details] Patch

Comment on attachment 131081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131081&action=review >> Source/WebCore/ChangeLog:9 >> + Covered by existing tests. > > You need to explain what tests this improves performance on. Done. Note that this is a speculative fix: I'm going to land it and evalute it based on the listed tests. If there's no improvement, I'll revert. >>>> Source/WebCore/dom/ContainerNode.cpp:77 >>>> + collectNodes(static_cast<ContainerNode*>(node), nodes); >>> >>> This doesn't seem safe. >> >> This is okay because node is a DocumentFragment at this point. See line 73 :) >> But it might be better to cast it to DocumentFragment for clarity. > > Ah yes, I had read the above as the DocumentFramgment-only special case, missed the !=. You're right though, casing to DocumentFragment would probably be more clear. OK, done.

Comment on attachment 131118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131118&action=review > Source/WebCore/ChangeLog:9 > + Covered by existing tests. Performance will be evaluated based on Nit: we use one space after period.

Committed r110358: <http://trac.webkit.org/changeset/110358>

Result: the tests of interest showed no change (positive or negative) after this CL, so I'm going to revert it. e.g., http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/bloat-http/report.html?history=150&rev=-1 (change corresponds to Chrome r125985 on that chart).

Rolled out in http://trac.webkit.org/changeset/110378.

I've looked into how many nodes are typically inserted by the following diff: ndex: Source/WebCore/dom/ContainerNode.cpp =================================================================== --- Source/WebCore/dom/ContainerNode.cpp (revision 110541) +++ Source/WebCore/dom/ContainerNode.cpp (working copy) @@ -65,6 +65,7 @@ { for (Node* child = node->firstChild(); child; child = child->nextSibling()) nodes.append(child); + fprintf(stderr, "[%ld]\n", nodes.size()); } and browsed on popular websites like facebook, twitter, gmail, etc... sample size: 188272 Statistics ignoring entries with 0s and 1s: mean: 4.41114811377 std: 3.31585908742 min: 2 max: 151 nodes.size() > 5: 9908 5.26% nodes.size() > 10: 2471 1.31% nodes.size() > 11: 915 0.49% nodes.size() > 12: 863 0.46% nodes.size() > 13: 834 0.44% nodes.size() > 15: 154 0.082% 11 is probably a good number to use.

Created attachment 131762 [details] Increases the stack allocated buffer size

Comment on attachment 131762 [details] Increases the stack allocated buffer size View in context: https://bugs.webkit.org/attachment.cgi?id=131762&action=review > Source/WebCore/dom/ContainerNode.cpp:58 > +typedef Vector<RefPtr<Node>, 11> NodeVector; You should probably add a comment explaining how you arrived at this number.

(In reply to comment #22) > (From update of attachment 131762 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131762&action=review > > > Source/WebCore/dom/ContainerNode.cpp:58 > > +typedef Vector<RefPtr<Node>, 11> NodeVector; > > You should probably add a comment explaining how you arrived at this number. We don't normally add that kind of comment in the code. Instead, we tend to document those things in the change log.

(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 131762 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=131762&action=review > > > > > Source/WebCore/dom/ContainerNode.cpp:58 > > > +typedef Vector<RefPtr<Node>, 11> NodeVector; > > > > You should probably add a comment explaining how you arrived at this number. > > We don't normally add that kind of comment in the code. Instead, we tend to document those things in the change log. My understanding of WebKit's policy on comments is that code should be self-documenting, through the use of descriptive variable names and the like. A magic value in the code is not self-documenting. The next developer who discovers this value in the code would have to root around in the VCS to find out the change which added it, and then look up the ChangeLog to find the reasoning.

(In reply to comment #24) > My understanding of WebKit's policy on comments is that code should be self-documenting, through the use of descriptive variable names and the like. A magic value in the code is not self-documenting. The next developer who discovers this value in the code would have to root around in the VCS to find out the change which added it, and then look up the ChangeLog to find the reasoning. e.g. http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSStyleSelector.cpp#L1143 As much as I understand where you're coming from, WebKit has a very different policy and culture from Google/Chromium. Please respect that.

(In reply to comment #25) > (In reply to comment #24) > > My understanding of WebKit's policy on comments is that code should be self-documenting, through the use of descriptive variable names and the like. A magic value in the code is not self-documenting. The next developer who discovers this value in the code would have to root around in the VCS to find out the change which added it, and then look up the ChangeLog to find the reasoning. > > e.g. > http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSStyleSelector.cpp#L1143 static const unsigned cStyleSearchThreshold = 10; ? This seems to be an example of a descriptive variable name (or at least, an attempt). If you'd do that in this patch, I think it would be an improvement, although I still think an explanation of how the value was derived wouldn't be out of place. > As much as I understand where you're coming from, WebKit has a very different policy and culture from Google/Chromium. Please respect that. That's exactly what I'm attempting to do (which is tricky, since this is not actually addressed in the coding guidelines). Here's a quote from Darin Adler which I think is relevant to this case: "In general my preference is to have comments that say things that the code does not. Many header comments I have seen repeat things already said by the names and signatures of functions. Comments that answer questions such as "why" and that are brief would make a good addition, even to a header file."

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > My understanding of WebKit's policy on comments is that code should be self-documenting, through the use of descriptive variable names and the like. A magic value in the code is not self-documenting. The next developer who discovers this value in the code would have to root around in the VCS to find out the change which added it, and then look up the ChangeLog to find the reasoning. > > > > e.g. > > http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSStyleSelector.cpp#L1143 > > static const unsigned cStyleSearchThreshold = 10; ? > > This seems to be an example of a descriptive variable name (or at least, an attempt). If you'd do that in this patch, I think it would be an improvement, although I still think an explanation of how the value was derived wouldn't be out of place. In this case, a descriptive variable name helps because the semantics isn't clear from just seeing the value 10. However, in my patch, the semantics of 11 is vacuously clear; it's the initial size of the stack allocated buffer. Anyone who can read the code that uses Vector understands this instantly.

Comment on attachment 131762 [details] Increases the stack allocated buffer size r=me. You could just make it even bigger. Looks like this type is just a temporary and the stack allocated size is essentially free.

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > (In reply to comment #24) > > > > My understanding of WebKit's policy on comments is that code should be self-documenting, through the use of descriptive variable names and the like. A magic value in the code is not self-documenting. The next developer who discovers this value in the code would have to root around in the VCS to find out the change which added it, and then look up the ChangeLog to find the reasoning. > > > > > > e.g. > > > http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSStyleSelector.cpp#L1143 > > > > static const unsigned cStyleSearchThreshold = 10; ? > > > > This seems to be an example of a descriptive variable name (or at least, an attempt). If you'd do that in this patch, I think it would be an improvement, although I still think an explanation of how the value was derived wouldn't be out of place. > > In this case, a descriptive variable name helps because the semantics isn't clear from just seeing the value 10. However, in my patch, the semantics of 11 is vacuously clear; it's the initial size of the stack allocated buffer. Anyone who can read the code that uses Vector understands this instantly. Again, you're missing the point. They'll understand the "what" but not the "why". If it's still unclear to you why code peppered with inexplicable literals is not maintainable, I'll just leave someone else to r+ this patch.

Also the patch is sufficiently self document as-is.

(In reply to comment #30) > Also the patch is sufficiently self document as-is. Agreed. I'd probably go with inlineCapacity=16 myself to get a more "rounded" number. Shouldn't matter if these are never used as member variables. LGTM regardless.

Committed in http://trac.webkit.org/changeset/110797.

Oops, wrong bug :(

Comment on attachment 131762 [details] Increases the stack allocated buffer size Rejecting attachment 131762 [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: ti Koiv..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Parsed 2 diffs from patch file(s). patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/dom/ContainerNode.cpp Hunk #1 FAILED at 55. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/dom/ContainerNode.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Antti Koiv..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11952511

Comment on attachment 131762 [details] Increases the stack allocated buffer size View in context: https://bugs.webkit.org/attachment.cgi?id=131762&action=review >>>> Source/WebCore/dom/ContainerNode.cpp:58 >>>> +typedef Vector<RefPtr<Node>, 11> NodeVector; >>> >>> You should probably add a comment explaining how you arrived at this number. >> >> We don't normally add that kind of comment in the code. Instead, we tend to document those things in the change log. > > My understanding of WebKit's policy on comments is that code should be self-documenting, through the use of descriptive variable names and the like. A magic value in the code is not self-documenting. The next developer who discovers this value in the code would have to root around in the VCS to find out the change which added it, and then look up the ChangeLog to find the reasoning. Yeah, I think static const int ChildCountMoslyCovered = 11 will be fine for example. The point which should be emphasized here is that the number is nothing absolute and more empirical.

> > Yeah, I think static const int ChildCountMoslyCovered = 11 will be fine for example. > The point which should be emphasized here is that the number is nothing absolute and more empirical. > Oops the party looks over before I join.

(In reply to comment #35) > Yeah, I think static const int ChildCountMoslyCovered = 11 will be fine for example. > The point which should be emphasized here is that the number is nothing absolute and more empirical. We normally document these things in the change log, not in the actual code base.

Apparently this patch was committed in http://trac.webkit.org/changeset/110797 but mangled with my patch for https://bugs.webkit.org/show_bug.cgi?id=80900. Fixed the change log in http://trac.webkit.org/changeset/110797.

I mean in http://trac.webkit.org/changeset/110817.

I repeated Ryosuke's experiment from comment 20 today with a set of sites I chose myself. 11 still covers between 98% and 99% of all hits to collectChildNodes (which I think is the modern version of collectNodes). Still a good choice of inline size 13 years later based on my rough study.

That's good to know. I wonder if should adjust it to a round number like 16 though.