Put histogram code in disk_cache on a diet.

net/disk_cache/blockfile/histogram_macros.h

Index: net/disk_cache/blockfile/histogram_macros.h
diff --git a/net/disk_cache/blockfile/histogram_macros.h b/net/disk_cache/blockfile/histogram_macros.h
index a5d6f56f5c10ca888b78c97e5c4cb8ac6793d24b..be8a98fc9efe32b68b94d554efcd9554cdad760f 100644
--- a/net/disk_cache/blockfile/histogram_macros.h
+++ b/net/disk_cache/blockfile/histogram_macros.h
@@ -14,16 +14,16 @@
 // -----------------------------------------------------------------------------
 
 // These histograms follow the definition of UMA_HISTOGRAMN_XXX except that
-// whenever the name changes (the experiment group changes), the histrogram
-// object is re-created.
+// the counter is not cached locally.
 // Note: These macros are only run on one thread, so the declarations of
-// |counter| was made static (i.e., there will be no race for reinitialization).
+// |counter| could be static (since there will be no race for reinitialization),
+// but as they are used (with new names every time) in CACHE_UMA that
+// would not work and if CACHE_UMA is made to have one instance for every
+// cache, they will generate too much non-reusable machine code.
 
 #define CACHE_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \
     do { \
-      static base::HistogramBase* counter(NULL); \
-      if (!counter || name != counter->histogram_name()) \
-        counter = base::Histogram::FactoryGet( \
+      base::HistogramBase* counter = base::Histogram::FactoryGet( \

[rvargas (doing something else), 2014/03/14 21:48:41]

Does this mean that we should change UMA_HISTOGRAM_XXX to eliminate the static
object?

I mean, these macros basically generate n independent histograms (ignore the
experiment part as that's not currently used and unlikely to be used again) that
could be as well named "AppCache.Foo", "HttpCache.Foo" and "ShaderCache.Foo".

So yes, there's a lot of histograms here, but there's a lot of histograms in
general.

[Daniel Bratell, 2014/03/17 09:46:57]

I think removing static from all UVA macros could be the right thing to do but
there might be one or two places where static is needed to not make the lookup
become a bottleneck.

This code was somewhat of a special case because of the use in the switch which
amplified the overhead. I basically found this by noticing that a few disk cache
functions were on the list of largest functions in Chromium (before the patch,
not afterwards).

[gavinp, 2014/03/17 10:15:28]

I think this would be great. It's very difficult for people to understand what
.0, .1, etc.. mean. I imagine there's a small benefit in binary size (because
histograms store the strings explicitly), but even this need not be so if we
generate them programatically.

[gavinp, 2014/03/17 10:15:28]

A few weeks ago, I started looking at this. It turns out that not only are these
UMA a cause of bloat, but they are unusually slow on ARM because of very
conservative dependent load semantics.

If you're interested, see https://codereview.chromium.org/148063009/ for the
start of this work (which keeps the semantics, but saves space on ARM at very
little cost on x86).

I'm hoping to continue that, but I wanted to look more carefully at the perf
gains from being more aggressive with memory barriers.

[rvargas (doing something else), 2014/03/17 18:03:08]

I would be more interested in an open bug.

[rvargas (doing something else), 2014/03/17 18:03:08]

How does the switch amplifies the overhead? What it enables is to generate more
histograms with less lines of source code, but if function xx wants to generate
5 different histograms using standard base UMA macros (with statics), I would
expect it to generate roughly the same binary code than one call of this macro.
Is that not the case?

[Daniel Bratell, 2014/03/17 19:35:54]

The "static" just prevented the optimizer from reusing the same code for all
paths in the code, even though the code looked the same because the value of the
histogram (hidden) variable depended on how the input the the first time that
code path was reached.

Having five distinct blocks for the five different histograms, distinct enough
to prevent any merge of code, would probably cause the same accidental bloating.

There is also a cost an overhead to static variables in that there has to be
machine code to check if it's initialized or not every time you reach it as well
as other things.

If you look at the size savings, you see that ReportStats shrunk by more than
80% so it's not just that there a fifth of the macro left, there is more to it
as well.

             name, min, max, bucket_count, \
             base::Histogram::kUmaTargetedHistogramFlag); \
       counter->Add(sample); \
@@ -40,9 +40,7 @@
 
 #define CACHE_HISTOGRAM_CUSTOM_TIMES(name, sample, min, max, bucket_count) \
     do { \
-      static base::HistogramBase* counter(NULL); \
-      if (!counter || name != counter->histogram_name()) \
-        counter = base::Histogram::FactoryTimeGet( \
+      base::HistogramBase* counter = base::Histogram::FactoryTimeGet( \
             name, min, max, bucket_count, \
             base::Histogram::kUmaTargetedHistogramFlag); \
       counter->AddTime(sample); \
@@ -53,9 +51,7 @@
     base::TimeDelta::FromSeconds(10), 50)
 
 #define CACHE_HISTOGRAM_ENUMERATION(name, sample, boundary_value) do { \
-    static base::HistogramBase* counter(NULL); \
-    if (!counter || name != counter->histogram_name()) \
-      counter = base::LinearHistogram::FactoryGet( \
+    base::HistogramBase* counter = base::LinearHistogram::FactoryGet( \
                     name, 1, boundary_value, boundary_value + 1, \
                     base::Histogram::kUmaTargetedHistogramFlag); \
     counter->Add(sample); \
@@ -98,24 +94,16 @@
     const std::string my_name =\
         CACHE_UMA_BACKEND_IMPL_OBJ->HistogramName(name, experiment);\
     switch (CACHE_UMA_BACKEND_IMPL_OBJ->cache_type()) {\

[gavinp, 2014/03/17 10:15:28]

Does it make sense to keep this as a switch statement now? I imagine a DCHECK
would work well for this now.

[rvargas (doing something else), 2014/03/17 18:03:08]

I would actually prefer to keep a switch (or equivalent), as I don't want new
cache types to generate new histograms automatically. In fact, having PNACL
histograms was probably a mistake (likely to be removed soon).

That intention was more clear with the v3 version (although to be fair,
irrelevant as that code is not currently used in prod)

+      default:\
+        NOTREACHED();\
+        /* Fall-through. */\
       case net::DISK_CACHE:\
-        CACHE_HISTOGRAM_##type(my_name.data(), sample);\
-        break;\
       case net::MEDIA_CACHE:\
-        CACHE_HISTOGRAM_##type(my_name.data(), sample);\
-        break;\
       case net::APP_CACHE:\
-        CACHE_HISTOGRAM_##type(my_name.data(), sample);\
-        break;\
       case net::SHADER_CACHE:\
-        CACHE_HISTOGRAM_##type(my_name.data(), sample);\
-        break;\
       case net::PNACL_CACHE:\
         CACHE_HISTOGRAM_##type(my_name.data(), sample);\
         break;\
-      default:\
-        NOTREACHED();\
-        break;\
     }\
   }