Remove SharedBuffer::unlock() and keep Resource's SharedBuffer always locked

third_party/WebKit/Source/core/fetch/Resource.cpp

Index: third_party/WebKit/Source/core/fetch/Resource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/Resource.cpp b/third_party/WebKit/Source/core/fetch/Resource.cpp
index 96c1129075a39e2484a4907b38b952ff3223b79b..b403e003f19229a167f701a5658b470735f1d65e 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -556,19 +556,10 @@ bool Resource::unlock()
     if (!m_data)
         return false;
 
-    if (!m_data->isLocked())
-        return true;
-
     if (!memoryCache()->contains(this) || hasClientsOrObservers() || !isLoaded() || !isSafeToUnlock())
         return false;
 
-    if (RuntimeEnabledFeatures::doNotUnlockSharedBufferEnabled())
-        return false;
-
-    DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, unlockHistogram, new EnumerationHistogram("Blink.SharedBuffer.Unlock", kLastResourceType));
-    unlockHistogram.count(getType());
-
-    m_data->unlock();
+    memoryCache()->remove(this);

[haraken, 2016/08/17 08:13:31]

Would you help me understand why you need to proactively remove the Resource
from MemoryCache? What happens if you don't call memoryCache()->remove()? (i.e.,
simply remove Resource::unlock)

[hiroshige, 2016/08/17 09:02:38]

Because the Resource is not expected to be reused frequently, I remove it from
MemoryCache to save memory.
Functionally we can also simply remove the unlock() call here, and the two
choices will be the same after WeakMemoryCache is landed.
(Added "Alternatives Considered" section in the doc:
https://docs.google.com/document/d/1Q1G8dZ3k78N7DZaxf4wUAlKsSfwsclmhWqDIEMWZO...)

[haraken, 2016/08/17 09:10:25]

Makes sense.

I'm feeling that whether we call memoryCache()->remove() or not is an orthogonal
issue to this CL (I guess calling memoryCache()->remove() changes the cache hit
rate of MemoryCache). Would it make more sense to simply remove
Resource::unlock() in this CL and then improving the lifetime of MemoryCache
when you weaken the references in MemoryCache?

     return true;
 }
 
@@ -676,7 +667,6 @@ static bool shouldSendCachedDataSynchronouslyForType(Resource::Type type)
 
 void Resource::willAddClientOrObserver()
 {
-    ASSERT(!isPurgeable());
     if (m_preloadResult == PreloadNotReferenced) {
         if (isLoaded())
             m_preloadResult = PreloadReferencedWhileComplete;
@@ -843,13 +833,12 @@ void Resource::onMemoryDump(WebMemoryDumpLevelOfDetail levelOfDetail, WebProcess
     const String dumpName = getMemoryDumpName();
     WebMemoryAllocatorDump* dump = memoryDump->createMemoryAllocatorDump(dumpName);
     dump->addScalar("encoded_size", "bytes", m_encodedSize);
-    if (m_data && m_data->isLocked())
+    if (m_data)
         dump->addScalar("live_size", "bytes", m_encodedSize);
     else
         dump->addScalar("dead_size", "bytes", m_encodedSize);
 
     if (m_data) {
-        dump->addScalar("purgeable_size", "bytes", isPurgeable() ? encodedSize() + overheadSize() : 0);
         m_data->onMemoryDump(dumpName, memoryDump);
     }
 
@@ -983,38 +972,6 @@ bool Resource::canUseCacheValidator()
     return m_response.hasCacheValidatorFields() || m_resourceRequest.hasCacheValidatorFields();
 }
 
-bool Resource::isPurgeable() const
-{
-    return m_data && !m_data->isLocked();
-}
-
-bool Resource::lock()
-{
-    if (!m_data)
-        return true;
-    if (m_data->isLocked())
-        return true;
-
-    ASSERT(!hasClientsOrObservers());
-
-    // If locking fails, our buffer has been purged. There's no point
-    // in leaving a purged resource in MemoryCache.
-    if (!m_data->lock()) {
-        DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, failedLockHistogram, new EnumerationHistogram("Blink.SharedBuffer.FailedLock", kLastResourceType));
-        failedLockHistogram.count(getType());
-
-        m_data.clear();
-        setEncodedSize(0);
-        memoryCache()->remove(this);
-        return false;
-    }
-
-    DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, successfulLockHistogram, new EnumerationHistogram("Blink.SharedBuffer.SuccessfulLock", kLastResourceType));
-    successfulLockHistogram.count(getType());
-
-    return true;
-}
-
 size_t Resource::calculateOverheadSize() const
 {
     static const int kAverageClientsHashMapSize = 384;