2010-04-26 Maciej Stachowiak <mjs@apple.com>
authormjs@apple.com <mjs@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Apr 2010 00:09:58 +0000 (00:09 +0000)
committermjs@apple.com <mjs@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Apr 2010 00:09:58 +0000 (00:09 +0000)
        Reviewed by Darin Adler.

        REGRESSION (r57292): 1.5% page load speed regression from visited link information leak fix
        https://bugs.webkit.org/show_bug.cgi?id=38131

        I did a number of separate optimizations which speed up style
        resolution enough to more than make up for the regression. This
        measures as a total PLT speedup of somewhere between 1.5% and
        3.7%.

        Optimizations done:
        - Cache determineLinkState results, to avoid the need to repeatedly compute
        the visited link hash for the same element. This directly addresses much
        of the slowdown, since all elements get their style computed twice now.
        - Added a fast way to get the length of a CSSMutableStyleDeclaration, and use
        in CSSStyleSelector::matchRulesForList, since it was hot there.
        - Hoist some loop invariant code that's not detected by the compiler out of the
        main loop in matchRulesForList
        - inline CSSStyleSelector::initElement and locateSharedStyle,
        since there is only one call site in each case
        - Inline the common non-line fast case of determineLinkState, and split the rest into
        out-of-line determineLinkStateSlowCase.
        - Added inline versions of the functions called by
        visitedLinkHash (the version called by determineLinkState).

        * css/CSSMutableStyleDeclaration.cpp:
        (WebCore::CSSMutableStyleDeclaration::length): Implemented in terms of new
        inline nonvirtual mutableLength().
        * css/CSSMutableStyleDeclaration.h:
        (WebCore::CSSMutableStyleDeclaration::mutableLength): Added new nonvirtual
        inline way to get the length if you know you have a mutable style decl.
        * css/CSSStyleSelector.cpp:
        (WebCore::CSSStyleSelector::init): Clear cached link state.
        (WebCore::CSSStyleSelector::matchRulesForList): hoist some code out of the main
        loop and get style decl length more efficiently.
        (WebCore::CSSStyleSelector::initElement): inline (only one call site)
        (WebCore::CSSStyleSelector::SelectorChecker::determineLinkState): Inline fast
        case, call slow case.
        (WebCore::CSSStyleSelector::SelectorChecker::determineLinkStateSlowCase): Split
        most of the above function into this slow case helper.
        (WebCore::CSSStyleSelector::canShareStyleWithElement): Use the cache-enabled
        way to get the current link state.
        (WebCore::CSSStyleSelector::locateSharedStyle): inline
        (WebCore::CSSStyleSelector::styleForElement): Use the cache-enabled way
        to get the current link state.
        * css/CSSStyleSelector.h:
        (WebCore::CSSStyleSelector::currentElementLinkState): inline way to
        get link state for the current element; manages the cache
        * platform/LinkHash.cpp:
        (WebCore::visitedLinkHashInline): inline version of below function
        (WebCore::visitedLinkHash): call the inline version
        (WebCore::visitedURLInline): inline version of below function
        (WebCore::visitedURL): call the inline version
        (WebCore::visitedURL): call inline versions of above two functions

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@58273 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/css/CSSComputedStyleDeclaration.cpp
WebCore/css/CSSComputedStyleDeclaration.h
WebCore/css/CSSMutableStyleDeclaration.cpp
WebCore/css/CSSMutableStyleDeclaration.h
WebCore/css/CSSStyleDeclaration.h
WebCore/css/CSSStyleSelector.cpp
WebCore/css/CSSStyleSelector.h
WebCore/platform/LinkHash.cpp

