Optimize StyleSheetContents::checkLoaded by keeping track of completed

Source/core/css/StyleSheetContents.cpp

Index: Source/core/css/StyleSheetContents.cpp
diff --git a/Source/core/css/StyleSheetContents.cpp b/Source/core/css/StyleSheetContents.cpp
index 37c644fd5f241c0d4b727c1967276e499e8ce34b..c4b489185123388e83612cfb085fb928b371115f 100644
--- a/Source/core/css/StyleSheetContents.cpp
+++ b/Source/core/css/StyleSheetContents.cpp
@@ -374,10 +374,8 @@ bool StyleSheetContents::loadCompleted() const
         return parentSheet->loadCompleted();
 
     StyleSheetContents* root = rootStyleSheet();
-    for (ClientsIterator it = root->m_clients.begin(); it != root->m_clients.end(); ++it) {
-        if (!(*it)->loadCompleted())
-            return false;
-    }
+    if (root->m_loadingClients.size())
+        return false;
     return true;

[esprehn, 2014/03/13 19:56:28]

return rootStyleSheet()->m_loadingClients.isEmpty();

[Mads Ager (chromium), 2014/03/14 09:30:18]

Heh, yes, thanks!

 }
 
@@ -398,7 +396,7 @@ void StyleSheetContents::checkLoaded()
     }
 
     StyleSheetContents* root = rootStyleSheet();
-    if (root->m_clients.isEmpty())
+    if (root->m_loadingClients.isEmpty())
         return;
 
     // Avoid |CSSSStyleSheet| and |ownerNode| being deleted by scripts that run via
@@ -409,8 +407,9 @@ void StyleSheetContents::checkLoaded()
     // FIXME: oilpan: with oilpan we do not need to protect the CSSStyleSheets
     // explicitly during the iteration. The iterator will make the pointers
     // strong during iteration so we can just directly iterate root->m_clients.
-    WillBeHeapVector<RefPtrWillBeMember<CSSStyleSheet>, 5> protectedClients;
-    for (ClientsIterator it = root->m_clients.begin(); it != root->m_clients.end(); ++it)
+    WillBeHeapVector<RefPtrWillBeMember<CSSStyleSheet> > protectedClients;
+    protectedClients.reserveInitialCapacity(root->m_loadingClients.size());
+    for (ClientsIterator it = root->m_loadingClients.begin(); it != root->m_loadingClients.end(); ++it)
         protectedClients.append(*it);

[esprehn, 2014/03/13 19:56:28]

This should just use copyToVector()

[Mads Ager (chromium), 2014/03/14 09:30:18]

