Bug 100738 - [Shadow DOM] Changing id, className, or attribute should invalidate distribution
: [Shadow DOM] Changing id, className, or attribute should invalidate distribution
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Shinya Kawanaka
:
Depends on:
Blocks: 100451 103474
  Show dependency treegraph
 
Reported: 2012-10-30 02:28 PDT by Shinya Kawanaka
Modified: 2012-11-27 18:04 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.44 KB, patch)
2012-10-30 03:44 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (54.63 KB, patch)
2012-11-08 21:05 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (20.92 KB, patch)
2012-11-11 21:35 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (26.57 KB, patch)
2012-11-12 20:38 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (26.55 KB, patch)
2012-11-12 21:01 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (26.19 KB, patch)
2012-11-16 00:31 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (31.01 KB, patch)
2012-11-18 21:49 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-10-30 02:28:09 PDT
When we change className dynamically, this does not reflect to distribution.
Comment 1 Shinya Kawanaka 2012-10-30 03:44:47 PDT
Created attachment 171409 [details]
Patch
Comment 2 Hajime Morrita 2012-10-30 04:03:43 PDT
Comment on attachment 171409 [details]
Patch

We could have attribute selector in @selector element. So covering class doesn't look sufficient.
Also, doing this for each attribut change looks so expensive. Is it possible to do this more efficiently?
Comment 3 Dimitri Glazkov (Google) 2012-10-30 10:57:05 PDT
Comment on attachment 171409 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171409&action=review

