Changeset 135174 in webkit


Ignore:
Timestamp:
Nov 19, 2012 10:14:04 AM (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
to invalidate distribution only if necessary.

For the code that 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.

Since attributeChanged() is a hot method, we don't want to make it slow. So we made one function to determine
whether we have to invalidate distribution, and make it called only if necessary. Also, we've optimized
shadowOfParentForDistribution() by making isInsertionPoint() de-virtualed. We consuded NodeFlags (IsInsertionPointFlag)
for this purpose.

We've measured how this patch makes changing attribute slow. I've measured each code 3 times.
DOM/ModifyAttribute.html is a micro benchmark which changes attribute a lot of times. The result of this benchmark
will be the most affected by this patch. However, it's only 2% performance regression.

DOM/ModifyAttribute.html
Before this patch:

median stdev min max [ms]

1st 494.0 3.36 490.0 502.0
2nd 503.5 3.44 497.0 512.0
3rd 494.0 3.48 488.0 499.0

After this patch:

median stdev min max [ms]

1st 504.0 2.00 501.0 509.0
2nd 505.5 3.08 500.0 513.0
3rd 507.0 2.32 502.0 510.0

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::hasSelectForClass returns true.
(WebCore::checkFunctorForClassChange):
(WebCore::checkNeedsStyleInvalidationForClassChange):
(WebCore::checkNeedsDistributionInvalidationForClassChange): Extracted the implementation to checkFunctorForClassChange.
(WebCore::Element::shouldInvalidateDistributionWhenAttributeChanged):

  • dom/Element.h:

(Element):

  • dom/Node.h:

(WebCore::Node::isInsertionPoint):

  • html/HTMLElement.h:

(HTMLElement):

  • html/shadow/InsertionPoint.cpp:

(WebCore::InsertionPoint::InsertionPoint):

  • html/shadow/InsertionPoint.h:

(InsertionPoint):
(WebCore::isInsertionPoint):
(WebCore::shadowOfParentForDistribution):
(WebCore::resolveReprojection):

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
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r135172 r135174  
     12012-11-19  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-19  Nate Chapin  <japhet@chromium.org>
    224

  • trunk/PerformanceTests/ChangeLog

    r134637 r135174  
     12012-11-19  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-14  Ryosuke Niwa  <rniwa@webkit.org>
    218

  • trunk/Source/WebCore/ChangeLog

    r135172 r135174  
     12012-11-19  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        to invalidate distribution only if necessary.
     11
     12        For the code that className is changed, we can share a lot of code between invalidating distribution and
     13        invalidating style. So we made checkNeedsStyleInvalidationForClassChange a template method, and share it.
     14
     15        Since attributeChanged() is a hot method, we don't want to make it slow. So we made one function to determine
     16        whether we have to invalidate distribution, and make it called only if necessary. Also, we've optimized
     17        shadowOfParentForDistribution() by making isInsertionPoint() de-virtualed. We consuded NodeFlags (IsInsertionPointFlag)
     18        for this purpose.
     19
     20        We've measured how this patch makes changing attribute slow. I've measured each code 3 times.
     21        DOM/ModifyAttribute.html is a micro benchmark which changes attribute a lot of times. The result of this benchmark
     22        will be the most affected by this patch. However, it's only 2% performance regression.
     23
     24        DOM/ModifyAttribute.html
     25        Before this patch:
     26                median  stdev    min    max    [ms]
     27          1st    494.0   3.36  490.0  502.0
     28          2nd    503.5   3.44  497.0  512.0
     29          3rd    494.0   3.48  488.0  499.0
     30
     31        After this patch:
     32                median  stdev  min      max    [ms]
     33          1st    504.0   2.00  501.0  509.0
     34          2nd    505.5   3.08  500.0  513.0
     35          3rd    507.0   2.32  502.0  510.0
     36
     37        Tests: fast/dom/shadow/distribution-attribute-modified.html
     38               fast/dom/shadow/distribution-className-modified.html
     39               fast/dom/shadow/distribution-id-modified.html
     40               fast/dom/shadow/reprojection-attribute-modified.html
     41               fast/dom/shadow/reprojection-className-modified.html
     42               fast/dom/shadow/reprojection-id-modified.html
     43
     44        * dom/Element.cpp:
     45        (WebCore::Element::attributeChanged):
     46        (WebCore::HasSelectorForClassStyleFunctor::HasSelectorForClassStyleFunctor):
     47        (HasSelectorForClassStyleFunctor):
     48        (WebCore::HasSelectorForClassStyleFunctor::operator()): Returns true if StyleResolver::hasSelectorForClass returns true.
     49        (WebCore):
     50        (WebCore::HasSelectorForClassDistributionFunctor::HasSelectorForClassDistributionFunctor):
     51        (HasSelectorForClassDistributionFunctor):
     52        (WebCore::HasSelectorForClassDistributionFunctor::operator()): Returns true if ElementShadow::hasSelectForClass returns true.
     53        (WebCore::checkFunctorForClassChange):
     54        (WebCore::checkNeedsStyleInvalidationForClassChange):
     55        (WebCore::checkNeedsDistributionInvalidationForClassChange): Extracted the implementation to checkFunctorForClassChange.
     56        (WebCore::Element::shouldInvalidateDistributionWhenAttributeChanged):
     57        * dom/Element.h:
     58        (Element):
     59        * dom/Node.h:
     60        (WebCore::Node::isInsertionPoint):
     61        * html/HTMLElement.h:
     62        (HTMLElement):
     63        * html/shadow/InsertionPoint.cpp:
     64        (WebCore::InsertionPoint::InsertionPoint):
     65        * html/shadow/InsertionPoint.h:
     66        (InsertionPoint):
     67        (WebCore::isInsertionPoint):
     68        (WebCore::shadowOfParentForDistribution):
     69        (WebCore::resolveReprojection):
     70
    1712012-11-19  Nate Chapin  <japhet@chromium.org>
    272

  • trunk/Source/WebCore/dom/Element.cpp

    r135101 r135174  
    5454#include "HTMLParserIdioms.h"
    5555#include "HTMLTableRowsCollection.h"
     56#include "InsertionPoint.h"
    5657#include "InspectorInstrumentation.h"
    5758#include "MutationObserverInterestGroup.h"
    … …  
    759760void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue)
    760761{
     762    if (ElementShadow* parentElementShadow = shadowOfParentForDistribution(this)) {
     763        if (shouldInvalidateDistributionWhenAttributeChanged(parentElementShadow, name, newValue))
     764            parentElementShadow->invalidateDistribution();
     765    }
     766
    761767    parseAttribute(name, newValue);
    762768
    … …  
    819825}
    820826
    821 static bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& changedClasses, StyleResolver* styleResolver)
     827struct HasSelectorForClassStyleFunctor {
     828    explicit HasSelectorForClassStyleFunctor(StyleResolver* resolver)
     829        : styleResolver(resolver)
     830    { }
     831
     832    bool operator()(const AtomicString& className) const
     833    {
     834        return styleResolver->hasSelectorForClass(className);
     835    }
     836
     837    StyleResolver* styleResolver;
     838};
     839
     840struct HasSelectorForClassDistributionFunctor {
     841    explicit HasSelectorForClassDistributionFunctor(ElementShadow* elementShadow)
     842        : elementShadow(elementShadow)
     843    { }
     844
     845    bool operator()(const AtomicString& className) const
     846    {
     847        return elementShadow->selectRuleFeatureSet().hasSelectorForClass(className);
     848    }
     849
     850    ElementShadow* elementShadow;
     851};
     852
     853template<typename Functor>
     854static bool checkFunctorForClassChange(const SpaceSplitString& changedClasses, Functor functor)
    822855{
    823856    unsigned changedSize = changedClasses.size();
    824857    for (unsigned i = 0; i < changedSize; ++i) {
    825         if (styleResolver->hasSelectorForClass(changedClasses[i]))
     858        if (functor(changedClasses[i]))
    826859            return true;
    827860    }
    … …  
    829862}
    830863
    831 static bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, StyleResolver* styleResolver)
     864template<typename Functor>
     865static bool checkFunctorForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, Functor functor)
    832866{
    833867    unsigned oldSize = oldClasses.size();
    834868    if (!oldSize)
    835         return checkNeedsStyleInvalidationForClassChange(newClasses, styleResolver);
     869        return checkFunctorForClassChange(newClasses, functor);
    836870    BitVector remainingClassBits;
    837871    remainingClassBits.ensureSize(oldSize);
    … …  
    845879            }
    846880        }
    847         if (styleResolver->hasSelectorForClass(newClasses[i]))
     881        if (functor(newClasses[i]))
    848882            return true;
    849883    }
    … …  
    852886        if (remainingClassBits.quickGet(i))
    853887            continue;
    854         if (styleResolver->hasSelectorForClass(oldClasses[i]))
     888        if (functor(oldClasses[i]))
    855889            return true;
    856890    }
    857891    return false;
     892}
     893
     894static inline bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& changedClasses, StyleResolver* styleResolver)
     895{
     896    return checkFunctorForClassChange(changedClasses, HasSelectorForClassStyleFunctor(styleResolver));
     897}
     898
     899static inline bool checkNeedsStyleInvalidationForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, StyleResolver* styleResolver)
     900{
     901    return checkFunctorForClassChange(oldClasses, newClasses, HasSelectorForClassStyleFunctor(styleResolver));
     902}
     903
     904static inline bool checkNeedsDistributionInvalidationForClassChange(const SpaceSplitString& changedClasses, ElementShadow* elementShadow)
     905{
     906    return checkFunctorForClassChange(changedClasses, HasSelectorForClassDistributionFunctor(elementShadow));
     907}
     908
     909static inline bool checkNeedsDistributionInvalidationForClassChange(const SpaceSplitString& oldClasses, const SpaceSplitString& newClasses, ElementShadow* elementShadow)
     910{
     911    return checkFunctorForClassChange(oldClasses, newClasses, HasSelectorForClassDistributionFunctor(elementShadow));
    858912}
    859913
    … …  
    885939    if (shouldInvalidateStyle)
    886940        setNeedsStyleRecalc();
     941}
     942
     943bool Element::shouldInvalidateDistributionWhenAttributeChanged(ElementShadow* elementShadow, const QualifiedName& name, const AtomicString& newValue)
     944{
     945    ASSERT(elementShadow);
     946    elementShadow->ensureSelectFeatureSetCollected();
     947
     948    if (isIdAttributeName(name)) {
     949        AtomicString oldId = attributeData()->idForStyleResolution();
     950        AtomicString newId = makeIdForStyleResolution(newValue, document()->inQuirksMode());
     951        if (newId != oldId) {
     952            if (!oldId.isEmpty() && elementShadow->selectRuleFeatureSet().hasSelectorForId(oldId))
     953                return true;
     954            if (!newId.isEmpty() && elementShadow->selectRuleFeatureSet().hasSelectorForId(newId))
     955                return true;
     956        }
     957    }
     958
     959    if (name == HTMLNames::classAttr) {
     960        const AtomicString& newClassString = newValue;
     961        if (classStringHasClassName(newClassString)) {
     962            const ElementAttributeData* attributeData = ensureAttributeData();
     963            const bool shouldFoldCase = document()->inQuirksMode();
     964            const SpaceSplitString& oldClasses = attributeData->classNames();
     965            const SpaceSplitString newClasses(newClassString, shouldFoldCase);
     966            if (checkNeedsDistributionInvalidationForClassChange(oldClasses, newClasses, elementShadow))
     967                return true;
     968        } else if (const ElementAttributeData* attributeData = this->attributeData()) {
     969            const SpaceSplitString& oldClasses = attributeData->classNames();
     970            if (checkNeedsDistributionInvalidationForClassChange(oldClasses, elementShadow))
     971                return true;
     972        }
     973    }
     974
     975    return elementShadow->selectRuleFeatureSet().hasSelectorForAttribute(name.localName());
    887976}
    888977

  • trunk/Source/WebCore/dom/Element.h

    r135101 r135174  
    524524    void createMutableAttributeData();
    525525
     526    bool shouldInvalidateDistributionWhenAttributeChanged(ElementShadow*, const QualifiedName&, const AtomicString&);
     527
    526528private:
    527529    ElementRareData* elementRareData() const;

  • trunk/Source/WebCore/dom/Node.h

    r135030 r135174  
    236236    bool isDocumentNode() const;
    237237    bool isShadowRoot() const { return getFlag(IsShadowRootFlag); }
     238    bool isInsertionPoint() const { return getFlag(IsInsertionPointFlag); }
    238239    bool inNamedFlow() const { return getFlag(InNamedFlowFlag); }
    239240    bool hasCustomCallbacks() const { return getFlag(HasCustomCallbacksFlag); }
    … …  
    725726        HasEventTargetDataFlag = 1 << 27,
    726727        InEdenFlag = 1 << 28,
     728        IsInsertionPointFlag = 1 << 29,
    727729
    728730#if ENABLE(SVG)
    … …  
    733735    };
    734736
    735     // 3 bits remaining
     737    // 2 bits remaining
    736738
    737739    bool getFlag(NodeFlags mask) const { return m_nodeFlags & mask; }
    … …  
    751753        CreateFrameOwnerElement = CreateHTMLElement | HasCustomCallbacksFlag,
    752754        CreateSVGElement = CreateStyledElement | IsSVGFlag,
    753         CreateDocument = CreateContainer | InDocumentFlag
     755        CreateDocument = CreateContainer | InDocumentFlag,
     756        CreateInsertionPoint = CreateHTMLElement | IsInsertionPointFlag
    754757    };
    755758    Node(Document*, ConstructionType);

  • trunk/Source/WebCore/html/HTMLElement.h

    r135069 r135174  
    107107#endif
    108108
    109     virtual bool isInsertionPoint() const { return false; }
    110109    virtual bool isLabelable() const { return false; }
    111110

  • trunk/Source/WebCore/html/shadow/InsertionPoint.cpp

    r132174 r135174  
    3939
    4040InsertionPoint::InsertionPoint(const QualifiedName& tagName, Document* document)
    41     : HTMLElement(tagName, document)
     41    : HTMLElement(tagName, document, CreateInsertionPoint)
    4242    , m_shouldResetStyleInheritance(false)
    4343{

  • trunk/Source/WebCore/html/shadow/InsertionPoint.h

    r133992 r135174  
    6363    virtual void attach();
    6464    virtual void detach();
    65     virtual bool isInsertionPoint() const OVERRIDE { return true; }
    6665
    6766    bool shouldUseFallbackElements() const;
    … …  
    9089inline bool isInsertionPoint(const Node* node)
    9190{
    92     if (node->isHTMLElement() && toHTMLElement(node)->isInsertionPoint())
    93         return true;
    94 
    95     return false;
     91    ASSERT(node);
     92    return node->isInsertionPoint();
    9693}
    9794
    … …  
    145142inline ElementShadow* shadowOfParentForDistribution(const Node* node)
    146143{
    147     if (!node)
    148         return 0;
    149 
     144    ASSERT(node);
    150145    if (Element* parent = parentElementForDistribution(node))
    151146        return parent->shadow();
    … …  
    159154    const Node* current = projectedNode;
    160155
    161     while (true) {
     156    while (current) {
    162157        if (ElementShadow* shadow = shadowOfParentForDistribution(current)) {
    163158            shadow->ensureDistribution();
Note: See TracChangeset for help on using the changeset viewer.