Oh, I didn't know about copyToVector. Much better! Done. :)

 
     for (unsigned i = 0; i < protectedClients.size(); ++i) {
@@ -438,7 +437,9 @@ void StyleSheetContents::notifyLoadedSheet(const CSSStyleSheetResource* sheet)
 void StyleSheetContents::startLoadingDynamicSheet()
 {
     StyleSheetContents* root = rootStyleSheet();
-    for (ClientsIterator it = root->m_clients.begin(); it != root->m_clients.end(); ++it)
+    for (ClientsIterator it = root->m_loadingClients.begin(); it != root->m_loadingClients.end(); ++it)
+        (*it)->startLoadingDynamicSheet();
+    for (ClientsIterator it = root->m_completedClients.begin(); it != root->m_completedClients.end(); ++it)
         (*it)->startLoadingDynamicSheet();
 }
 
@@ -453,18 +454,17 @@ StyleSheetContents* StyleSheetContents::rootStyleSheet() const
 bool StyleSheetContents::hasSingleOwnerNode() const
 {
     StyleSheetContents* root = rootStyleSheet();
-    if (root->m_clients.isEmpty())
-        return false;
-    return root->m_clients.size() == 1;
+    return root->hasOneClient();

[esprehn, 2014/03/13 19:56:28]

rootStyleSheet()->hasOneClient()

[Mads Ager (chromium), 2014/03/14 09:30:18]

Done.

 }
 
 Node* StyleSheetContents::singleOwnerNode() const
 {
     StyleSheetContents* root = rootStyleSheet();
-    if (root->m_clients.isEmpty())
-        return 0;
-    ASSERT(root->m_clients.size() == 1);
-    return (*root->m_clients.begin())->ownerNode();
+    if (root->hasOneClient()) {
+        return root->m_loadingClients.size() ? (*root->m_loadingClients.begin())->ownerNode() : (*root->m_completedClients.begin())->ownerNode();

[esprehn, 2014/03/13 19:56:28]

Lets break this up since it's easier to read.

if (!root->hasOneClient())
  return 0;
if (root->m_loadingClients.size())
  return ...;
return ...;

you don't need the assert.

[Mads Ager (chromium), 2014/03/14 09:30:18]

Yes, much better, thanks.

+    }
+    ASSERT(root->m_loadingClients.isEmpty() && root->m_completedClients.isEmpty());
+    return 0;
 }
 
 Document* StyleSheetContents::singleOwnerDocument() const
@@ -525,14 +525,29 @@ StyleSheetContents* StyleSheetContents::parentStyleSheet() const
 
 void StyleSheetContents::registerClient(CSSStyleSheet* sheet)
 {
-    ASSERT(!m_clients.contains(sheet));
-    m_clients.add(sheet);
+    ASSERT(!m_loadingClients.contains(sheet) && !m_completedClients.contains(sheet));
+    m_loadingClients.add(sheet);

[esprehn, 2014/03/13 19:56:28]

Why is it a HashSet if you're just going to ASSERT that you're not already in
it. Could it be just a Vector?

[Mads Ager (chromium), 2014/03/14 09:30:18]

The set of clients is a weak set. It does not keep the clients alive and when
they die they remove themselves from the set. So there are random order
removals. For those a vector will perform a lot of copying and a set seems more
reasonable.

Having these be sets makes the move between loading and non-loading that this
change is introducing more efficient as well.

I kept the assertion in order to not losen any requirements that I do not know
about in this code. Also, it seems like a good idea at least to assert that the
client is not going to appear in both the loading and completed set. I could
simplify the assert to just check that the client is not in m_completedClients
when we add it to m_loadingClients.

(There is an oilpan-only reason as well which is that weak processing of vectors
is hard to make sense of and weak processing of HashSets is easy. However, that
is not really relevant to this particular change.)

 }
 
 void StyleSheetContents::unregisterClient(CSSStyleSheet* sheet)
 {
-    ASSERT(m_clients.contains(sheet));
-    m_clients.remove(sheet);
+    ASSERT(m_loadingClients.contains(sheet) || m_completedClients.contains(sheet));
+    m_loadingClients.remove(sheet);
+    m_completedClients.remove(sheet);
+}
+
+void StyleSheetContents::clientLoadCompleted(CSSStyleSheet* sheet)
+{
+    ASSERT(m_loadingClients.contains(sheet));
+    m_loadingClients.remove(sheet);
+    m_completedClients.add(sheet);
+}
+
+void StyleSheetContents::clientLoadStarted(CSSStyleSheet* sheet)
+{
+    ASSERT(m_completedClients.contains(sheet));
+    m_completedClients.remove(sheet);
+    m_loadingClients.add(sheet);
 }
 
 void StyleSheetContents::addedToMemoryCache()
@@ -564,6 +579,14 @@ RuleSet& StyleSheetContents::ensureRuleSet(const MediaQueryEvaluator& medium, Ad
     return *m_ruleSet.get();
 }
 
+static void clearResolvers(WillBeHeapHashSet<RawPtrWillBeWeakMember<CSSStyleSheet> >& clients)
+{
+    for (WillBeHeapHashSet<RawPtrWillBeWeakMember<CSSStyleSheet> >::iterator it = clients.begin(); it != clients.end(); ++it) {
+        if (Document* document = (*it)->ownerDocument())
+            document->styleEngine()->clearResolver();
+    }
+}
+
 void StyleSheetContents::clearRuleSet()
 {
     if (StyleSheetContents* parentSheet = parentStyleSheet())
@@ -577,23 +600,26 @@ void StyleSheetContents::clearRuleSet()
 
     // Clearing the ruleSet means we need to recreate the styleResolver data structures.
     // See the StyleResolver calls in ScopedStyleResolver::addRulesFromSheet.
-    for (ClientsIterator it = m_clients.begin(); it != m_clients.end(); ++it) {
-        if (Document* document = (*it)->ownerDocument())
-            document->styleEngine()->clearResolver();
-    }
+    clearResolvers(m_loadingClients);
+    clearResolvers(m_completedClients);
     m_ruleSet.clear();
 }
 
-void StyleSheetContents::notifyRemoveFontFaceRule(const StyleRuleFontFace* fontFaceRule)
+static void removeFontFaceRules(WillBeHeapHashSet<RawPtrWillBeWeakMember<CSSStyleSheet> >& clients, const StyleRuleFontFace* fontFaceRule)
 {
-    StyleSheetContents* root = rootStyleSheet();
-
-    for (ClientsIterator it = root->m_clients.begin(); it != root->m_clients.end(); ++it) {
+    for (WillBeHeapHashSet<RawPtrWillBeWeakMember<CSSStyleSheet> >::iterator it = clients.begin(); it != clients.end(); ++it) {
         if (Node* ownerNode = (*it)->ownerNode())
             ownerNode->document().styleEngine()->removeFontFaceRules(WillBeHeapVector<RawPtrWillBeMember<const StyleRuleFontFace> >(1, fontFaceRule));
     }
 }
 
+void StyleSheetContents::notifyRemoveFontFaceRule(const StyleRuleFontFace* fontFaceRule)
+{
+    StyleSheetContents* root = rootStyleSheet();
+    removeFontFaceRules(root->m_loadingClients, fontFaceRule);
+    removeFontFaceRules(root->m_completedClients, fontFaceRule);
+}
+
 static void findFontFaceRulesFromRules(const WillBeHeapVector<RefPtrWillBeMember<StyleRuleBase> >& rules, WillBeHeapVector<RawPtrWillBeMember<const StyleRuleFontFace> >& fontFaceRules)
 {
     for (unsigned i = 0; i < rules.size(); ++i) {
@@ -626,7 +652,8 @@ void StyleSheetContents::trace(Visitor* visitor)
     visitor->trace(m_ownerRule);
     visitor->trace(m_importRules);
     visitor->trace(m_childRules);
-    visitor->trace(m_clients);
+    visitor->trace(m_loadingClients);
+    visitor->trace(m_completedClients);
     visitor->trace(m_ruleSet);
 }