MemoryCache: Enable MemoryCache to have multiple isolated resource maps

Source/core/fetch/MemoryCache.cpp

Index: Source/core/fetch/MemoryCache.cpp
diff --git a/Source/core/fetch/MemoryCache.cpp b/Source/core/fetch/MemoryCache.cpp
index 4aa2a5aca2024e0ee814717db0681b78b05a0111..9d10f0f8f5772d3b391a3805f3b92a7b6b816448 100644
--- a/Source/core/fetch/MemoryCache.cpp
+++ b/Source/core/fetch/MemoryCache.cpp
@@ -153,12 +153,25 @@ KURL MemoryCache::removeFragmentIdentifierIfNeeded(const KURL& originalURL)
     return url;
 }
 
+String MemoryCache::defaultCacheIdentifier()
+{
+    return emptyString();
+}
+
+MemoryCache::ResourceMap* MemoryCache::getResourceMap(const String& cacheIdentifier)
+{
+    if (!m_resources.contains(cacheIdentifier))
+        m_resources.add(cacheIdentifier, adoptPtrWillBeNoop(new ResourceMap()));

[Mike West, 2014/10/24 12:42:59]

`add()` can fail, can't it? That could cause a null-deref on line 173.

[nhiroki, 2014/10/27 14:30:12]

Hmmm... I think a null-deref never happens because "add()" can fail (can do
nothing) only if "m_resources" has already contained a key "cacheIdentifier" and
it's confirmed at line 163. You mean other cases? (eg. out-of-memory??)

+    return m_resources.get(cacheIdentifier);
+}
+
 void MemoryCache::add(Resource* resource)
 {
     ASSERT(WTF::isMainThread());
     ASSERT(resource->url().isValid());
-    RELEASE_ASSERT(!m_resources.contains(resource->url()));
-    m_resources.set(resource->url().string(), MemoryCacheEntry::create(resource));
+    ResourceMap* resources = getResourceMap(resource->cacheIdentifier());
+    RELEASE_ASSERT(!resources->contains(resource->url()));
+    resources->set(resource->url(), MemoryCacheEntry::create(resource));
     update(resource, 0, resource->size(), true);
 
     WTF_LOG(ResourceLoading, "MemoryCache::add Added '%s', resource %p\n", resource->url().string().latin1().data(), resource);
@@ -166,11 +179,13 @@ void MemoryCache::add(Resource* resource)
 
 void MemoryCache::replace(Resource* newResource, Resource* oldResource)
 {
-    if (MemoryCacheEntry* oldEntry = m_resources.get(oldResource->url()))
+    ASSERT(newResource->cacheIdentifier() == oldResource->cacheIdentifier());
+    ResourceMap* resources = getResourceMap(oldResource->cacheIdentifier());
+    if (MemoryCacheEntry* oldEntry = resources->get(oldResource->url()))
         evict(oldEntry);
     add(newResource);
     if (newResource->decodedSize() && newResource->hasClients())
-        insertInLiveDecodedResourcesList(m_resources.get(newResource->url()));
+        insertInLiveDecodedResourcesList(resources->get(newResource->url()));
 }
 
 void MemoryCache::remove(Resource* resource)
@@ -179,22 +194,26 @@ void MemoryCache::remove(Resource* resource)
     // who needed a fresh copy for a reload.
     if (!contains(resource))

[Mike West, 2014/10/24 12:42:59]

Rather than doing the lookup twice, it might make sense to phrase it as:

`if (ResourceCacheEntry* entry = getEntryForResource(resource)) { ... }`

[nhiroki, 2014/10/27 14:30:13]

Good point! Removed redundant contains() calls.

         return;
-    evict(m_resources.get(resource->url()));
+    evict(getEntryForResource(resource));
 }
 
 bool MemoryCache::contains(const Resource* resource) const
 {
     if (resource->url().isNull())
         return false;
-    const MemoryCacheEntry* entry = m_resources.get(resource->url());
+    ResourceMap* resources = m_resources.get(resource->cacheIdentifier());

[Mike West, 2014/10/24 12:43:00]

It's odd to have `getResourceMap` and not use it here. I understand why; you
don't want the side-effect of creating the resourceMap if it doesn't exist.
Perhaps you could rename `getResourceMap` to `ensureResourceMap` or something
similar to resolve the confusion?

[nhiroki, 2014/10/27 14:30:12]

Yes, that's right.
Sounds reasonable. Renamed it to ensureResourceMap.

+    if (!resources)
+        return false;
+    MemoryCacheEntry* entry = resources->get(resource->url());
     return entry && entry->m_resource == resource;
 }
 
