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

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 20b69e38de6bd385ea801e11c98a8aafe177faef..20691353bf0ef31b5719e3deb64a3a1d27647b33 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -564,6 +564,9 @@ bool Resource::unlock()
     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;
 }
@@ -986,11 +989,18 @@ bool Resource::lock()
     // 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;
 }