Add histogram for IIRFilter orders

third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp

Index: third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp
diff --git a/third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp b/third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp
index 0cf20480324d354b2f7beddd9f74b94e074f3051..ac9c3af7ba24c3af68e233be0fda8ac23d0eaaff 100644
--- a/third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp
+++ b/third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp
@@ -8,6 +8,7 @@
 #include "bindings/core/v8/ExceptionState.h"
 #include "core/dom/ExceptionCode.h"
 #include "modules/webaudio/AudioBasicProcessorHandler.h"
+#include "platform/Histogram.h"
 
 namespace blink {
 
@@ -17,6 +18,14 @@ IIRFilterNode::IIRFilterNode(AbstractAudioContext& context, float sampleRate, co
     setHandler(AudioBasicProcessorHandler::create(
         AudioHandler::NodeTypeIIRFilter, *this, sampleRate,
         adoptPtr(new IIRProcessor(sampleRate, 1, feedforwardCoef, feedbackCoef))));
+
+    // Histogram of the IIRFilter order.  createIIRFilter ensures that the length of |feedbackCoef|
+    // is in the range [1, IIRFilter::kMaxOrder + 1].  The order is one less than the length of this
+    // vector.
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, filterOrderHistogram,
+        ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder));

[Ilya Sherman, 2016/04/29 20:51:40]

Could this be a sparse histogram instead?  Or is this code called in a
performance-critical location?

[Ilya Sherman, 2016/04/29 20:51:40]

Could you please add a compile-time assertion to verify that kMaxOrder is at
most 50?  (Just picking a reasonable number.)  UMA histograms don't do well with
unbounded values, so the assertion would just be a guard against someone later
changing this value and not thinking about the UMA use of it.

[Raymond Toy, 2016/04/29 20:56:25]

This is the constructor of the IIRFilterNode and is not time critical.

[Ilya Sherman, 2016/04/29 21:01:04]

Okay, so it sounds like it would be fine to use a sparse histogram then :)

[Raymond Toy, 2016/04/29 21:09:34]

Yep.  Working on that now and on your other comments.

On a side note, what does the second arg (kMaxOrder) mean for the
EnumerationHistogram?

And if I change to SparseHistogram, do I still need a check on kMaxOrder?  Per
spec, the largest this can be today is 19 (20 coef max).  It's very unlikely
that this will ever get larger, even in future versions. (High orders have bad
numerical properties and are hard to ensure they are stable.)

[Ilya Sherman, 2016/04/29 21:42:10]

It specifies the minimum value of the overflow bucket.  Actually, I should have
spotted that in my previous review: This should have been kMaxOrder+1, so that
the overflow bucket isn't actually used (which therefore allows you to resize
the histogram, if need be).
If you think it's very unlikely that this constant would ever exceed, say, 1000,
then there's no check needed for the sparse histogram case.  (A good rule of
thumb, in general, is to use ~50 buckets for dense histograms, and to use up to
~1000 buckets across all clients for sparse histograms.)

+
+    filterOrderHistogram.count(feedbackCoef.size() - 1);
 }
 
 DEFINE_TRACE(IIRFilterNode)