Changeset 102584 in webkit

Timestamp:

Dec 12, 2011 7:47:43 AM (12 years ago)

Author:

apavlov@chromium.org

Message:

Implement a cache for CSSStyleRule::selectorText()
​https://bugs.webkit.org/show_bug.cgi?id=74269

This change is geared towards speeding up the CSS selector profiler,
its implementation tracked at ​https://bugs.webkit.org/show_bug.cgi?id=74004.
Using a proof-of-concept implementation of the profiler, this change reduces
the profiler temporal overhead on PerformanceTests/Parser/html5-full-render.html
roughly by 86% (from ~72 seconds down to ~10 seconds). This change also does not
considerably increase average memory usage, as reading selectorText is a relatively
rare case during normal web browsing.

Reviewed by Andreas Kling.

No new tests, as this functionality is covered by existing tests.

• css/CSSRule.h:
• css/CSSStyleRule.cpp:

(WebCore::CSSStyleRule::~CSSStyleRule):
(WebCore::selectorTextCache):
(WebCore::CSSStyleRule::cleanup):
(WebCore::CSSStyleRule::generateSelectorText):
(WebCore::CSSStyleRule::selectorText):
(WebCore::CSSStyleRule::setSelectorText):

• css/CSSStyleRule.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• css/CSSRule.h (modified) (2 diffs)
• css/CSSStyleRule.cpp (modified) (3 diffs)
• css/CSSStyleRule.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-12-12  Alexander Pavlov  <apavlov@chromium.org>
+
+        Implement a cache for CSSStyleRule::selectorText()
+        https://bugs.webkit.org/show_bug.cgi?id=74269
+
+        This change is geared towards speeding up the CSS selector profiler,
+        its implementation tracked at https://bugs.webkit.org/show_bug.cgi?id=74004.
+        Using a proof-of-concept implementation of the profiler, this change reduces
+        the profiler temporal overhead on PerformanceTests/Parser/html5-full-render.html
+        roughly by 86% (from ~72 seconds down to ~10 seconds). This change also does not
+        considerably increase average memory usage, as reading selectorText is a relatively
+        rare case during normal web browsing.
+
+        Reviewed by Andreas Kling.
+
+        No new tests, as this functionality is covered by existing tests.
+
+        * css/CSSRule.h:
+        * css/CSSStyleRule.cpp:
+        (WebCore::CSSStyleRule::~CSSStyleRule):
+        (WebCore::selectorTextCache):
+        (WebCore::CSSStyleRule::cleanup):
+        (WebCore::CSSStyleRule::generateSelectorText):
+        (WebCore::CSSStyleRule::selectorText):
+        (WebCore::CSSStyleRule::setSelectorText):
+        * css/CSSStyleRule.h:
+
 2011-12-12  Alexander Pavlov  <apavlov@chromium.org>
 

trunk/Source/WebCore/css/CSSRule.h

@@ -111 +111 @@
     CSSRule(CSSStyleSheet* parent, Type type)
         : m_sourceLine(0)
+        , m_hasCachedSelectorText(false)
         , m_parentIsRule(false)
         , m_type(type)
@@ -123 +124 @@
 
     // Only used by CSSStyleRule but kept here to maximize struct packing.
-    signed m_sourceLine : 27;
+    signed m_sourceLine : 26;
+    mutable bool m_hasCachedSelectorText : 1;
 
 private:

trunk/Source/WebCore/css/CSSStyleRule.cpp

@@ -32 +32 @@
 #include "StyleSheet.h"
 
+#include <wtf/text/StringBuilder.h>
+
 namespace WebCore {
 
@@ -47 +49 @@
     if (m_style)
         m_style->setParentRule(0);
+    cleanup();
+}
+
+typedef HashMap<const CSSStyleRule*, String> SelectorTextCache;
+static SelectorTextCache& selectorTextCache()
+{
+    DEFINE_STATIC_LOCAL(SelectorTextCache, cache, ());
+    return cache;
+}
+
+inline void CSSStyleRule::cleanup()
+{
+    if (m_hasCachedSelectorText) {
+        selectorTextCache().remove(this);
+        m_hasCachedSelectorText = false;
+    }
+}
+
+String CSSStyleRule::generateSelectorText() const
+{
+    if (isPageRule())
+        return static_cast<const CSSPageRule*>(this)->pageSelectorText();
+    else {
+        StringBuilder builder;
+        for (CSSSelector* s = selectorList().first(); s; s = CSSSelectorList::next(s)) {
+            if (s != selectorList().first())
+                builder.append(", ");
+            builder.append(s->selectorText());
+        }
+        return builder.toString();
+    }
 }
 
 String CSSStyleRule::selectorText() const
 {
-    if (isPageRule())
-        return static_cast<const CSSPageRule*>(this)->pageSelectorText();
+    if (m_hasCachedSelectorText) {
+        ASSERT(selectorTextCache().contains(this));
+        return selectorTextCache().get(this);
+    }
 
-    String str;
-    for (CSSSelector* s = selectorList().first(); s; s = CSSSelectorList::next(s)) {
-        if (s != selectorList().first())
-            str += ", ";
-        str += s->selectorText();
-    }
-    return str;
+    ASSERT(!selectorTextCache().contains(this));
+    String text = generateSelectorText();
+    selectorTextCache().set(this, text);
+    m_hasCachedSelectorText = true;
+    return text;
 }
 
@@ -86 +119 @@
     String oldSelectorText = this->selectorText();
     m_selectorList.adopt(selectorList);
+
+    if (m_hasCachedSelectorText) {
+        ASSERT(selectorTextCache().contains(this));
+        selectorTextCache().set(this, generateSelectorText());
+    }
+
     if (this->selectorText() == oldSelectorText)
         return;

trunk/Source/WebCore/css/CSSStyleRule.h

@@ -62 +62 @@
 
 private:
+    void cleanup();
+    String generateSelectorText() const;
+
     RefPtr<CSSMutableStyleDeclaration> m_style;
     CSSSelectorList m_selectorList;