-Resource* MemoryCache::resourceForURL(const KURL& resourceURL)
+Resource* MemoryCache::resourceForURL(const String& cacheIdentifier, const KURL& resourceURL)
 {
     ASSERT(WTF::isMainThread());
+    ResourceMap* resources = getResourceMap(cacheIdentifier);
     KURL url = removeFragmentIdentifierIfNeeded(resourceURL);
-    MemoryCacheEntry* entry = m_resources.get(url);
+    MemoryCacheEntry* entry = resources->get(url);
     if (!entry)
         return 0;
     Resource* resource = entry->m_resource.get();
@@ -207,6 +226,19 @@ Resource* MemoryCache::resourceForURL(const KURL& resourceURL)
     return resource;
 }
 
+WillBeHeapVector<Member<Resource>> MemoryCache::resourcesForURL(const KURL& resourceURL)
+{
+    ASSERT(WTF::isMainThread());
+    KURL url = removeFragmentIdentifierIfNeeded(resourceURL);
+    WillBeHeapVector<Member<Resource>> results;
+    for (const auto& resources : m_resources) {

[Mike West, 2014/10/24 12:42:59]

"ResourceMap" seems clearer than "auto".

[nhiroki, 2014/10/27 14:30:12]

"m_resources" is a HashMap object, so this "auto" is not inferred as
"ResourceMap" but "ResourceMapIndex::ValueType". You mean ...

for (const OwnPtr<ResourceMap>& resources : m_resources.values()) {...}

? Or am I missing something?

+        MemoryCacheEntry* entry = resources.value->get(url);
+        if (entry)
+            results.append(entry->m_resource.get());
+    }
+    return results;
+}
+
 size_t MemoryCache::deadCapacity() const
 {
     // Dead resource capacity is whatever space is not occupied by live resources, bounded by an independent minimum and maximum.
@@ -374,15 +406,16 @@ bool MemoryCache::evict(MemoryCacheEntry* entry)
     update(resource, resource->size(), 0, false);
     removeFromLiveDecodedResourcesList(entry);
 
-    ResourceMap::iterator it = m_resources.find(resource->url());
-    ASSERT(it != m_resources.end());
+    ResourceMap* resources = getResourceMap(resource->cacheIdentifier());
+    ResourceMap::iterator it = resources->find(resource->url());
+    ASSERT(it != resources->end());
 #if ENABLE(OILPAN)
     MemoryCacheEntry* entryPtr = it->value;
 #else
     OwnPtr<MemoryCacheEntry> entryPtr;
     entryPtr.swap(it->value);
 #endif
-    m_resources.remove(it);
+    resources->remove(it);
 #if ENABLE(OILPAN)
     if (entryPtr)
         entryPtr->dispose();
@@ -390,6 +423,19 @@ bool MemoryCache::evict(MemoryCacheEntry* entry)
     return canDelete;
 }
 
