Optimize StyleSheetContents::checkLoaded by keeping track of completed

Source/core/css/StyleSheetContents.cpp

 /*
  * (C) 1999-2003 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2004, 2006, 2007, 2012 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
@@ skipping 356 matching lines @@
     return false;
 }
 
 bool StyleSheetContents::loadCompleted() const
 {
     StyleSheetContents* parentSheet = parentStyleSheet();
     if (parentSheet)
         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!

 }
 
 void StyleSheetContents::checkLoaded()
 {
     if (isLoading())
         return;
 
     // Avoid |this| being deleted by scripts that run via
     // ScriptableDocumentParser::executeScriptsWaitingForResources().
     // See https://bugs.webkit.org/show_bug.cgi?id=95106
     RefPtrWillBeRawPtr<StyleSheetContents> protect(this);
 
     StyleSheetContents* parentSheet = parentStyleSheet();
     if (parentSheet) {
         parentSheet->checkLoaded();
         return;
     }
 
     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
     // ScriptableDocumentParser::executeScriptsWaitingForResources(). Also protect
     // the |CSSStyleSheet| from being deleted during iteration via the |sheetLoaded|
     // method.
     //
     // 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) {
         if (protectedClients[i]->loadCompleted())
             continue;
 
         // sheetLoaded might be invoked after its owner node is removed from document.
         if (RefPtr<Node> ownerNode = protectedClients[i]->ownerNode()) {
             if (protectedClients[i]->sheetLoaded())
                 ownerNode->notifyLoadedSheetAndAllCriticalSubresources(m_didLoadErrorOccur);
         }
     }
 }
 
 void StyleSheetContents::notifyLoadedSheet(const CSSStyleSheetResource* sheet)
 {
     ASSERT(sheet);
     m_didLoadErrorOccur |= sheet->errorOccurred();
     // updateLayoutIgnorePendingStyleSheets can cause us to create the RuleSet on this
     // sheet before its imports have loaded. So clear the RuleSet when the imports
     // load since the import's subrules are flattened into its parent sheet's RuleSet.
     clearRuleSet();
 }
 
 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();
 }
 
 StyleSheetContents* StyleSheetContents::rootStyleSheet() const
 {
     const StyleSheetContents* root = this;
     while (root->parentStyleSheet())
         root = root->parentStyleSheet();
     return const_cast<StyleSheetContents*>(root);
 }
 
 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
 {
     Node* ownerNode = singleOwnerNode();
     return ownerNode ? &ownerNode->document() : 0;
 }
 
 KURL StyleSheetContents::completeURL(const String& url) const
 {
@@ skipping 40 matching lines @@
     return childRulesHaveFailedOrCanceledSubresources(m_childRules);
 }
 
 StyleSheetContents* StyleSheetContents::parentStyleSheet() const
 {
     return m_ownerRule ? m_ownerRule->parentStyleSheet() : 0;
 }
 
 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()
 {
     ASSERT(!m_isInMemoryCache);
     ASSERT(isCacheable());
     m_isInMemoryCache = true;
 }
 
 void StyleSheetContents::removedFromMemoryCache()
@@ skipping 11 matching lines @@
 
 RuleSet& StyleSheetContents::ensureRuleSet(const MediaQueryEvaluator& medium, AddRuleFlags addRuleFlags)
 {
     if (!m_ruleSet) {
         m_ruleSet = RuleSet::create();
         m_ruleSet->addRulesFromSheet(this, medium, addRuleFlags);
     }
     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())
         parentSheet->clearRuleSet();
 
     // Don't want to clear the StyleResolver if the RuleSet hasn't been created
     // since we only clear the StyleResolver so that it's members are properly
     // updated in ScopedStyleResolver::addRulesFromSheet.
     if (!m_ruleSet)
         return;
 
     // 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();
+}
+
+static void removeFontFaceRules(WillBeHeapHashSet<RawPtrWillBeWeakMember<CSSStyleSheet> >& clients, const StyleRuleFontFace* fontFaceRule)
+{
+    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));
     }
-    m_ruleSet.clear();
 }
 
 void StyleSheetContents::notifyRemoveFontFaceRule(const StyleRuleFontFace* fontFaceRule)
 {
     StyleSheetContents* root = rootStyleSheet();
-
-    for (ClientsIterator it = root->m_clients.begin(); it != root->m_clients.end(); ++it) {
-        if (Node* ownerNode = (*it)->ownerNode())
-            ownerNode->document().styleEngine()->removeFontFaceRules(WillBeHeapVector<RawPtrWillBeMember<const StyleRuleFontFace> >(1, fontFaceRule));
-    }
+    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) {
         StyleRuleBase* rule = rules[i].get();
 
         if (rule->isFontFaceRule()) {
             fontFaceRules.append(toStyleRuleFontFace(rule));
         } else if (rule->isMediaRule()) {
@@ skipping 14 matching lines @@
     }
 
     findFontFaceRulesFromRules(childRules(), fontFaceRules);
 }
 
 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);
 }
 
 }