Changeset 91384 in webkit

Timestamp:

Jul 20, 2011 11:45:47 AM (13 years ago)

Author:

commit-queue@webkit.org

Message:

Patch by Scott Graham <​scottmg@chromium.org> on 2011-07-20
Reviewed by Antti Koivisto.

REGRESSION (r39725?): Resources removed from document can not be freed
until the document is deleted
​https://bugs.webkit.org/show_bug.cgi?id=61006

Upon completing a load start a Timer to iterate through
CachedResourceLoader's m_documentResources map to check for any items
that have only one reference (thus being the reference in the map
itself). The map should really be weak, but because the
CachedResourceHandle achieves bookkeeping work in addition to
reference counting, this is a simpler and more localized way to free
the used memory while maintaining the other behaviour (when
CachedResource is used as proxy).

No new layout tests, but with this patch the testcase at
​https://bugs.webkit.org/attachment.cgi?id=93850 should no longer
consume 400MB of ram on load.

• loader/cache/CachedResource.h:

(WebCore::CachedResource::getHandleCount):

• loader/cache/CachedResourceLoader.cpp:

(WebCore::CachedResourceLoader::loadDone):
(WebCore::CachedResourceLoader::garbageCollectDocumentResources):

• loader/cache/CachedResourceLoader.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/cache/CachedResource.h (modified) (1 diff)
• loader/cache/CachedResourceLoader.cpp (modified) (2 diffs)
• loader/cache/CachedResourceLoader.h (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-07-20  Scott Graham  <scottmg@chromium.org>
+
+        Reviewed by Antti Koivisto.
+
+        REGRESSION (r39725?): Resources removed from document can not be freed
+        until the document is deleted
+        https://bugs.webkit.org/show_bug.cgi?id=61006
+
+        Upon completing a load start a Timer to iterate through
+        CachedResourceLoader's m_documentResources map to check for any items
+        that have only one reference (thus being the reference in the map
+        itself). The map should really be weak, but because the
+        CachedResourceHandle achieves bookkeeping work in addition to
+        reference counting, this is a simpler and more localized way to free
+        the used memory while maintaining the other behaviour (when
+        CachedResource is used as proxy).
+
+        No new layout tests, but with this patch the testcase at
+        https://bugs.webkit.org/attachment.cgi?id=93850 should no longer
+        consume 400MB of ram on load.
+
+        * loader/cache/CachedResource.h:
+        (WebCore::CachedResource::getHandleCount):
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::loadDone):
+        (WebCore::CachedResourceLoader::garbageCollectDocumentResources):
+        * loader/cache/CachedResourceLoader.h:
+
 2011-07-20  James Robinson  <jamesr@chromium.org>
 

trunk/Source/WebCore/loader/cache/CachedResource.h

@@ -183 +183 @@
 
     bool canDelete() const { return !hasClients() && !m_request && !m_preloadCount && !m_handleCount && !m_resourceToRevalidate && !m_proxyResource; }
+    bool hasOneHandle() const { return m_handleCount == 1; }
 
     bool isExpired() const;

trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp

@@ -87 +87 @@
     : m_document(document)
     , m_requestCount(0)
+    , m_garbageCollectDocumentResourcesTimer(this, &CachedResourceLoader::garbageCollectDocumentResourcesTimerFired)
     , m_autoLoadImages(true)
     , m_loadFinishing(false)
@@ -562 +563 @@
         frame()->loader()->loadDone();
     performPostLoadActions();
+
+    if (!m_garbageCollectDocumentResourcesTimer.isActive())
+        m_garbageCollectDocumentResourcesTimer.startOneShot(0);
+}
+
+// Garbage collecting m_documentResources is a workaround for the
+// CachedResourceHandles on the RHS being strong references. Ideally this
+// would be a weak map, however CachedResourceHandles perform additional
+// bookkeeping on CachedResources, so instead pseudo-GC them -- when the
+// reference count reaches 1, m_documentResources is the only reference, so
+// remove it from the map.
+void CachedResourceLoader::garbageCollectDocumentResourcesTimerFired(Timer<CachedResourceLoader>* timer)
+{
+    Vector<String, 10> toDelete;
+
+    for (DocumentResourceMap::iterator it = m_documentResources.begin(); it != m_documentResources.end(); ++it) {
+        if (it->second->hasOneHandle())
+            toDelete.append(it->first);
+    }
+
+    for (Vector<String, 10>::const_iterator idel = toDelete.begin(); idel != toDelete.end(); ++idel)
+        m_documentResources.remove(*idel);
 }
 

trunk/Source/WebCore/loader/cache/CachedResourceLoader.h

@@ -31 +31 @@
 #include "CachePolicy.h"
 #include "ResourceLoadPriority.h"
+#include "Timer.h"
 #include <wtf/Deque.h>
 #include <wtf/HashMap.h>
@@ -118 +119 @@
     bool canRequest(CachedResource::Type, const KURL&, bool forPreload = false);
 
+    void garbageCollectDocumentResourcesTimerFired(Timer<CachedResourceLoader>*);
     void performPostLoadActions();
     
@@ -133 +135 @@
     };
     Deque<PendingPreload> m_pendingPreloads;
+
+    Timer<CachedResourceLoader> m_garbageCollectDocumentResourcesTimer;
     
     //29 bits left