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

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

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 538 matching lines @@
     m_response = response;
     if (m_response.wasFetchedViaServiceWorker())
         m_cacheHandler = ServiceWorkerResponseCachedMetadataHandler::create(this, m_fetcherSecurityOrigin.get());
 }
 
 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;
 }
 
 void Resource::responseReceived(const ResourceResponse& response, std::unique_ptr<WebDataConsumerHandle>)
 {
     m_responseTimestamp = currentTime();
     if (m_preloadDiscoveryTime) {
         int timeSinceDiscovery = static_cast<int>(1000 * (monotonicallyIncreasingTime() - m_preloadDiscoveryTime));
         DEFINE_STATIC_LOCAL(CustomCountHistogram, preloadDiscoveryToFirstByteHistogram, ("PreloadScanner.TTFB", 0, 10000, 50));
         preloadDiscoveryToFirstByteHistogram.count(timeSinceDiscovery);
@@ skipping 87 matching lines @@
         return true;
     if (type == Resource::Script)
         return true;
     if (type == Resource::Font)
         return true;
     return false;
 }
 
 void Resource::willAddClientOrObserver()
 {
-    ASSERT(!isPurgeable());
     if (m_preloadResult == PreloadNotReferenced) {
         if (isLoaded())
             m_preloadResult = PreloadReferencedWhileComplete;
         else if (isLoading())
             m_preloadResult = PreloadReferencedWhileLoading;
         else
             m_preloadResult = PreloadReferenced;
 
         if (m_preloadDiscoveryTime) {
             int timeSinceDiscovery = static_cast<int>(1000 * (monotonicallyIncreasingTime() - m_preloadDiscoveryTime));
@@ skipping 146 matching lines @@
 }
 
 void Resource::onMemoryDump(WebMemoryDumpLevelOfDetail levelOfDetail, WebProcessMemoryDump* memoryDump) const
 {
     static const size_t kMaxURLReportLength = 128;
     static const int kMaxResourceClientToShowInMemoryInfra = 10;
 
     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);
     }
 
     if (levelOfDetail == WebMemoryDumpLevelOfDetail::Detailed) {
         String urlToReport = url().getString();
         if (urlToReport.length() > kMaxURLReportLength) {
             urlToReport.truncate(kMaxURLReportLength);
             urlToReport = urlToReport + "...";
         }
         dump->addString("url", "", urlToReport);
@@ skipping 113 matching lines @@
     if (hasCacheControlNoStoreHeader())
         return false;
 
     // Do not revalidate Resource with redirects. https://crbug.com/613971
     if (!redirectChain().isEmpty())
         return false;
 
     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;
     return sizeof(Resource) + m_response.memoryUsage() + kAverageClientsHashMapSize + m_resourceRequest.url().getString().length() * 2;
 }
 
 void Resource::didChangePriority(ResourceLoadPriority loadPriority, int intraPriorityValue)
 {
     m_resourceRequest.setPriority(loadPriority, intraPriorityValue);
     if (m_loader)
@@ skipping 83 matching lines @@
     case Resource::TextTrack:
     case Resource::Media:
     case Resource::Manifest:
         return false;
     }
     ASSERT_NOT_REACHED();
     return false;
 }
 
 } // namespace blink