Changeset 201159 in webkit


Ignore:
Timestamp:
May 19, 2016 6:22:18 AM (8 years ago)
Author:
Antti Koivisto
Message:

Style resolution for explicitly inherited properties is inefficient
https://bugs.webkit.org/show_bug.cgi?id=157860

Reviewed by Andreas Kling.

Source/WebCore:

We mark the parent style with hasExplicitlyInheritedProperties bit rather than the style that is actually
affected by inherited properties. This leads to various inefficiencies including unnecessarily wide style recalcs.

  • css/StyleResolver.cpp:

(WebCore::isCacheableInMatchedPropertiesCache):

Check the style itself rather than the parent. This allows more caching.

(WebCore::StyleResolver::applyProperty):

Mark the style rather than the parent.

  • style/StyleChange.cpp:

(WebCore::Style::determineChange):

Remove hasExplicitlyInheritedProperties test and just return NoInherit. Having explicitly inherited
properties doesn't make the children inherit them automatically.

This allows smaller style recalcs.

  • style/StyleTreeResolver.cpp:

(WebCore::Style::TreeResolver::resolveElement):

This patch exposed a bug with appearance property in meter and progress elements.
They may construct different renderer based on appearance so we need to force
render tree reconstruction when it changes.

(WebCore::Style::TreeResolver::popParentsToDepth):
(WebCore::Style::shouldResolvePseudoElement):

Don't clear the style recalc bits here.

(WebCore::Style::shouldResolveElement):

Add a helper.
If the parent had a NoInherit style change, test if the element has existing style with
hasExplicitlyInheritedProperties bit. If so we need to re-resolve this element.

(WebCore::Style::clearNeedsStyleResolution):

Also clear pseudo elements.

(WebCore::Style::TreeResolver::resolveComposedTree):

LayoutTests:

  • platform/ios-simulator/fast/dom/HTMLProgressElement/progress-bar-value-pseudo-element-expected.txt:
  • platform/mac/fast/dom/HTMLProgressElement/progress-bar-value-pseudo-element-expected.txt:

This is a progression.

Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r201130 r201159  
     12016-05-18  Antti Koivisto  <antti@apple.com>
     2
     3        Style resolution for explicitly inherited properties is inefficient
     4        https://bugs.webkit.org/show_bug.cgi?id=157860
     5
     6        Reviewed by Andreas Kling.
     7
     8        * platform/ios-simulator/fast/dom/HTMLProgressElement/progress-bar-value-pseudo-element-expected.txt:
     9        * platform/mac/fast/dom/HTMLProgressElement/progress-bar-value-pseudo-element-expected.txt:
     10
     11        This is a progression.
     12
    1132016-05-19  Yoav Weiss  <yoav@yoav.ws>
    214
  • trunk/LayoutTests/platform/ios-simulator/fast/dom/HTMLProgressElement/progress-bar-value-pseudo-element-expected.txt

    r179104 r201159  
    5757          RenderText {#text} at (0,0) size 235x19
    5858            text run at (0,0) width 235: "Removing appearance dynamically: "
    59           RenderProgress {PROGRESS} at (234,2) size 161x17
     59          RenderBlock {PROGRESS} at (234,2) size 161x17
    6060            RenderProgress {DIV} at (0,0) size 160x16
    6161              RenderBlock {DIV} at (0,0) size 160x16 [bgcolor=#808080]
  • trunk/LayoutTests/platform/mac/fast/dom/HTMLProgressElement/progress-bar-value-pseudo-element-expected.txt

    r177774 r201159  
    5757          RenderText {#text} at (0,0) size 235x18
    5858            text run at (0,0) width 235: "Removing appearance dynamically: "
    59           RenderProgress {PROGRESS} at (234,1) size 161x17
     59          RenderBlock {PROGRESS} at (234,1) size 161x17
    6060            RenderProgress {DIV} at (0,0) size 160x16
    6161              RenderBlock {DIV} at (0,0) size 160x16 [bgcolor=#808080]
  • trunk/Source/WebCore/ChangeLog

    r201129 r201159  
     12016-05-18  Antti Koivisto  <antti@apple.com>
     2
     3        Style resolution for explicitly inherited properties is inefficient
     4        https://bugs.webkit.org/show_bug.cgi?id=157860
     5
     6        Reviewed by Andreas Kling.
     7
     8        We mark the parent style with hasExplicitlyInheritedProperties bit rather than the style that is actually
     9        affected by inherited properties. This leads to various inefficiencies including unnecessarily wide style recalcs.
     10
     11        * css/StyleResolver.cpp:
     12        (WebCore::isCacheableInMatchedPropertiesCache):
     13
     14            Check the style itself rather than the parent. This allows more caching.
     15
     16        (WebCore::StyleResolver::applyProperty):
     17
     18            Mark the style rather than the parent.
     19
     20        * style/StyleChange.cpp:
     21        (WebCore::Style::determineChange):
     22
     23            Remove hasExplicitlyInheritedProperties test and just return NoInherit. Having explicitly inherited
     24            properties doesn't make the children inherit them automatically.
     25
     26            This allows smaller style recalcs.
     27
     28        * style/StyleTreeResolver.cpp:
     29        (WebCore::Style::TreeResolver::resolveElement):
     30
     31            This patch exposed a bug with appearance property in meter and progress elements.
     32            They may construct different renderer based on appearance so we need to force
     33            render tree reconstruction when it changes.
     34
     35        (WebCore::Style::TreeResolver::popParentsToDepth):
     36        (WebCore::Style::shouldResolvePseudoElement):
     37
     38            Don't clear the style recalc bits here.
     39
     40        (WebCore::Style::shouldResolveElement):
     41
     42            Add a helper.
     43            If the parent had a NoInherit style change, test if the element has existing style with
     44            hasExplicitlyInheritedProperties bit. If so we need to re-resolve this element.
     45
     46        (WebCore::Style::clearNeedsStyleResolution):
     47
     48            Also clear pseudo elements.
     49
     50        (WebCore::Style::TreeResolver::resolveComposedTree):
     51
    1522016-05-19  Youenn Fablet  <youenn.fablet@crf.canon.fr>
    253
  • trunk/Source/WebCore/css/StyleResolver.cpp

    r201113 r201159  
    12561256        return false;
    12571257    // The cache assumes static knowledge about which properties are inherited.
    1258     if (parentStyle->hasExplicitlyInheritedProperties())
     1258    if (style->hasExplicitlyInheritedProperties())
    12591259        return false;
    12601260    return true;
     
    16371637    }
    16381638
    1639     if (isInherit && !state.parentStyle()->hasExplicitlyInheritedProperties() && !CSSProperty::isInheritedProperty(id))
    1640         const_cast<RenderStyle*>(state.parentStyle())->setHasExplicitlyInheritedProperties();
     1639    if (isInherit && !CSSProperty::isInheritedProperty(id))
     1640        state.style()->setHasExplicitlyInheritedProperties();
    16411641   
    16421642    if (id == CSSPropertyCustom) {
  • trunk/Source/WebCore/style/StyleChange.cpp

    r194584 r201159  
    6464        if (s1.inheritedNotEqual(&s2))
    6565            return Inherit;
    66         if (s1.hasExplicitlyInheritedProperties() || s2.hasExplicitlyInheritedProperties())
    67             return Inherit;
    6866
    6967        return NoInherit;
  • trunk/Source/WebCore/style/StyleTreeResolver.cpp

    r200633 r201159  
    3333#include "ElementIterator.h"
    3434#include "HTMLBodyElement.h"
     35#include "HTMLMeterElement.h"
     36#include "HTMLProgressElement.h"
    3537#include "HTMLSlotElement.h"
    3638#include "LoaderStrategy.h"
     
    199201        update.isSynthetic = true;
    200202
     203    auto* existingStyle = element.renderStyle();
     204
    201205    if (&element == m_document.documentElement()) {
    202206        m_documentElementStyle = RenderStyle::clonePtr(*update.style);
     
    205209        // If "rem" units are used anywhere in the document, and if the document element's font size changes, then force font updating
    206210        // all the way down the tree. This is simpler than having to maintain a cache of objects (and such font size changes should be rare anyway).
    207         if (m_document.authorStyleSheets().usesRemUnits() && update.change != NoChange && element.renderer() && element.renderer()->style().fontSize() != update.style->fontSize()) {
     211        if (m_document.authorStyleSheets().usesRemUnits() && update.change != NoChange && existingStyle && existingStyle->fontSize() != update.style->fontSize()) {
    208212            // Cached RenderStyles may depend on the rem units.
    209213            scope().styleResolver.invalidateMatchedPropertiesCache();
     
    216220    if (&element == m_document.body())
    217221        m_document.setTextColor(update.style->visitedDependentColor(CSSPropertyColor));
     222
     223    // FIXME: These elements should not change renderer based on appearance property.
     224    if (is<HTMLMeterElement>(element) || is<HTMLProgressElement>(element)) {
     225        if (existingStyle && update.style->appearance() != existingStyle->appearance())
     226            update.change = Detach;
     227    }
    218228
    219229    if (update.change != Detach && (parent().change == Force || element.styleChangeType() >= FullStyleChange))
     
    342352}
    343353
    344 static bool shouldResolvePseudoElement(PseudoElement* pseudoElement)
     354static bool shouldResolvePseudoElement(const PseudoElement* pseudoElement)
    345355{
    346356    if (!pseudoElement)
    347357        return false;
    348     bool needsStyleRecalc = pseudoElement->needsStyleRecalc();
    349     pseudoElement->clearNeedsStyleRecalc();
    350     return needsStyleRecalc;
     358    return pseudoElement->needsStyleRecalc();
     359}
     360
     361static bool shouldResolveElement(const Element& element, Style::Change parentChange)
     362{
     363    if (parentChange >= Inherit)
     364        return true;
     365    if (parentChange == NoInherit) {
     366        auto* existingStyle = element.renderStyle();
     367        if (existingStyle && existingStyle->hasExplicitlyInheritedProperties())
     368            return true;
     369    }
     370    if (element.needsStyleRecalc())
     371        return true;
     372    if (element.hasDisplayContents())
     373        return true;
     374    if (shouldResolvePseudoElement(element.beforePseudoElement()))
     375        return true;
     376    if (shouldResolvePseudoElement(element.afterPseudoElement()))
     377        return true;
     378
     379    return false;
     380}
     381
     382static void clearNeedsStyleResolution(Element& element)
     383{
     384    element.clearNeedsStyleRecalc();
     385    if (auto* before = element.beforePseudoElement())
     386        before->clearNeedsStyleRecalc();
     387    if (auto* after = element.afterPseudoElement())
     388        after->clearNeedsStyleRecalc();
    351389}
    352390
     
    396434            parent.elementNeedingStyleRecalcAffectsNextSiblingElementStyle = element.affectsNextSiblingElementStyle();
    397435
    398         bool shouldResolveForPseudoElement = shouldResolvePseudoElement(element.beforePseudoElement()) || shouldResolvePseudoElement(element.afterPseudoElement());
    399 
    400         const RenderStyle* style;
    401         Change change;
    402 
    403         bool shouldResolve = parent.change >= Inherit || element.needsStyleRecalc() || shouldResolveForPseudoElement || affectedByPreviousSibling || element.hasDisplayContents();
     436        auto* style = element.renderStyle();
     437        auto change = NoChange;
     438
     439        bool shouldResolve = shouldResolveElement(element, parent.change) || affectedByPreviousSibling;
    404440        if (shouldResolve) {
    405441#if PLATFORM(IOS)
     
    429465                m_update->addElement(element, parent.element, WTFMove(elementUpdate));
    430466
    431             element.clearNeedsStyleRecalc();
    432         } else {
    433             style = element.renderStyle();
    434             change = NoChange;
     467            clearNeedsStyleResolution(element);
    435468        }
    436469
Note: See TracChangeset for help on using the changeset viewer.