index b00f198453cbbcfc42dba144c69cfe19a65726d6..c0da98eb40f0cfe3a35c10d28c38db731f37f9dc 100644 (file)
@@ -1,3 +1,60 @@
+2010-04-26  Maciej Stachowiak  <mjs@apple.com>
+
+        Reviewed by Darin Adler.
+
+        REGRESSION (r57292): 1.5% page load speed regression from visited link information leak fix
+        https://bugs.webkit.org/show_bug.cgi?id=38131
+
+        I did a number of separate optimizations which speed up style
+        resolution enough to more than make up for the regression. This
+        measures as a total PLT speedup of somewhere between 1.5% and
+        3.7%.
+       
+        Optimizations done:
+        - Cache determineLinkState results, to avoid the need to repeatedly compute
+        the visited link hash for the same element. This directly addresses much
+        of the slowdown, since all elements get their style computed twice now.
+        - Added a fast way to get the length of a CSSMutableStyleDeclaration, and use
+        in CSSStyleSelector::matchRulesForList, since it was hot there.
+        - Hoist some loop invariant code that's not detected by the compiler out of the
+        main loop in matchRulesForList
+        - inline CSSStyleSelector::initElement and locateSharedStyle,
+        since there is only one call site in each case
+        - Inline the common non-line fast case of determineLinkState, and split the rest into
+        out-of-line determineLinkStateSlowCase.
+        - Added inline versions of the functions called by
+        visitedLinkHash (the version called by determineLinkState).
+
+        * css/CSSMutableStyleDeclaration.cpp:
+        (WebCore::CSSMutableStyleDeclaration::length): Implemented in terms of new
+        inline nonvirtual mutableLength().
+        * css/CSSMutableStyleDeclaration.h:
+        (WebCore::CSSMutableStyleDeclaration::mutableLength): Added new nonvirtual
+        inline way to get the length if you know you have a mutable style decl.
+        * css/CSSStyleSelector.cpp:
+        (WebCore::CSSStyleSelector::init): Clear cached link state.
+        (WebCore::CSSStyleSelector::matchRulesForList): hoist some code out of the main
+        loop and get style decl length more efficiently.
+        (WebCore::CSSStyleSelector::initElement): inline (only one call site)
+        (WebCore::CSSStyleSelector::SelectorChecker::determineLinkState): Inline fast
+        case, call slow case.
+        (WebCore::CSSStyleSelector::SelectorChecker::determineLinkStateSlowCase): Split
+        most of the above function into this slow case helper.
+        (WebCore::CSSStyleSelector::canShareStyleWithElement): Use the cache-enabled
+        way to get the current link state.
+        (WebCore::CSSStyleSelector::locateSharedStyle): inline
+        (WebCore::CSSStyleSelector::styleForElement): Use the cache-enabled way
+        to get the current link state.
+        * css/CSSStyleSelector.h:
+        (WebCore::CSSStyleSelector::currentElementLinkState): inline way to
+        get link state for the current element; manages the cache
+        * platform/LinkHash.cpp:
+        (WebCore::visitedLinkHashInline): inline version of below function
+        (WebCore::visitedLinkHash): call the inline version
+        (WebCore::visitedURLInline): inline version of below function
+        (WebCore::visitedURL): call the inline version
+        (WebCore::visitedURL): call inline versions of above two functions
+
 2010-04-26  Sam Weinig  <sam@webkit.org>
 
         Reviewed by Alexey Proskuryakov.
index f1c8dbfabe90031a34305fa9dfda56372e95100c..efb9d1c98613ecffbcf57b08fa5d08da96eb8afb 100644 (file)
@@ -1499,7 +1499,7 @@ void CSSComputedStyleDeclaration::setProperty(int /*propertyID*/, const String&
     ec = NO_MODIFICATION_ALLOWED_ERR;
 }
 
