100738 – [Shadow DOM] Changing id, className, or attribute should invalidate distribution

RESOLVED FIXED 100738

[Shadow DOM] Changing id, className, or attribute should invalidate distribution

https://bugs.webkit.org/show_bug.cgi?id=100738

Summary [Shadow DOM] Changing id, className, or attribute should invalidate distribution

Shinya Kawanaka

Reported 2012-10-30 02:28:09 PDT

When we change className dynamically, this does not reflect to distribution.

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2012-10-30 03:44:47 PDT

Created attachment 171409 [details] Patch

Hajime Morrita

Comment 2 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?

Dimitri Glazkov (Google)

Comment 3 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?

Shinya Kawanaka

Comment 4 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?

Dimitri Glazkov (Google)

Comment 5 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.

Shinya Kawanaka

Comment 6 2012-11-08 21:05:03 PST

Created attachment 173192 [details] WIP

Shinya Kawanaka

Comment 7 2012-11-08 21:05:51 PST

The previous WIP patch contains patches for Bug 101692 and Bug 101180.

Shinya Kawanaka

Comment 8 2012-11-11 21:35:20 PST

Created attachment 173551 [details] Patch

Shinya Kawanaka

Comment 9 2012-11-11 22:46:18 PST

Comment on attachment 173551 [details] Patch This will conflict with Bug 101697.

Shinya Kawanaka

Comment 10 2012-11-11 22:47:47 PST

Oops, not Bug 101697 but Bug 101891.

Dimitri Glazkov (Google)

Comment 11 2012-11-12 09:16:01 PST

Is there any performance impact on this change?

Shinya Kawanaka

Comment 12 2012-11-12 17:29:37 PST

(In reply to comment #11) > Is there any performance impact on this change? Let me measure it.

Shinya Kawanaka

Comment 13 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%...

Shinya Kawanaka

Comment 14 2012-11-12 20:38:22 PST

Created attachment 173803 [details] Patch

Dimitri Glazkov (Google)

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

Shinya Kawanaka

Comment 16 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.

Shinya Kawanaka

Comment 17 2012-11-12 21:01:30 PST

Created attachment 173807 [details] Patch

WebKit Review Bot

Comment 18 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>

WebKit Review Bot

Comment 19 2012-11-12 22:50:54 PST

All reviewed patches have been landed. Closing bug.

Ojan Vafai

Comment 20 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

Ojan Vafai

Comment 21 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?

Dimitri Glazkov (Google)

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

Dimitri Glazkov (Google)

Comment 23 2012-11-13 10:54:25 PST

Rolled out in http://trac.webkit.org/changeset/134443/ to speculate.

Shinya Kawanaka

Comment 24 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?

Shinya Kawanaka

Comment 25 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...

Dimitri Glazkov (Google)

Comment 26 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?

Shinya Kawanaka

Comment 27 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.

Shinya Kawanaka

Comment 28 2012-11-16 00:31:54 PST

Created attachment 174622 [details] Patch

Ojan Vafai

Comment 29 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.

Shinya Kawanaka

Comment 30 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.

Shinya Kawanaka

Comment 31 2012-11-18 21:49:08 PST

Created attachment 174888 [details] Patch

Dimitri Glazkov (Google)

Comment 32 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.

WebKit Review Bot

Comment 33 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>

WebKit Review Bot

Comment 34 2012-11-19 10:14:50 PST

All reviewed patches have been landed. Closing bug.

Shinya Kawanaka

Comment 35 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?

Hajime Morrita

Comment 36 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.

Antti Koivisto

Comment 37 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.

Shinya Kawanaka

Comment 38 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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Shinya Kawanaka

Reported

2012-10-30 02:28 PDT

Modified

2012-11-27 18:04 PST History

CC List

9 users Show

URL

Keywords

Depends on

Blocks

100451 103474

Dependencies

tree graph