Changeset 134367 in webkit


Ignore:
Timestamp:
Nov 12, 2012 10:50:15 PM (11 years ago)
Author:
shinyak@chromium.org
Message:

Changing id, className, or attribute should invalidate distribution
https://bugs.webkit.org/show_bug.cgi?id=100738

Reviewed by Dimitri Glazkov.

PerformanceTests:

Added test code to modify id/class/attribute.

  • DOM/ModifyAttribute.html: Added.
  • DOM/resources/dom-perf/modify-attribute.js: Added.

(ModifyAttribute.CreateElementToSetUp):
(ModifyAttribute.ModifyId):
(ModifyAttribute.ModifyClass):
(ModifyAttribute.ModifyTitle):

Source/WebCore:

When id, className, or attribute is changed, we might have to invalidate distribution.
However, we don't want to do useless invalidation. So we consult with the RuleFeatureSet of ElementShadow,
and invalidate distribution only if necessary.

When className is changed, we can share a lot of code between invalidating distribution and invalidating style.
So we made checkNeedsStyleInvalidationForClassChange a template method, and share it.

Also we've measured how this patch makes changing attribute slow. By converting checkNeedsStyleInvalidationForClassChange
to template, actually this improves the performance of changing attribute code. I've measured each code 3 times.

DOM/ModifyAttribute.html
Before this patch:

median stdev min max [ms]

1st 115.62 0.67 114.75 117.00
2nd 115.08 1.13 113.25 117.58
3rd 114.75 1.16 113.42 117.83

After this patch:

median stdev min max [ms]

1st 102.55 0.95 101.25 104.50
2nd 103.10 0.86 102.20 100.95
3rd 103.30 1.05 102.10 106.65

Tests: fast/dom/shadow/distribution-attribute-modified.html

fast/dom/shadow/distribution-className-modified.html
fast/dom/shadow/distribution-id-modified.html
fast/dom/shadow/reprojection-attribute-modified.html
fast/dom/shadow/reprojection-className-modified.html
fast/dom/shadow/reprojection-id-modified.html

  • dom/Element.cpp:

(WebCore::Element::attributeChanged):
(WebCore::HasSelectorForClassStyleFunctor::HasSelectorForClassStyleFunctor):
(HasSelectorForClassStyleFunctor):
(WebCore::HasSelectorForClassStyleFunctor::operator()): Returns true if StyleResolver::hasSelectorForClass returns true.
(WebCore):
(WebCore::HasSelectorForClassDistributionFunctor::HasSelectorForClassDistributionFunctor):
(HasSelectorForClassDistributionFunctor):
(WebCore::HasSelectorForClassDistributionFunctor::operator()): Returns true if ElementShadow::hasSelectorForClass returns true.
(WebCore::checkFunctorForClassChange):
(WebCore::checkNeedsStyleInvalidationForClassChange): Extacted the implementation to checkFunctorForClassChange.
(WebCore::checkNeedsDistributionInvalidationForClassChange):
(WebCore::Element::classAttributeChanged):

LayoutTests:

We have test cases that id/class/attribute is changed, and thier reprojection cases.

  • fast/dom/shadow/distribution-attribute-modified-expected.html: Added.
  • fast/dom/shadow/distribution-attribute-modified.html: Added.
  • fast/dom/shadow/distribution-className-modified-expected.html: Added.
  • fast/dom/shadow/distribution-className-modified.html: Added.
  • fast/dom/shadow/distribution-id-modified-expected.html: Added.
  • fast/dom/shadow/distribution-id-modified.html: Added.
  • fast/dom/shadow/reprojection-attribute-modified-expected.html: Added.
  • fast/dom/shadow/reprojection-attribute-modified.html: Added.
  • fast/dom/shadow/reprojection-className-modified-expected.html: Added.
  • fast/dom/shadow/reprojection-className-modified.html: Added.
  • fast/dom/shadow/reprojection-id-modified-expected.html: Added.
  • fast/dom/shadow/reprojection-id-modified.html: Added.
