Add UMAs for SharedBuffer::lock()/unlock()

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 546 matching lines @@
 
     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", Manifest + 1));

[bashi, 2016/07/11 08:02:27]

nit: does it make sense to change the name of metric to Blink.Resource.Unlock
(or move this to SharedBuffer::unlock()) ?

[hiroshige, 2016/07/11 08:15:58]

I also considered that option, but I names Blink.SharedBuffer.Unlock and placed
the code here because:
- "Blink.Resource.Unlock" looks like counting the number of Resource::unlock()
calls, which is larger than that of SharedBuffer::unlock().
- This is the only callsite of SharedBuffer::unlock(), so moving this to
SharedBuffer::unlock() doesn't affect the UMA result.
- Putting the code in SharedBuffer::unlock() needs passing getType() to
SharedBuffer, which is not nice.

[bashi, 2016/07/11 08:23:22]

I see. Thanks for the explanation.

[Ilya Sherman, 2016/07/11 19:28:33]

Please document (in the header file, where the enum is defined) that this enum
is now being used to back an UMA histogram, and should therefore be treated as
append-only.

[hiroshige, 2016/07/12 10:06:29]

Done.

+    unlockHistogram.count(getType());
+
     m_data->unlock();
     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));
@@ skipping 402 matching lines @@
     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", Manifest + 1));

[Ilya Sherman, 2016/07/11 19:28:33]

Using "Manifest + 1" as a boundary value is a bit fragile: If anyone updates the
enum, they might not notice that they should update this callsite as well.  For
this reason, UMA enums typically have a "COUNT" or "LAST" meta-entry, which is
much more likely to remain correctly up-to-date.

[hiroshige, 2016/07/12 10:06:29]

Added |kLastResourceType|.

I avoid adding |Last| entry directly to |enum Type|, because adding |Last|
causes compile errors like:
    error: enumeration value 'Last' not handled in switch ...

+        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", Manifest + 1));
+    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)
@@ skipping 124 matching lines @@
     case Resource::Media:
         return "Media";
     case Resource::Manifest:
         return "Manifest";
     }
     ASSERT_NOT_REACHED();
     return "Unknown";
 }
 
 } // namespace blink