+MemoryCacheEntry* MemoryCache::getEntryForResource(const Resource* resource) const
+{
+    if (resource->url().isNull())

[Mike West, 2014/10/24 12:43:00]

isNull or isEmpty?

[nhiroki, 2014/10/27 14:30:13]

Done.

+        return nullptr;
+    ResourceMap* resources = m_resources.get(resource->cacheIdentifier());
+    if (!resources)
+        return nullptr;
+    MemoryCacheEntry* entry = resources->get(resource->url());
+    if (!entry || entry->m_resource != resource)
+        return nullptr;
+    return entry;
+}
+
 MemoryCacheLRUList* MemoryCache::lruListFor(unsigned accessCount, size_t size)
 {
     ASSERT(accessCount > 0);
@@ -522,14 +568,14 @@ void MemoryCache::makeDead(Resource* resource)
         return;
     m_liveSize -= resource->size();
     m_deadSize += resource->size();
-    removeFromLiveDecodedResourcesList(m_resources.get(resource->url()));
+    removeFromLiveDecodedResourcesList(getEntryForResource(resource));
 }
 
 void MemoryCache::update(Resource* resource, size_t oldSize, size_t newSize, bool wasAccessed)
 {
     if (!contains(resource))
         return;
-    MemoryCacheEntry* entry = m_resources.get(resource->url());
+    MemoryCacheEntry* entry = getEntryForResource(resource);
 
     // The object must now be moved to a different queue, since either its size or its accessCount has been changed,
     // and both of those are used to determine which LRU queue the resource should be in.
@@ -554,7 +600,7 @@ void MemoryCache::updateDecodedResource(Resource* resource, UpdateReason reason,
 {
     if (!contains(resource))
         return;
-    MemoryCacheEntry* entry = m_resources.get(resource->url());
+    MemoryCacheEntry* entry = getEntryForResource(resource);
 
     removeFromLiveDecodedResourcesList(entry);
     if (priority != MemoryCacheLiveResourcePriorityUnknown && priority != entry->m_liveResourcePriority)
@@ -575,7 +621,7 @@ MemoryCacheLiveResourcePriority MemoryCache::priority(Resource* resource) const
 {
     if (!contains(resource))
         return MemoryCacheLiveResourcePriorityUnknown;
-    MemoryCacheEntry* entry = m_resources.get(resource->url());
+    MemoryCacheEntry* entry = getEntryForResource(resource);
     return entry->m_liveResourcePriority;
 }
 
@@ -591,7 +637,8 @@ void MemoryCache::removeURLFromCache(ExecutionContext* context, const KURL& url)
 
 void MemoryCache::removeURLFromCacheInternal(ExecutionContext*, const KURL& url)
 {
-    if (Resource* resource = memoryCache()->resourceForURL(url))
+    auto resources = memoryCache()->resourcesForURL(url);
+    for (const auto& resource : resources)

[Mike West, 2014/10/24 12:42:59]

"Resource" seems clearer than "auto".

[nhiroki, 2014/10/27 14:30:12]

Done.

         memoryCache()->remove(resource);
 }
 
@@ -613,27 +660,29 @@ void MemoryCache::TypeStatistic::addResource(Resource* o)
 MemoryCache::Statistics MemoryCache::getStatistics()
 {
     Statistics stats;
-    for (const auto& resourceIter : m_resources) {
-        Resource* resource = resourceIter.value->m_resource.get();
-        switch (resource->type()) {
-        case Resource::Image:
-            stats.images.addResource(resource);
-            break;
-        case Resource::CSSStyleSheet:
-            stats.cssStyleSheets.addResource(resource);
-            break;
-        case Resource::Script:
-            stats.scripts.addResource(resource);
-            break;
-        case Resource::XSLStyleSheet:
-            stats.xslStyleSheets.addResource(resource);
-            break;
-        case Resource::Font:
-            stats.fonts.addResource(resource);
-            break;
-        default:
-            stats.other.addResource(resource);
-            break;
+    for (const auto& resources : m_resources) {

[Mike West, 2014/10/24 12:42:59]

"ResourceMap* resourceMap" seems clearer.

[nhiroki, 2014/10/27 14:30:13]

Ditto ("const OwnPtr<ResourceMap>& resources"?).

+        for (const auto& resourceIter : *resources.value) {

[Mike West, 2014/10/24 12:42:59]

Resource rather than auto.

[nhiroki, 2014/10/27 14:30:12]

Ditto.

+            Resource* resource = resourceIter.value->m_resource.get();
+            switch (resource->type()) {
+            case Resource::Image:
+                stats.images.addResource(resource);
+                break;
+            case Resource::CSSStyleSheet:
+                stats.cssStyleSheets.addResource(resource);
+                break;
+            case Resource::Script:
+                stats.scripts.addResource(resource);
+                break;
+            case Resource::XSLStyleSheet:
+                stats.xslStyleSheets.addResource(resource);
+                break;
+            case Resource::Font:
+                stats.fonts.addResource(resource);
+                break;
+            default:
+                stats.other.addResource(resource);
+                break;
+            }
         }
     }
     return stats;
@@ -642,10 +691,17 @@ MemoryCache::Statistics MemoryCache::getStatistics()
 void MemoryCache::evictResources()
 {
     for (;;) {
-        ResourceMap::iterator i = m_resources.begin();
-        if (i == m_resources.end())
+        ResourceMapIndex::iterator indexIter = m_resources.begin();
+        if (indexIter == m_resources.end())
             break;
-        evict(i->value.get());
+        ResourceMap* resources = indexIter->value.get();
+        for (;;) {

[Mike West, 2014/10/24 12:42:59]

Would you mind changing this, and line 693, to `while (true)`? The latter is
much more common in Blink code.

[nhiroki, 2014/10/27 14:30:13]

Done.

+            ResourceMap::iterator resourceIter = resources->begin();
+            if (resourceIter == resources->end())
+                break;
+            evict(resourceIter->value.get());
+        }
+        m_resources.remove(indexIter);
     }
 }
 
@@ -690,7 +746,7 @@ void MemoryCache::prune(Resource* justReleasedResource)
         // Main Resources in the cache are only substitue data that was
         // precached and should not be evicted.
         if (contains(justReleasedResource) && justReleasedResource->type() != Resource::MainResource)
-            evict(m_resources.get(justReleasedResource->url()));
+            evict(getEntryForResource(justReleasedResource));
 
         // As a last resort, prune immediately
         if (m_deadSize > m_maxDeferredPruneDeadCapacity)