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:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@21245 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 646f214..2cc1468 100644
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,4 +1,41 @@
-<<<<<<< .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>
Reviewed by Brady Eidson.
@@ -18,7 +55,6 @@
the function name.
* loader/FrameLoader.h:
-=======
2007-05-03 Justin Garcia <justin.garcia@apple.com>
Reviewed by harrison.
@@ -31,7 +67,6 @@
(WebCore::createMarkup):
Nil-check checkAncestor and lastClosed.
->>>>>>> .r21243
2007-05-03 Timothy Hatcher <timothy@apple.com>
Reviewed by Kevin.
diff --git a/WebCore/loader/DocLoader.cpp b/WebCore/loader/DocLoader.cpp
index bd8bedc..b94cd44 100644
--- a/WebCore/loader/DocLoader.cpp
+++ b/WebCore/loader/DocLoader.cpp
@@ -45,6 +45,7 @@
, m_cachePolicy(CachePolicyVerify)
, m_frame(frame)
, m_doc(doc)
+ , m_requestCount(0)
, m_autoLoadImages(true)
, m_loadInProgress(false)
, m_allowStaleResources(false)
@@ -205,4 +206,22 @@
m_frame->loader()->didTellBridgeAboutLoad(resource->url());
}
+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;
+}
+
}
diff --git a/WebCore/loader/DocLoader.h b/WebCore/loader/DocLoader.h
index 1e4a0ab..e62bfaf 100644
--- a/WebCore/loader/DocLoader.h
+++ b/WebCore/loader/DocLoader.h
@@ -88,6 +88,9 @@
void replaceDocument(Document* doc) { m_doc = doc; }
#endif
+ void incrementRequestCount();
+ void decrementRequestCount();
+ int requestCount();
private:
CachedResource* requestResource(CachedResource::Type, const String& url, const String* charset = 0, bool skipCanLoadCheck = false);
@@ -101,6 +104,8 @@
Frame* m_frame;
Document *m_doc;
+ int m_requestCount;
+
//29 bits left
bool m_autoLoadImages : 1;
bool m_loadInProgress : 1;
diff --git a/WebCore/loader/FrameLoader.cpp b/WebCore/loader/FrameLoader.cpp
index 0bf11a0..5acbbcc 100644
--- a/WebCore/loader/FrameLoader.cpp
+++ b/WebCore/loader/FrameLoader.cpp
@@ -195,9 +195,10 @@
static int numRequests(Document* document)
{
- if (document)
- return cache()->loader()->numRequests(document->docLoader());
- return 0;
+ if (!document)
+ return 0;
+
+ return document->docLoader()->requestCount();
}
FrameLoader::FrameLoader(Frame* frame, FrameLoaderClient* client)
@@ -1126,8 +1127,8 @@
return;
// 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;
#if USE(LOW_BANDWIDTH_DISPLAY)
diff --git a/WebCore/loader/loader.cpp b/WebCore/loader/loader.cpp
index 7fe515e..9c5642b 100644
--- a/WebCore/loader/loader.cpp
+++ b/WebCore/loader/loader.cpp
@@ -58,8 +58,10 @@
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);
}
@@ -70,25 +72,28 @@
// get the first pending request
Request* req = m_requestsPending.take(0);
+ DocLoader* dl = req->docLoader();
+ dl->decrementRequestCount();
ResourceRequest request(req->cachedResource()->url());
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();
+ }
}
void Loader::didFinishLoading(SubresourceLoader* loader)
@@ -99,9 +104,11 @@
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);
object->data(loader->resourceData(), true);
@@ -126,9 +133,11 @@
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) {
docLoader->setLoadInProgress(true);
@@ -160,6 +169,10 @@
req->docLoader()->frame()->loader()->checkCompleted();
} 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());
if (!req->cachedResource()->isImage())
@@ -183,30 +196,6 @@
object->data(loader->resourceData(), false);
}
-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)
{
DeprecatedPtrListIterator<Request> pIt(m_requestsPending);
@@ -214,6 +203,7 @@
if (pIt.current()->docLoader() == dl) {
cache()->remove(pIt.current()->cachedResource());
m_requestsPending.remove(pIt);
+ dl->decrementRequestCount();
} else
++pIt;
}
@@ -231,6 +221,11 @@
SubresourceLoader* loader = loadersToCancel[i];
didFail(loader, true);
}
+
+ if (dl->loadInProgress())
+ ASSERT(dl->requestCount() == 1);
+ else
+ ASSERT(dl->requestCount() == 0);
}
} //namespace WebCore
diff --git a/WebCore/loader/loader.h b/WebCore/loader/loader.h
index 3f8ae35..c02dc29 100644
--- a/WebCore/loader/loader.h
+++ b/WebCore/loader/loader.h
@@ -51,9 +51,7 @@
void load(DocLoader*, CachedResource*, bool incremental = true, bool skipCanLoadCheck = false);
- int numRequests(DocLoader*) const;
void cancelRequests(DocLoader*);
-
private:
virtual void didReceiveResponse(SubresourceLoader*, const ResourceResponse&);
virtual void didReceiveData(SubresourceLoader*, const char*, int);