> Source/WebCore/dom/Element.cpp:779
> +    if (Element* parent = parentElement()) {

This seems way too specific. Can't we just put this in StyledElement::attributeChanged or something?
Comment 4 Shinya Kawanaka 2012-10-30 18:10:43 PDT
Ah, yeah... I've forgot the existence of attribute selector like input[type="something"}...

maybe we should check selectors when we invalidate the distribution?
Comment 5 Dimitri Glazkov (Google) 2012-10-31 10:36:51 PDT
(In reply to comment #4)
> Ah, yeah... I've forgot the existence of attribute selector like input[type="something"}...
> 
> maybe we should check selectors when we invalidate the distribution?

I wonder if we need to revisit when we invalidate/ensure the distribution. For style calculation purposes, all the StyledElement::attributeChanged needs to do is invalidate.

I wish we had the same type of laziness available to us.
Comment 6 Shinya Kawanaka 2012-11-08 21:05:03 PST
Created attachment 173192 [details]
WIP
Comment 7 Shinya Kawanaka 2012-11-08 21:05:51 PST
The previous WIP patch contains patches for Bug 101692 and Bug 101180.
Comment 8 Shinya Kawanaka 2012-11-11 21:35:20 PST
Created attachment 173551 [details]
Patch
Comment 9 Shinya Kawanaka 2012-11-11 22:46:18 PST
Comment on attachment 173551 [details]
Patch

This will conflict with Bug 101697.
Comment 10 Shinya Kawanaka 2012-11-11 22:47:47 PST
Oops, not Bug 101697 but Bug 101891.
Comment 11 Dimitri Glazkov (Google) 2012-11-12 09:16:01 PST
Is there any performance impact on this change?
Comment 12 Shinya Kawanaka 2012-11-12 17:29:37 PST
(In reply to comment #11)
> Is there any performance impact on this change?

Let me measure it.
Comment 13 Shinya Kawanaka 2012-11-12 20:27:41 PST
(In reply to comment #12)
> (In reply to comment #11)
> > Is there any performance impact on this change?
> 
> Let me measure it.

Surprisingly this patch makes AttributeChanged faster around 10%...
Comment 14 Shinya Kawanaka 2012-11-12 20:38:22 PST
Created attachment 173803 [details]
Patch
Comment 15 Dimitri Glazkov (Google) 2012-11-12 20:40:11 PST
Comment on attachment 173551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173551&action=review

> Source/WebCore/dom/Element.cpp:836
> +struct StyleInvalidationProposition {

proposition seems like the wrong word. Can we just call it a functor?  HasSelectorForClassStyleFunctor an HasSelectorForClassDistributionFunctor :)
Comment 16 Shinya Kawanaka 2012-11-12 20:51:03 PST
(In reply to comment #15)
> (From update of attachment 173551 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173551&action=review
> 
> > Source/WebCore/dom/Element.cpp:836
> > +struct StyleInvalidationProposition {
> 
> proposition seems like the wrong word. Can we just call it a functor?  HasSelectorForClassStyleFunctor an HasSelectorForClassDistributionFunctor :)

OK.
Comment 17 Shinya Kawanaka 2012-11-12 21:01:30 PST
Created attachment 173807 [details]
Patch
Comment 18 WebKit Review Bot 2012-11-12 22:50:49 PST
Comment on attachment 173807 [details]
Patch

Clearing flags on attachment: 173807

Committed r134367: <http://trac.webkit.org/changeset/134367>
Comment 19 WebKit Review Bot 2012-11-12 22:50:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Ojan Vafai 2012-11-13 10:28:36 PST
Hard to be sure it was this patch with so many webkit revisions going into this one build, but this looks like it was a ~5% improvement on dromaeo_jslibstylejquery/jslib_style_jquery_jQuery___height___x10.

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=167395&graph=jslib_style_jquery_jQuery___height___x10&history=150
Comment 21 Ojan Vafai 2012-11-13 10:42:50 PST
Unfortunately, it looks like it's 5% regression on dromaeo_domcoreattr/dom_attr_setAttribute:

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_domcoreattr/report.html?rev=167405&graph=dom_attr_setAttribute&history=150

And the improvement I mention in comment 20, I'm now pretty sure is actually from https://bugs.webkit.org/show_bug.cgi?id=102031.

Mind committing a fix or trying a rollout so we can be sure it was this patch?
Comment 22 Dimitri Glazkov (Google) 2012-11-13 10:46:20 PST
(In reply to comment #21)
> Unfortunately, it looks like it's 5% regression on dromaeo_domcoreattr/dom_attr_setAttribute:
> 
> http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_domcoreattr/report.html?rev=167405&graph=dom_attr_setAttribute&history=150
> 
> And the improvement I mention in comment 20, I'm now pretty sure is actually from https://bugs.webkit.org/show_bug.cgi?id=102031.
> 
> Mind committing a fix or trying a rollout so we can be sure it was this patch?

When in doubt, roll it out :)
Comment 23 Dimitri Glazkov (Google) 2012-11-13 10:54:25 PST
Rolled out in http://trac.webkit.org/changeset/134443/ to speculate.
Comment 24 Shinya Kawanaka 2012-11-13 17:46:10 PST
(In reply to comment #23)
> Rolled out in http://trac.webkit.org/changeset/134443/ to speculate.

Hmm...
OK. I also felt weird that this patch improves the performance.

However, we have to do something like this to make ShadowDOM implementation correct.
What should I do?
Comment 25 Shinya Kawanaka 2012-11-14 18:25:32 PST
(In reply to comment #24)
> (In reply to comment #23)
> > Rolled out in http://trac.webkit.org/changeset/134443/ to speculate.
> 
> Hmm...
> OK. I also felt weird that this patch improves the performance.
> 
> However, we have to do something like this to make ShadowDOM implementation correct.
> What should I do?

Dimitri, do you have any ideas?
If not, I will try to consider how we improve performance, but definitely it's not an easy task...
Comment 26 Dimitri Glazkov (Google) 2012-11-15 21:01:56 PST
Okay, here are some high-level thoughts. The problem that I see right now is that we are too slow in this situation:
1) a chunk of JS runs, twiddling a bunch of attributes
2) repaint occurs only after JS finishes.

This is exactly the optimization that style resolution made by making style recalc lazy: they only set flags and then traverse to update these flags only when the new styles are actually necessary.

Perhaps we could switch to a similar scheme somehow?
Comment 27 Shinya Kawanaka 2012-11-16 00:26:29 PST
(In reply to comment #26)
> Okay, here are some high-level thoughts. The problem that I see right now is that we are too slow in this situation:
> 1) a chunk of JS runs, twiddling a bunch of attributes
> 2) repaint occurs only after JS finishes.
> 

Yes.

> This is exactly the optimization that style resolution made by making style recalc lazy: they only set flags and then traverse to update these flags only when the new styles are actually necessary.
> 
> Perhaps we could switch to a similar scheme somehow?

Actually we're doing similar things already.

I'm now thinking the regression reason is because just a lot of code are scatted in attributeChanged.
Let me try to merge it to one function. It will reduce performance regression, though there will still exist a bit.
Comment 28 Shinya Kawanaka 2012-11-16 00:31:54 PST
Created attachment 174622 [details]
Patch
Comment 29 Ojan Vafai 2012-11-16 09:39:26 PST
Comment on attachment 174622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174622&action=review

> Source/WebCore/dom/Element.cpp:762
> +    if (ElementShadow* parentElementShadow = shadowOfParentForDistribution(this)) {

I would guess that the 3% regression that remains is from the shadowOfParentForDistribution. Looking at that call, I would guess that the most expensive bit is the virtual call to isInsertionPoint.

I'm not sure what the best fix would be, but something you could experiment with locally to see if it fixes the regression would be to add a bit to NodeFlags to track whether a Node is an insertion point. Then isInsertionPoint doesn't need to be virtual.
Comment 30 Shinya Kawanaka 2012-11-18 21:39:40 PST
(In reply to comment #29)
> (From update of attachment 174622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174622&action=review
> 
> > Source/WebCore/dom/Element.cpp:762
> > +    if (ElementShadow* parentElementShadow = shadowOfParentForDistribution(this)) {
> 
> I would guess that the 3% regression that remains is from the shadowOfParentForDistribution. Looking at that call, I would guess that the most expensive bit is the virtual call to isInsertionPoint.
> 
> I'm not sure what the best fix would be, but something you could experiment with locally to see if it fixes the regression would be to add a bit to NodeFlags to track whether a Node is an insertion point. Then isInsertionPoint doesn't need to be virtual.

I've tried this. I could improve peformance a bit (3% regression -> 2% regression).
Improving attributeChanged() more needs another optimization. I think we should do in another patch if there exists such optimization.
Comment 31 Shinya Kawanaka 2012-11-18 21:49:08 PST
Created attachment 174888 [details]
Patch
Comment 32 Dimitri Glazkov (Google) 2012-11-19 09:44:44 PST
Comment on attachment 174888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174888&action=review

This patch has grown! :)

> Source/WebCore/ChangeLog:22
> +        will be the most affected by this patch. However, it's only 2% performance regression.

I am willing to say okay to this regression, only because it's such a silly use case. There's no need for anyone to modify an attribute in a tight loop like that.

> Source/WebCore/dom/Node.h:238
> +    bool isInsertionPoint() const { return getFlag(IsInsertionPointFlag); }

What if instead, we stored the state of whether the node's attributes would affect distribution here? Just a thought.
Comment 33 WebKit Review Bot 2012-11-19 10:14:44 PST
Comment on attachment 174888 [details]
Patch

Clearing flags on attachment: 174888

Committed r135174: <http://trac.webkit.org/changeset/135174>
Comment 34 WebKit Review Bot 2012-11-19 10:14:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 35 Shinya Kawanaka 2012-11-19 18:16:27 PST
(In reply to comment #32)
> > Source/WebCore/dom/Node.h:238
> > +    bool isInsertionPoint() const { return getFlag(IsInsertionPointFlag); }
> 
> What if instead, we stored the state of whether the node's attributes would affect distribution here? Just a thought.

What do you mean?
Comment 36 Hajime Morrita 2012-11-19 20:05:57 PST
(In reply to comment #35)
> (In reply to comment #32)
> > > Source/WebCore/dom/Node.h:238
> > > +    bool isInsertionPoint() const { return getFlag(IsInsertionPointFlag); }
> > 
> > What if instead, we stored the state of whether the node's attributes would affect distribution here? Just a thought.
> 
> What do you mean?

The point is that the flag can be more generic: It can also imply that there are some attributes which 
affects node distribution. I have no clear idea what these attributes exactly are.
Comment 37 Antti Koivisto 2012-11-21 06:12:03 PST
Comment on attachment 174888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174888&action=review

> Source/WebCore/dom/Element.cpp:866
> +struct HasSelectorForClassStyleFunctor {
> +    explicit HasSelectorForClassStyleFunctor(StyleResolver* resolver)
> +        : styleResolver(resolver)
> +    { }
> +
> +    bool operator()(const AtomicString& className) const
> +    {
> +        return styleResolver->hasSelectorForClass(className);
> +    }
> +
> +    StyleResolver* styleResolver;
> +};
> +
> +struct HasSelectorForClassDistributionFunctor {
> +    explicit HasSelectorForClassDistributionFunctor(ElementShadow* elementShadow)
> +        : elementShadow(elementShadow)
> +    { }
> +
> +    bool operator()(const AtomicString& className) const
> +    {
> +        return elementShadow->selectRuleFeatureSet().hasSelectorForClass(className);
> +    }
> +
> +    ElementShadow* elementShadow;
> +};
> +
> +template<typename Functor>
> +static bool checkFunctorForClassChange(const SpaceSplitString& changedClasses, Functor functor)
>  {
>      unsigned changedSize = changedClasses.size();
>      for (unsigned i = 0; i < changedSize; ++i) {
> -        if (styleResolver->hasSelectorForClass(changedClasses[i]))
> +        if (functor(changedClasses[i]))
>              return true;
>      }
>      return false;
>  }

This is ridiculously verbose.

> Source/WebCore/dom/Element.cpp:916
> +static inline bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& changedClasses, StyleResolver* styleResolver)
> +{
> +    return checkFunctorForClassChange(changedClasses, HasSelectorForClassStyleFunctor(styleResolver));
> +}
> +
> +static inline bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, StyleResolver* styleResolver)
> +{
> +    return checkFunctorForClassChange(oldClasses, newClasses, HasSelectorForClassStyleFunctor(styleResolver));
> +}
> +
> +static inline bool checkNeedsDistributionInvalidationForClassChange(const SpaceSplitString& changedClasses, ElementShadow* elementShadow)
> +{
> +    return checkFunctorForClassChange(changedClasses, HasSelectorForClassDistributionFunctor(elementShadow));
> +}
> +
> +static inline bool checkNeedsDistributionInvalidationForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, ElementShadow* elementShadow)
> +{
> +    return checkFunctorForClassChange(oldClasses, newClasses, HasSelectorForClassDistributionFunctor(elementShadow));
> +}

Our core code is turning into overly abstract mush. I don't think functor pattern should be used at all.
Comment 38 Shinya Kawanaka 2012-11-27 18:04:45 PST
Antti,

Sorry for late response. I'll try to make it simple.
https://bugs.webkit.org/show_bug.cgi?id=103474