Bug 50184

Created attachment 75072 [details] WIP: Measuring perf impact.

(In reply to comment #1) > Created an attachment (id=75072) [details] > WIP: Measuring perf impact. It's kind of weird, but I don't see any measurable difference showing up on Dromaeo DOM traversal tests. I think I'll go cook up a crazy microbenchmark for this.

(In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=75072) [details] [details] > > WIP: Measuring perf impact. > > It's kind of weird, but I don't see any measurable difference showing up on Dromaeo DOM traversal tests. I think I'll go cook up a crazy microbenchmark for this. For reference, here are the results for before (first two) and after (next two) applying the patch. http://dromaeo.com/?id=124189,124205,124183,124209

Created attachment 75190 [details] Dumb parentNode microbenchmark So I wrote a benchy. Running it before/after reveals a nearly unnoticeable (0.8%) performance degradation for this specific parentNode() operation. However, I am pretty sure it will be offset by net gain, given that style recalc and event propagation use parentOrHostNode, which is now faster. Let me see if I can capture this in a separate microbenchmark.

Created attachment 75214 [details] Dumb event propagation microbenchmark Meh. Turns out the performance on event firing gain is also fractional (less than 1%). Oh well. I'll run this on the Chromium page cyclers next and if everything looks good, I think we should go with this approach.

(In reply to comment #5) > Created an attachment (id=75214) [details] > Dumb event propagation microbenchmark > > Meh. Turns out the performance on event firing gain is also fractional (less than 1%). Oh well. I'll run this on the Chromium page cyclers next and if everything looks good, I think we should go with this approach. Ok, it's looking pretty good here. I am going to polish the patch and watch the perf bots when it lands.

Created attachment 75317 [details] Numbers from Chromium page cycler Just for reference, here are the numbers from the Chromium page cycler (pretty-printer attached): moz before: 14839, after: 15914, delta: 1075, perc:7% intl1 before: 136332, after: 114888, delta: -21444, perc:-18% intl2 before: 88355, after: 74797, delta: -13558, perc:-18% morejs before: 35605, after: 37228, delta: 1623, perc:5% alexa before: 14745, after: 10316, delta: -4429, perc:-42% moz2 before: 14835, after: 14384, delta: -451, perc:-3% bloat before: 34306, after: 35311, delta: 1005, perc:3% All regressions are within tolerance range. Alexa shows 42% improvement, but that's probably a fluke :)

Created attachment 75980 [details] Patch

Comment on attachment 75980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75980&action=review > WebCore/dom/Node.cpp:492 > + clearFlag(IsShadowRootFlag); Why clear the flag here? I guess just for clarity or something. > WebCore/rendering/ShadowElement.h:53 > + virtual void detach() > + { > + BaseElement::detach(); > + // FIXME: Remove once shadow DOM uses Element::setShadowRoot(). > + BaseElement::setShadowHost(0); > + } Is there a reason to have this in the class definition so it is treated as an inline request? I know the old code was doing that, but it’s best not to. > WebCore/rendering/TextControlInnerElements.cpp:106 > + // For elements not in shadow DOM (why?!), add the node to the DOM normally. This comment is now confusing to me. > WebCore/rendering/TextControlInnerElements.h:42 > + virtual void detach(); Can we make it protected or private? > WebCore/svg/SVGElement.cpp:121 > + ContainerNode* n = parentOrHostNode(); This could be a for loop. Later, I guess. > WebCore/svg/SVGElement.cpp:136 > + ContainerNode* n = parentOrHostNode(); This could be a for loop. Later, I guess.

(In reply to comment #9) > (From update of attachment 75980 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75980&action=review > > > WebCore/dom/Node.cpp:492 > > + clearFlag(IsShadowRootFlag); > > Why clear the flag here? I guess just for clarity or something. Yup, I didn't want to have two zero-states for parent. If it's zero, it's assumed to not be shadow DOM. In other words, an isolated subtree can't be a shadow DOM subtree. > > > WebCore/rendering/ShadowElement.h:53 > > + virtual void detach() > > + { > > + BaseElement::detach(); > > + // FIXME: Remove once shadow DOM uses Element::setShadowRoot(). > > + BaseElement::setShadowHost(0); > > + } > > Is there a reason to have this in the class definition so it is treated as an inline request? I know the old code was doing that, but it’s best not to. The reason is I was lazy! :) I'll move it out on landing. > > > WebCore/rendering/TextControlInnerElements.cpp:106 > > + // For elements not in shadow DOM (why?!), add the node to the DOM normally. > > This comment is now confusing to me. I'll remove my verbal flippiness. > > WebCore/rendering/TextControlInnerElements.h:42 > > + virtual void detach(); > > Can we make it protected or private? No, I tried -- we call these directly. > > > WebCore/svg/SVGElement.cpp:121 > > + ContainerNode* n = parentOrHostNode(); > > This could be a for loop. Later, I guess. > > > WebCore/svg/SVGElement.cpp:136 > > + ContainerNode* n = parentOrHostNode(); > > This could be a for loop. Later, I guess. Sounds good, I'll clean up in a follow-up patch.

Committed r73618: <http://trac.webkit.org/changeset/73618>

http://trac.webkit.org/changeset/73618 might have broken Leopard Intel Release (Tests) The following tests are not passing: inspector/styles-source-offsets.html