Bug 50184 – Provide a generic way to store shadowParent on a Node.

• Component: DOM
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Currently, exploring two approaches:

1) Adding a ptr to NodeRareData (impacts memory)
2) Overloading Node::parent to store either parent or shadowParent (impacts performance).

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

(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

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

Created attachment 75980 [details]
Patch

[Darin Adler]

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.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

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

[WebKit Review Bot]

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