Changeset 21245 in webkit

Timestamp:

May 3, 2007, 9:35:28 PM (18 years ago)

Author:

beidson

Message:

Reviewed by Oliver

Resolve an outstanding FIXME in Loader::numRequests()

Before, numRequests() would iterate through the list of requests pending load and the list of currently
loading requests and tally up a count matching the current DocLoader.

I noticed while studying and cleaning up the loader code that numRequests() is potentially very hot!
Indeed load a complex site with many resources and multiple frames, and this method gets called very often,
tallying up this number every time.

The FIXME was to keep a collection of Requests mapped to each DocLoader. In reality, since this map would
simply be used for retrieving a count, that was overkill. Keeping a request count in the DocLoader itself
along with maintaining that count in Loader as requests come and go is a much better way to do this.

• loader/DocLoader.cpp: (WebCore::DocLoader::DocLoader): (WebCore::DocLoader::incrementRequestCount): (WebCore::DocLoader::decrementRequestCount): (WebCore::DocLoader::requestCount): Emulate the defunct Loader::numRequests()
• loader/DocLoader.h:

• loader/FrameLoader.cpp: (WebCore::numRequests): Call DocLoader::requestCount() directly (WebCore::FrameLoader::checkCompleted): Use numRequests()

• loader/loader.cpp: (WebCore::Loader::load): Increment the DocLoader's request count (WebCore::Loader::servePendingRequests): If the SubresourceLoader failed to create, decrement the count (WebCore::Loader::didFinishLoading): If the Request is not Multipart, decrement the count (WebCore::Loader::didFail): If the Request is not Multipart, decrement the count (WebCore::Loader::didReceiveResponse): If the Request becomes Multipart, decrement the count (WebCore::Loader::cancelRequests): Decrement the count for the pending requests being tossed, and ASSERT the count is zero after all requests have been cancelled
• loader/loader.h:

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (3 diffs)
• loader/DocLoader.cpp (modified) (2 diffs)
• loader/DocLoader.h (modified) (2 diffs)
• loader/FrameLoader.cpp (modified) (2 diffs)
• loader/loader.cpp (modified) (9 diffs)
• loader/loader.h (modified) (1 diff)

trunk/WebCore/ChangeLog

@@ -1 @@
-<<<<<<< .mine
+2007-05-03  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Oliver
+
+        Resolve an outstanding FIXME in Loader::numRequests()
+
+        Before, numRequests() would iterate through the list of requests pending load and the list of currently
+        loading requests and tally up a count matching the current DocLoader.
+
+        I noticed while studying and cleaning up the loader code that numRequests() is potentially very hot!
+        Indeed load a complex site with many resources and multiple frames, and this method gets called very often,
+        tallying up this number every time.
+
+        The FIXME was to keep a collection of Requests mapped to each DocLoader.  In reality, since this map would
+        simply be used for retrieving a count, that was overkill.  Keeping a request count in the DocLoader itself
+        along with maintaining that count in Loader as requests come and go is a much better way to do this.
+
+        * loader/DocLoader.cpp:
+        (WebCore::DocLoader::DocLoader):
+        (WebCore::DocLoader::incrementRequestCount):
+        (WebCore::DocLoader::decrementRequestCount):
+        (WebCore::DocLoader::requestCount): Emulate the defunct Loader::numRequests()
+        * loader/DocLoader.h:
+
+        * loader/FrameLoader.cpp:
+        (WebCore::numRequests): Call DocLoader::requestCount() directly
+        (WebCore::FrameLoader::checkCompleted): Use numRequests()
+
+        * loader/loader.cpp:
+        (WebCore::Loader::load): Increment the DocLoader's request count
+        (WebCore::Loader::servePendingRequests): If the SubresourceLoader failed to create, decrement the count
+        (WebCore::Loader::didFinishLoading): If the Request is not Multipart, decrement the count
+        (WebCore::Loader::didFail): If the Request is not Multipart, decrement the count
+        (WebCore::Loader::didReceiveResponse): If the Request becomes Multipart, decrement the count
+        (WebCore::Loader::cancelRequests): Decrement the count for the pending requests being tossed, and ASSERT the
+          count is zero after all requests have been cancelled
+        * loader/loader.h:
+
 2007-05-03  Geoffrey Garen  <ggaren@apple.com>
 
@@ -19 +56 @@
         * loader/FrameLoader.h:
 
-=======
 2007-05-03  Justin Garcia  <justin.garcia@apple.com>
 
@@ -32 +68 @@
         Nil-check checkAncestor and lastClosed.
 
->>>>>>> .r21243
 2007-05-03  Timothy Hatcher  <timothy@apple.com>
 

trunk/WebCore/loader/DocLoader.cpp

@@ -46 +46 @@
     , m_frame(frame)
     , m_doc(doc)
+    , m_requestCount(0)
     , m_autoLoadImages(true)
     , m_loadInProgress(false)
@@ -206 +207 @@
 }
 
-}
+void DocLoader::incrementRequestCount()
+{
+    ++m_requestCount;
+}
+
+void DocLoader::decrementRequestCount()
+{
+    --m_requestCount;
+    ASSERT(m_requestCount > -1);
+}
+
+int DocLoader::requestCount()
+{
+    if (loadInProgress())
+         return m_requestCount + 1;
+    return m_requestCount;
+}
+
+}

trunk/WebCore/loader/DocLoader.h

@@ -89 +89 @@
 #endif
 
+    void incrementRequestCount();
+    void decrementRequestCount();
+    int requestCount();
 private:
     CachedResource* requestResource(CachedResource::Type, const String& url, const String* charset = 0, bool skipCanLoadCheck = false);