-unsigned CSSComputedStyleDeclaration::length() const
+unsigned CSSComputedStyleDeclaration::virtualLength() const
 {
     Node* node = m_node.get();
     if (!node)
index eb93badaacbc72b3c170d47bdc93eee7ecfbf14f..ba55d770e92ff113f465b936e2cd9f9958be3ba3 100644 (file)
@@ -39,7 +39,7 @@ public:
 
     virtual String cssText() const;
 
-    virtual unsigned length() const;
+    virtual unsigned virtualLength() const;
     virtual String item(unsigned index) const;
 
     virtual PassRefPtr<CSSValue> getPropertyCSSValue(int propertyID) const;
index 31c75072ae049d7ba5d72cba130443b978594699..327f39889cfda351e3eacf24a148a6c38dbc44b6 100644 (file)
@@ -627,9 +627,9 @@ void CSSMutableStyleDeclaration::setLengthProperty(int propertyId, const String&
     setStrictParsing(parseMode);
 }
 
-unsigned CSSMutableStyleDeclaration::length() const
+unsigned CSSMutableStyleDeclaration::virtualLength() const
 {
-    return m_properties.size();
+    return length();
 }
 
 String CSSMutableStyleDeclaration::item(unsigned i) const
index 5eb8a2747c4b83aebd9c3b6d934e89e600f26e59..f7759f4562e095bb1f47a8e07a96f5a31eea4e45 100644 (file)
@@ -88,7 +88,9 @@ public:
     virtual String cssText() const;
     virtual void setCssText(const String&, ExceptionCode&);
 
-    virtual unsigned length() const;
+    virtual unsigned virtualLength() const;
+    unsigned length() const { return m_properties.size(); }
+
     virtual String item(unsigned index) const;
 
     virtual PassRefPtr<CSSValue> getPropertyCSSValue(int propertyID) const;
index 18493dff0f96f6a75b5c1bead8df36134bd1b3dd..08b3bf0365ec6e375775514e10e4446c2aebcadf 100644 (file)
@@ -42,7 +42,8 @@ public:
     virtual String cssText() const = 0;
     virtual void setCssText(const String&, ExceptionCode&) = 0;
 
-    virtual unsigned length() const = 0;
+    unsigned length() const { return virtualLength(); }
+    virtual unsigned virtualLength() const = 0;
     bool isEmpty() const { return !length(); }
     virtual String item(unsigned index) const = 0;
 
index a672c8f3d107b232f2466abde71c688901564900..307db301a6b47aa624bb898e91b932540b7e9ffc 100644 (file)
@@ -486,6 +486,7 @@ void CSSStyleSelector::addKeyframeStyle(PassRefPtr<WebKitCSSKeyframesRule> rule)
 void CSSStyleSelector::init()
 {
     m_element = 0;
+    m_haveCachedLinkState = false;
     m_matchedDecls.clear();
     m_ruleList = 0;
     m_rootDefaultStyle = 0;
@@ -694,9 +695,9 @@ void CSSStyleSelector::matchRulesForList(CSSRuleDataList* rules, int& firstRuleI
     if (!rules)
         return;
 
+    const AtomicString& localName = m_element->localName();
     for (CSSRuleData* d = rules->first(); d; d = d->next()) {
         CSSStyleRule* rule = d->rule();
-        const AtomicString& localName = m_element->localName();
         const AtomicString& selectorLocalName = d->selector()->m_tag.localName();
         if ((localName == selectorLocalName || selectorLocalName == starAtom) && checkSelector(d->selector())) {
             // If the rule has no properties to apply, then ignore it.
@@ -800,8 +801,11 @@ void CSSStyleSelector::sortMatchedRules(unsigned start, unsigned end)
         m_matchedRules[i] = rulesMergeBuffer[i - start];
 }
 
-void CSSStyleSelector::initElement(Element* e)
+inline void CSSStyleSelector::initElement(Element* e)
 {
+    if (m_element != e)
+        m_haveCachedLinkState = false;
+
     m_element = e;
     m_styledElement = m_element && m_element->isStyledElement() ? static_cast<StyledElement*>(m_element) : 0;
 }
@@ -874,10 +878,17 @@ CSSStyleSelector::SelectorChecker::SelectorChecker(Document* document, bool stri
 {
 }
 
-EInsideLink CSSStyleSelector::SelectorChecker::determineLinkState(Element* element) const
+inline EInsideLink CSSStyleSelector::SelectorChecker::determineLinkState(Element* element) const
 {
     if (!element->isLink())
         return NotInsideLink;
+    return determineLinkStateSlowCase(element);
+}
+    
+
+EInsideLink CSSStyleSelector::SelectorChecker::determineLinkStateSlowCase(Element* element) const
+{
+    ASSERT(element->isLink());
     
     const AtomicString* attr = linkAttribute(element);
     if (!attr || attr->isNull())
@@ -1025,7 +1036,7 @@ bool CSSStyleSelector::canShareStyleWithElement(Node* n)
                     mappedAttrsMatch = s->mappedAttributes()->mapsEquivalent(m_styledElement->mappedAttributes());
                 if (mappedAttrsMatch) {
                     if (s->isLink()) {
-                        if (m_checker.determineLinkState(m_element) != style->insideLink())
+                        if (currentElementLinkState() != style->insideLink())
                             return false;
                     }
                     return true;
@@ -1036,7 +1047,7 @@ bool CSSStyleSelector::canShareStyleWithElement(Node* n)
     return false;
 }
 
-RenderStyle* CSSStyleSelector::locateSharedStyle()
+ALWAYS_INLINE RenderStyle* CSSStyleSelector::locateSharedStyle()
 {
     if (m_styledElement && !m_styledElement->inlineStyleDecl() && !m_styledElement->hasID() && !m_styledElement->document()->usesSiblingRules()) {
         // Check previous siblings.
@@ -1169,7 +1180,7 @@ PassRefPtr<RenderStyle> CSSStyleSelector::styleForElement(Element* e, RenderStyl
 
     if (e->isLink()) {
         m_style->setIsLink(true);
-        m_style->setInsideLink(m_checker.determineLinkState(e));
+        m_style->setInsideLink(currentElementLinkState());
     }
     
     if (simpleDefaultStyleSheet && !elementCanUseSimpleDefaultStyle(e))
index 80a186bb0a7261325cd9fab0d48fcca6873fcbf7..151f61d9876dd760b93de4a27ad1903d91ce0963 100644 (file)
@@ -85,7 +85,6 @@ public:
                          bool strictParsing, bool matchAuthorAndUserStyles);
         ~CSSStyleSelector();
 
-        void initElement(Element*);
         void initForStyleResolve(Element*, RenderStyle* parentStyle = 0, PseudoId = NOPSEUDO);
         PassRefPtr<RenderStyle> styleForElement(Element*, RenderStyle* parentStyle = 0, bool allowSharing = true, bool resolveForRootDefault = false, bool matchVisitedLinks = false);
         void keyframeStylesForAnimation(Element*, const RenderStyle*, KeyframeList& list);
@@ -101,6 +100,7 @@ public:
 #endif
 
     private:
+        void initElement(Element*);
         RenderStyle* locateSharedStyle();
         Node* locateCousinList(Element* parent, unsigned depth = 1);
         bool canShareStyleWithElement(Node*);
@@ -201,6 +201,7 @@ public:
             bool checkScrollbarPseudoClass(CSSSelector*, PseudoId& dynamicPseudo) const;
 
             EInsideLink determineLinkState(Element* element) const;
+            EInsideLink determineLinkStateSlowCase(Element* element) const;
             void allVisitedStateChanged();
             void visitedStateChanged(LinkHash visitedHash);
 
@@ -252,6 +253,19 @@ public:
 
         StyleImage* styleImage(CSSValue* value);
 
+
+        EInsideLink currentElementLinkState() const
+        {
+            if (!m_haveCachedLinkState) {
+                m_cachedLinkState = m_checker.determineLinkState(m_element);
+                m_haveCachedLinkState = true;
+            }
+            return m_cachedLinkState;
+        }
+
+        mutable EInsideLink m_cachedLinkState;
+        mutable bool m_haveCachedLinkState;
+
         // We collect the set of decls that match in |m_matchedDecls|.  We then walk the
         // set of matched decls four times, once for those properties that others depend on (like font-size),
         // and then a second time for all the remaining properties.  We then do the same two passes
@@ -347,7 +361,7 @@ public:
         CSSRuleData* m_first;
         CSSRuleData* m_last;
     };
-    
+
 } // namespace WebCore
 
 #endif // CSSStyleSelector_h
index 0bd589c28eb88a5c4e4e1a512dd29c84786417a3..12437ab77df934d6a7ad48577879327f65af241d 100644 (file)
@@ -147,12 +147,17 @@ static inline bool needsTrailingSlash(const UChar* characters, unsigned length)
     return pos == length;
 }
 
+static ALWAYS_INLINE LinkHash visitedLinkHashInline(const UChar* url, unsigned length)
+{
+    return AlreadyHashed::avoidDeletedValue(StringImpl::computeHash(url, length));
+}
+
 LinkHash visitedLinkHash(const UChar* url, unsigned length)
 {
-  return AlreadyHashed::avoidDeletedValue(StringImpl::computeHash(url, length));
+    return visitedLinkHashInline(url, length);
 }
 
-void visitedURL(const KURL& base, const AtomicString& attributeURL, Vector<UChar, 512>& buffer)
+static ALWAYS_INLINE void visitedURLInline(const KURL& base, const AtomicString& attributeURL, Vector<UChar, 512>& buffer)
 {
     if (attributeURL.isNull())
         return;
@@ -213,14 +218,19 @@ void visitedURL(const KURL& base, const AtomicString& attributeURL, Vector<UChar
     return;
 }
 
+void visitedURL(const KURL& base, const AtomicString& attributeURL, Vector<UChar, 512>& buffer)
+{
+    return visitedURLInline(base, attributeURL, buffer);
+}
+
 LinkHash visitedLinkHash(const KURL& base, const AtomicString& attributeURL)
 {
     Vector<UChar, 512> url;
-    visitedURL(base, attributeURL, url);
+    visitedURLInline(base, attributeURL, url);
     if (url.isEmpty())
         return 0;
 
-    return visitedLinkHash(url.data(), url.size());
+    return visitedLinkHashInline(url.data(), url.size());
 }
 
 }  // namespace WebCore