Bug 50184 - Provide a generic way to store shadowParent on a Node.
Summary: Provide a generic way to store shadowParent on a Node.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 49686 52905
Blocks: 48698
  Show dependency treegraph
 
Reported: 2010-11-29 14:14 PST by Dimitri Glazkov (Google)
Modified: 2011-01-21 10:54 PST (History)
5 users (show)

See Also:


Attachments
WIP: Measuring perf impact. (13.21 KB, patch)
2010-11-29 15:35 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Dumb parentNode microbenchmark (1.35 KB, text/html)
2010-11-30 13:32 PST, Dimitri Glazkov (Google)
no flags Details
Dumb event propagation microbenchmark (1.78 KB, text/html)
2010-11-30 15:16 PST, Dimitri Glazkov (Google)
no flags Details
Numbers from Chromium page cycler (14.22 KB, text/html)
2010-12-01 13:38 PST, Dimitri Glazkov (Google)
no flags Details
Patch (34.47 KB, patch)
2010-12-08 15:38 PST, Dimitri Glazkov (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2010-11-29 14:14:17 PST
Currently, exploring two approaches:

1) Adding a ptr to NodeRareData (impacts memory)
2) Overloading Node::parent to store either parent or shadowParent (impacts performance).
Comment 1 Dimitri Glazkov (Google) 2010-11-29 15:35:40 PST
Created attachment 75072 [details]
WIP: Measuring perf impact.
Comment 2 Dimitri Glazkov (Google) 2010-11-29 19:20:23 PST
(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.
Comment 3 Dimitri Glazkov (Google) 2010-11-30 09:18:32 PST
(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
Comment 4 Dimitri Glazkov (Google) 2010-11-30 13:32:10 PST
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.
Comment 5 Dimitri Glazkov (Google) 2010-11-30 15:16:04 PST
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.
Comment 6 Dimitri Glazkov (Google) 2010-11-30 20:08:46 PST
(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.
Comment 7 Dimitri Glazkov (Google) 2010-12-01 13:38:29 PST
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 :)
Comment 8 Dimitri Glazkov (Google) 2010-12-08 15:38:20 PST
Created attachment 75980 [details]
Patch
Comment 9 Darin Adler 2010-12-08 16:33:02 PST
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.
Comment 10 Dimitri Glazkov (Google) 2010-12-09 08:49:20 PST
(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.
Comment 11 Dimitri Glazkov (Google) 2010-12-09 09:23:13 PST
Committed r73618: <http://trac.webkit.org/changeset/73618>
Comment 12 WebKit Review Bot 2010-12-09 10:53:16 PST
http://trac.webkit.org/changeset/73618 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
inspector/styles-source-offsets.html