@@ -102 +105 @@
     Document *m_doc;
     
+    int m_requestCount;
+    
     //29 bits left
     bool m_autoLoadImages : 1;

trunk/WebCore/loader/FrameLoader.cpp

@@ -196 +196 @@
 static int numRequests(Document* document)
 {
-    if (document)
-        return cache()->loader()->numRequests(document->docLoader());
-    return 0;
+    if (!document)
+        return 0;
+    
+    return document->docLoader()->requestCount();
 }
 
@@ -1127 +1128 @@
 
     // Still waiting for images/scripts?
-    if (m_frame->document() && m_frame->document()->docLoader())
-        if (cache()->loader()->numRequests(m_frame->document()->docLoader()))
+    if (m_frame->document())
+        if (numRequests(m_frame->document()))
             return;
 

trunk/WebCore/loader/loader.cpp

@@ -59 +59 @@
 void Loader::load(DocLoader* dl, CachedResource* object, bool incremental, bool skipCanLoadCheck)
 {
+    ASSERT(dl);
     Request* req = new Request(dl, object, incremental);
     m_requestsPending.append(req);
+    dl->incrementRequestCount();
     servePendingRequests(skipCanLoadCheck);
 }
@@ -71 +73 @@
     // get the first pending request
     Request* req = m_requestsPending.take(0);
+    DocLoader* dl = req->docLoader();
+    dl->decrementRequestCount();
 
     ResourceRequest request(req->cachedResource()->url());
@@ -76 +80 @@
     if (!req->cachedResource()->accept().isEmpty())
         request.setHTTPAccept(req->cachedResource()->accept());
-    if (req->docLoader())  {
-        KURL r = req->docLoader()->doc()->URL();
-        if (r.protocol().startsWith("http") && r.path().isEmpty())
-            r.setPath("/");
-        request.setHTTPReferrer(r.url());
-        DeprecatedString domain = r.host();
-        if (req->docLoader()->doc()->isHTMLDocument())
-            domain = static_cast<HTMLDocument*>(req->docLoader()->doc())->domain().deprecatedString();
-    }
-    
-    RefPtr<SubresourceLoader> loader = SubresourceLoader::create(req->docLoader()->doc()->frame(), this, request, skipCanLoadCheck);
-
-    if (loader)
+
+    KURL r = dl->doc()->URL();
+    if (r.protocol().startsWith("http") && r.path().isEmpty())
+        r.setPath("/");
+    request.setHTTPReferrer(r.url());
+    DeprecatedString domain = r.host();
+    if (dl->doc()->isHTMLDocument())
+        domain = static_cast<HTMLDocument*>(dl->doc())->domain().deprecatedString();
+    
+    RefPtr<SubresourceLoader> loader = SubresourceLoader::create(dl->doc()->frame(), this, request, skipCanLoadCheck);
+
+    if (loader) {
         m_requestsLoading.add(loader.release(), req);
+        dl->incrementRequestCount();
+    }
 }
 
@@ -100 +105 @@
     Request* req = i->second;
     m_requestsLoading.remove(i);
+    DocLoader* docLoader = req->docLoader();
+    if (!req->isMultipart())
+        docLoader->decrementRequestCount();
 
     CachedResource* object = req->cachedResource();
-    DocLoader* docLoader = req->docLoader();
 
     docLoader->setLoadInProgress(true);
@@ -127 +134 @@
     Request* req = i->second;
     m_requestsLoading.remove(i);
+    DocLoader* docLoader = req->docLoader();
+    if (!req->isMultipart())
+        docLoader->decrementRequestCount();
 
     CachedResource* object = req->cachedResource();
-    DocLoader* docLoader = req->docLoader();
 
     if (!cancelled) {
@@ -161 +170 @@
     } else if (response.isMultipart()) {
         req->setIsMultipart(true);
+        
+        // We don't count multiParts in a DocLoader's request count
+        req->docLoader()->decrementRequestCount();
+            
         // If we get a multipart response, we must have a handle
         ASSERT(loader->handle());
@@ -184 +197 @@
 }
 
-int Loader::numRequests(DocLoader* dl) const
-{
-    // FIXME: Maybe we should keep a collection of requests by DocLoader, so we can do this instantly.
-
-    int res = 0;
-
-    DeprecatedPtrListIterator<Request> pIt(m_requestsPending);
-    for (; pIt.current(); ++pIt) {
-        if (pIt.current()->docLoader() == dl)
-            res++;
-    }
-
-    RequestMap::const_iterator end = m_requestsLoading.end();
-    for (RequestMap::const_iterator i = m_requestsLoading.begin(); !(i == end); ++i) {
-        Request* r = i->second;
-        res += (r->docLoader() == dl && !r->isMultipart());
-    }
-
-    if (dl->loadInProgress())
-        res++;
-
-    return res;
-}
-
 void Loader::cancelRequests(DocLoader* dl)
 {
@@ -215 +204 @@
             cache()->remove(pIt.current()->cachedResource());
             m_requestsPending.remove(pIt);
+            dl->decrementRequestCount();
         } else
             ++pIt;
@@ -232 +222 @@
         didFail(loader, true);
     }
+    
+    if (dl->loadInProgress())
+        ASSERT(dl->requestCount() == 1);
+    else
+        ASSERT(dl->requestCount() == 0);
 }
 

trunk/WebCore/loader/loader.h

@@ -52 +52 @@
         void load(DocLoader*, CachedResource*, bool incremental = true, bool skipCanLoadCheck = false);
 
-        int numRequests(DocLoader*) const;
         void cancelRequests(DocLoader*);
-
     private:
         virtual void didReceiveResponse(SubresourceLoader*, const ResourceResponse&);