Location:
trunk
Files:
14 added
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r134352 r134367  
     12012-11-12  Shinya Kawanaka  <shinyak@chromium.org>
     2
     3        Changing id, className, or attribute should invalidate distribution
     4        https://bugs.webkit.org/show_bug.cgi?id=100738
     5
     6        Reviewed by Dimitri Glazkov.
     7
     8        We have test cases that id/class/attribute is changed, and thier reprojection cases.
     9
     10        * fast/dom/shadow/distribution-attribute-modified-expected.html: Added.
     11        * fast/dom/shadow/distribution-attribute-modified.html: Added.
     12        * fast/dom/shadow/distribution-className-modified-expected.html: Added.
     13        * fast/dom/shadow/distribution-className-modified.html: Added.
     14        * fast/dom/shadow/distribution-id-modified-expected.html: Added.
     15        * fast/dom/shadow/distribution-id-modified.html: Added.
     16        * fast/dom/shadow/reprojection-attribute-modified-expected.html: Added.
     17        * fast/dom/shadow/reprojection-attribute-modified.html: Added.
     18        * fast/dom/shadow/reprojection-className-modified-expected.html: Added.
     19        * fast/dom/shadow/reprojection-className-modified.html: Added.
     20        * fast/dom/shadow/reprojection-id-modified-expected.html: Added.
     21        * fast/dom/shadow/reprojection-id-modified.html: Added.
     22
    1232012-11-10  Dirk Schulze  <krit@webkit.org>
    224
  • trunk/PerformanceTests/ChangeLog

    r133657 r134367  
     12012-11-12  Shinya Kawanaka  <shinyak@chromium.org>
     2
     3        Changing id, className, or attribute should invalidate distribution
     4        https://bugs.webkit.org/show_bug.cgi?id=100738
     5
     6        Reviewed by Dimitri Glazkov.
     7
     8        Added test code to modify id/class/attribute.
     9
     10        * DOM/ModifyAttribute.html: Added.
     11        * DOM/resources/dom-perf/modify-attribute.js: Added.
     12        (ModifyAttribute.CreateElementToSetUp):
     13        (ModifyAttribute.ModifyId):
     14        (ModifyAttribute.ModifyClass):
     15        (ModifyAttribute.ModifyTitle):
     16
    1172012-11-06  Adam Barth  <abarth@webkit.org>
    218
  • trunk/Source/WebCore/ChangeLog

    r134364 r134367  
     12012-11-12  Shinya Kawanaka  <shinyak@chromium.org>
     2
     3        Changing id, className, or attribute should invalidate distribution
     4        https://bugs.webkit.org/show_bug.cgi?id=100738
     5
     6        Reviewed by Dimitri Glazkov.
     7
     8        When id, className, or attribute is changed, we might have to invalidate distribution.
     9        However, we don't want to do useless invalidation. So we consult with the RuleFeatureSet of ElementShadow,
     10        and invalidate distribution only if necessary.
     11
     12        When className is changed, we can share a lot of code between invalidating distribution and invalidating style.
     13        So we made checkNeedsStyleInvalidationForClassChange a template method, and share it.
     14
     15        Also we've measured how this patch makes changing attribute slow. By converting checkNeedsStyleInvalidationForClassChange
     16        to template, actually this improves the performance of changing attribute code. I've measured each code 3 times.
     17
     18        DOM/ModifyAttribute.html
     19        Before this patch:
     20                median  stdev  min      max    [ms]
     21          1st   115.62   0.67  114.75   117.00
     22          2nd   115.08   1.13  113.25   117.58
     23          3rd   114.75   1.16  113.42   117.83
     24
     25        After this patch:
     26                median  stdev  min      max    [ms]
     27          1st   102.55   0.95  101.25   104.50
     28          2nd   103.10   0.86  102.20   100.95
     29          3rd   103.30   1.05  102.10   106.65
     30
     31        Tests: fast/dom/shadow/distribution-attribute-modified.html
     32               fast/dom/shadow/distribution-className-modified.html
     33               fast/dom/shadow/distribution-id-modified.html
     34               fast/dom/shadow/reprojection-attribute-modified.html
     35               fast/dom/shadow/reprojection-className-modified.html
     36               fast/dom/shadow/reprojection-id-modified.html
     37
     38        * dom/Element.cpp:
     39        (WebCore::Element::attributeChanged):
     40        (WebCore::HasSelectorForClassStyleFunctor::HasSelectorForClassStyleFunctor):
     41        (HasSelectorForClassStyleFunctor):
     42        (WebCore::HasSelectorForClassStyleFunctor::operator()): Returns true if StyleResolver::hasSelectorForClass returns true.
     43        (WebCore):
     44        (WebCore::HasSelectorForClassDistributionFunctor::HasSelectorForClassDistributionFunctor):
     45        (HasSelectorForClassDistributionFunctor):
     46        (WebCore::HasSelectorForClassDistributionFunctor::operator()): Returns true if ElementShadow::hasSelectorForClass returns true.
     47        (WebCore::checkFunctorForClassChange):
     48        (WebCore::checkNeedsStyleInvalidationForClassChange): Extacted the implementation to checkFunctorForClassChange.
     49        (WebCore::checkNeedsDistributionInvalidationForClassChange):
     50        (WebCore::Element::classAttributeChanged):
     51
    1522012-11-12  Joe Mason  <jmason@rim.com>
    253
  • trunk/Source/WebCore/dom/Element.cpp

    r134163 r134367  
    5454#include "HTMLParserIdioms.h"
    5555#include "HTMLTableRowsCollection.h"
     56#include "InsertionPoint.h"
    5657#include "InspectorInstrumentation.h"
    5758#include "MutationObserverInterestGroup.h"
     
    764765
    765766    StyleResolver* styleResolver = document()->styleResolverIfExists();
     767    ElementShadow* elementShadow = shadowOfParentForDistribution(this);
     768    if (elementShadow)
     769        elementShadow->ensureSelectFeatureSetCollected();
     770
    766771    bool testShouldInvalidateStyle = attached() && styleResolver && styleChangeType() < FullStyleChange;
    767772    bool shouldInvalidateStyle = false;
     773    bool shouldInvalidateDistribution = false;
    768774
    769775    if (isIdAttributeName(name)) {
     
    773779            attributeData()->setIdForStyleResolution(newId);
    774780            shouldInvalidateStyle = testShouldInvalidateStyle && checkNeedsStyleInvalidationForIdChange(oldId, newId, styleResolver);
     781            shouldInvalidateDistribution |= elementShadow && !oldId.isEmpty() && elementShadow->selectRuleFeatureSet().hasSelectorForId(oldId);
     782            shouldInvalidateDistribution |= elementShadow && !newId.isEmpty() && elementShadow->selectRuleFeatureSet().hasSelectorForId(newId); 
    775783        }
    776784    } else if (name == HTMLNames::nameAttr)
     
    780788
    781789    shouldInvalidateStyle |= testShouldInvalidateStyle && styleResolver->hasSelectorForAttribute(name.localName());
     790    shouldInvalidateDistribution |= elementShadow && elementShadow->selectRuleFeatureSet().hasSelectorForAttribute(name.localName());
    782791
    783792    invalidateNodeListCachesInAncestors(&name, this);
     
    785794    if (shouldInvalidateStyle)
    786795        setNeedsStyleRecalc();
    787 
     796    if (shouldInvalidateDistribution)
     797        elementShadow->invalidateDistribution();
     798       
    788799    if (AXObjectCache::accessibilityEnabled())
    789800        document()->axObjectCache()->handleAttributeChanged(name, this);
     
    823834}
    824835
    825 static bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& changedClasses, StyleResolver* styleResolver)
     836struct HasSelectorForClassStyleFunctor {
     837    explicit HasSelectorForClassStyleFunctor(StyleResolver* resolver)
     838        : styleResolver(resolver)
     839    { }
     840
     841    bool operator()(const AtomicString& className) const
     842    {
     843        return styleResolver->hasSelectorForClass(className);
     844    }
     845
     846    StyleResolver* styleResolver;
     847};
     848
     849struct HasSelectorForClassDistributionFunctor {
     850    explicit HasSelectorForClassDistributionFunctor(ElementShadow* elementShadow)
     851        : elementShadow(elementShadow)
     852    { }
     853
     854    bool operator()(const AtomicString& className) const
     855    {
     856        return elementShadow->selectRuleFeatureSet().hasSelectorForClass(className);
     857    }
     858
     859    ElementShadow* elementShadow;
     860};
     861
     862template<typename Functor>
     863static bool checkFunctorForClassChange(const SpaceSplitString& changedClasses, Functor functor)
    826864{
    827865    unsigned changedSize = changedClasses.size();
    828866    for (unsigned i = 0; i < changedSize; ++i) {
    829         if (styleResolver->hasSelectorForClass(changedClasses[i]))
     867        if (functor(changedClasses[i]))
    830868            return true;
    831869    }
     
    833871}
    834872
    835 static bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, StyleResolver* styleResolver)
     873template<typename Functor>
     874static bool checkFunctorForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, Functor functor)
    836875{
    837876    unsigned oldSize = oldClasses.size();
    838877    if (!oldSize)
    839         return checkNeedsStyleInvalidationForClassChange(newClasses, styleResolver);
     878        return checkFunctorForClassChange(newClasses, functor);
    840879    BitVector remainingClassBits;
    841880    remainingClassBits.ensureSize(oldSize);
     
    849888            }
    850889        }
    851         if (styleResolver->hasSelectorForClass(newClasses[i]))
     890        if (functor(newClasses[i]))
    852891            return true;
    853892    }
     
    856895        if (remainingClassBits.quickGet(i))
    857896            continue;
    858         if (styleResolver->hasSelectorForClass(oldClasses[i]))
     897        if (functor(oldClasses[i]))
    859898            return true;
    860899    }
    861900    return false;
     901}
     902
     903static inline bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& changedClasses, StyleResolver* styleResolver)
     904{
     905    return checkFunctorForClassChange(changedClasses, HasSelectorForClassStyleFunctor(styleResolver));
     906}
     907
     908static inline bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, StyleResolver* styleResolver)
     909{
     910    return checkFunctorForClassChange(oldClasses, newClasses, HasSelectorForClassStyleFunctor(styleResolver));
     911}
     912
     913static inline bool checkNeedsDistributionInvalidationForClassChange(const SpaceSplitString& changedClasses, ElementShadow* elementShadow)
     914{
     915    return checkFunctorForClassChange(changedClasses, HasSelectorForClassDistributionFunctor(elementShadow));
     916}
     917
     918static inline bool checkNeedsDistributionInvalidationForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, ElementShadow* elementShadow)
     919{
     920    return checkFunctorForClassChange(oldClasses, newClasses, HasSelectorForClassDistributionFunctor(elementShadow));
    862921}
    863922
     
    867926    bool testShouldInvalidateStyle = attached() && styleResolver && styleChangeType() < FullStyleChange;
    868927    bool shouldInvalidateStyle = false;
     928
     929    ElementShadow* elementShadow = shadowOfParentForDistribution(this);
     930    if (elementShadow)
     931        elementShadow->ensureSelectFeatureSetCollected();
     932    bool shouldInvalidateDistribution = false;
    869933
    870934    if (classStringHasClassName(newClassString)) {
     
    877941        const SpaceSplitString& newClasses = attributeData->classNames();
    878942        shouldInvalidateStyle = testShouldInvalidateStyle && checkNeedsStyleInvalidationForClassChange(oldClasses, newClasses, styleResolver);
     943        shouldInvalidateDistribution = elementShadow && checkNeedsDistributionInvalidationForClassChange(oldClasses, newClasses, elementShadow);
    879944    } else if (const ElementAttributeData* attributeData = this->attributeData()) {
    880945        const SpaceSplitString& oldClasses = attributeData->classNames();
    881946        shouldInvalidateStyle = testShouldInvalidateStyle && checkNeedsStyleInvalidationForClassChange(oldClasses, styleResolver);
    882 
     947        shouldInvalidateDistribution = elementShadow && checkNeedsDistributionInvalidationForClassChange(oldClasses, elementShadow);
    883948        attributeData->clearClass();
    884949    }
     
    889954    if (shouldInvalidateStyle)
    890955        setNeedsStyleRecalc();
     956    if (shouldInvalidateDistribution)
     957        elementShadow->invalidateDistribution();
    891958}
    892959
Note: See TracChangeset for help on using the changeset viewer.