Bug 100738

Summary: [Shadow DOM] Changing id, className, or attribute should invalidate distribution
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dglazkov, koivisto, macpherson, menard, morrita, ojan, webcomponents-bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 100451, 103474    
Attachments:
Description Flags
Patch
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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
DetailsFormatted DiffDiff
WIP (54.63 KB, patch)
2012-11-08 21:05 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (20.92 KB, patch)
2012-11-11 21:35 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (26.57 KB, patch)
2012-11-12 20:38 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (26.55 KB, patch)
2012-11-12 21:01 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (26.19 KB, patch)
2012-11-16 00:31 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (31.01 KB, patch)
2012-11-18 21:49 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
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.