When we change className dynamically, this does not reflect to distribution.
Created attachment 171409 [details] Patch
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 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?
Ah, yeah... I've forgot the existence of attribute selector like input[type="something"}... maybe we should check selectors when we invalidate the distribution?
(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.
Created attachment 173192 [details] WIP
The previous WIP patch contains patches for Bug 101692 and Bug 101180.
Created attachment 173551 [details] Patch
Comment on attachment 173551 [details] Patch This will conflict with Bug 101697.
Oops, not Bug 101697 but Bug 101891.
Is there any performance impact on this change?
(In reply to comment #11) > Is there any performance impact on this change? Let me measure it.
(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%...
Created attachment 173803 [details] Patch
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 :)
(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.
Created attachment 173807 [details] Patch
Comment on attachment 173807 [details] Patch Clearing flags on attachment: 173807 Committed r134367: <http://trac.webkit.org/changeset/134367>
All reviewed patches have been landed. Closing bug.
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
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?
(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 :)
Rolled out in http://trac.webkit.org/changeset/134443/ to speculate.
(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?
(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...
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?
(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.
Created attachment 174622 [details] Patch
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.
(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.
Created attachment 174888 [details] Patch
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 on attachment 174888 [details] Patch Clearing flags on attachment: 174888 Committed r135174: <http://trac.webkit.org/changeset/135174>
(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?
(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 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.
Antti, Sorry for late response. I'll try to make it simple. https://bugs.webkit.org/show_bug.cgi?id=103474