Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1927313002:
Add histogram for IIRFilter orders (Closed)
|
Created:
4 years, 7 months ago by Raymond Toy Modified:
4 years, 7 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, haraken, hongchan Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histogram for IIRFilter orders
We want to keep track of the filter orders that are used for the
IIRFilterNode.
BUG=607665
TEST=none
Committed: https://crrev.com/85034d9ae88718eac42a093c22e330cd6fe218f9
Cr-Commit-Position: refs/heads/master@{#390796}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 8
Patch Set 4 : Address comments #
Messages
Total messages: 25 (4 generated)
rtoy@chromium.org changed reviewers: + hongchan@chromium.org, jbroman@chromium.org
PTAL.
I have one suggestion: Can we make a list of items being tracked by Histogram in a new file like |webaudio/UsageMetric.h|? (i.e. enum) Then we can clearly see which is tracked in one place. https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:25: DEFINE_STATIC_LOCAL(CustomCountHistogram, qHistogram, Why qHistogram? Does |q| have some meaning?
https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:25: DEFINE_STATIC_LOCAL(CustomCountHistogram, qHistogram, On 2016/04/28 22:07:07, hoch wrote: > Why qHistogram? Does |q| have some meaning? Don't know. I stole this example from the blink thread on this.
On 2016/04/28 22:07:07, hoch wrote: > I have one suggestion: Can we make a list of items being tracked by Histogram in > a new file like |webaudio/UsageMetric.h|? (i.e. enum) I have no idea what this really means. Is it not enough that histograms.xml has this information? > > Then we can clearly see which is tracked in one place. > > https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): > > https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:25: > DEFINE_STATIC_LOCAL(CustomCountHistogram, qHistogram, > Why qHistogram? Does |q| have some meaning?
On 2016/04/28 22:11:04, Raymond Toy wrote: > On 2016/04/28 22:07:07, hoch wrote: > > I have one suggestion: Can we make a list of items being tracked by Histogram > in > > a new file like |webaudio/UsageMetric.h|? (i.e. enum) > > I have no idea what this really means. Is it not enough that histograms.xml has > this information? > > > > Then we can clearly see which is tracked in one place. > > > > > https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): > > > > > https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:25: > > DEFINE_STATIC_LOCAL(CustomCountHistogram, qHistogram, > > Why qHistogram? Does |q| have some meaning? Then we can rewrite this: DEFINE_STATIC_LOCAL(CustomCountHistogram, qHistogram, (WebAudioTracker::IIRFilter.filterOrder, 1, IIRFilter::kMaxOrder, IIRFilter::kMaxOrder + 1)); If that makes sense.
I'm not a metrics expert; you will probably want the advice (and for histograms.xml, the l-g-t-m) of someone like asvitkine@ or isherman@. You might also consider: - verifying manually at chrome://histograms that your histogram is recorded as you expect - writing a test (using HistogramTester) https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:25: DEFINE_STATIC_LOCAL(CustomCountHistogram, qHistogram, nit: "qHistogram" was a name in my example because it was "Q" that was being recorded. Suggest a name that describes what this records ("orderHistogram"?). https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: ("WebAudio.IIRFilterNode.order", 0, IIRFilter::kMaxOrder, IIRFilter::kMaxOrder + 1)); CustomCountHistogram is, by default, logarithmic; here I think you want a linear histogram. I believe EnumerationHistogram is intended for cases like this: DEFINE_STATIC_LOCAL(EnumerationHistogram, orderHistogram, ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder + 1)); orderHistogram.count(feedbackCoef.size() - 1); https://codereview.chromium.org/1927313002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1927313002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57841: +<histogram name="WebAudio.IIRFilterNode.order"> nit: I think an uppercase O for "Order" is more consistent with the others. https://codereview.chromium.org/1927313002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57844: + <summary>The order of the WebAudio IIRFilterNode</summary> I suspect you'll be asked to expand this summary, at least to mention how often it's sampled. Perhaps something like: "The order of a WebAudio IIRFilterNode. Recorded each time a node is constructed."
https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:25: DEFINE_STATIC_LOCAL(CustomCountHistogram, qHistogram, On 2016/04/28 22:18:09, jbroman wrote: > nit: "qHistogram" was a name in my example because it was "Q" that was being > recorded. Suggest a name that describes what this records ("orderHistogram"?). Renamed to filterOrderHistogram. https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: ("WebAudio.IIRFilterNode.order", 0, IIRFilter::kMaxOrder, IIRFilter::kMaxOrder + 1)); On 2016/04/28 22:18:09, jbroman wrote: > CustomCountHistogram is, by default, logarithmic; here I think you want a linear > histogram. I believe EnumerationHistogram is intended for cases like this: > > DEFINE_STATIC_LOCAL(EnumerationHistogram, orderHistogram, > ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder + 1)); > orderHistogram.count(feedbackCoef.size() - 1); Changed to EnumerationHistogram. (It's impossible to figure out what these things mean from reading platform/Histogram.h.)
On 2016/04/28 22:18:09, jbroman wrote:
> I'm not a metrics expert; you will probably want the advice (and for
> histograms.xml, the l-g-t-m) of someone like asvitkine@ or isherman@.
>
> You might also consider:
> - verifying manually at chrome://histograms that your histogram is recorded as
> you expect
I tested this and it seems to work. The displayed histograms looks funny
though:
Histogram: WebAudio.IIRFilterNode.Order recorded 7 samples, average = 11.3
(flags = 0x1)
0 ------------------------------------------------------------------------O (1
= 14.3%)
1 ------------------------------------------------------------------------O (1
= 14.3%) {14.3%}
2 ------------------------------------------------------------------------O (1
= 14.3%) {28.6%}
3 ...
19 O (4
= 57.1%) {42.9%}
The numbers are correct, though. I created 1 filter of orders 0, 1, 2 and 4
filters of order 19 (the max allowed).
> - writing a test (using HistogramTester)
>
>
https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right):
>
>
https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:25:
> DEFINE_STATIC_LOCAL(CustomCountHistogram, qHistogram,
> nit: "qHistogram" was a name in my example because it was "Q" that was being
> recorded. Suggest a name that describes what this records ("orderHistogram"?).
>
>
https://codereview.chromium.org/1927313002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26:
> ("WebAudio.IIRFilterNode.order", 0, IIRFilter::kMaxOrder, IIRFilter::kMaxOrder
+
> 1));
> CustomCountHistogram is, by default, logarithmic; here I think you want a
linear
> histogram. I believe EnumerationHistogram is intended for cases like this:
>
> DEFINE_STATIC_LOCAL(EnumerationHistogram, orderHistogram,
> ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder + 1));
> orderHistogram.count(feedbackCoef.size() - 1);
>
>
https://codereview.chromium.org/1927313002/diff/20001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
>
>
https://codereview.chromium.org/1927313002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:57841: +<histogram
> name="WebAudio.IIRFilterNode.order">
> nit: I think an uppercase O for "Order" is more consistent with the others.
>
>
https://codereview.chromium.org/1927313002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:57844: + <summary>The order of the
> WebAudio IIRFilterNode</summary>
> I suspect you'll be asked to expand this summary, at least to mention how
often
> it's sampled. Perhaps something like:
>
> "The order of a WebAudio IIRFilterNode. Recorded each time a node is
> constructed."
On 2016/04/28 at 22:46:10, rtoy wrote:
> On 2016/04/28 22:18:09, jbroman wrote:
> > I'm not a metrics expert; you will probably want the advice (and for
> > histograms.xml, the l-g-t-m) of someone like asvitkine@ or isherman@.
> >
> > You might also consider:
> > - verifying manually at chrome://histograms that your histogram is recorded
as
> > you expect
>
> I tested this and it seems to work. The displayed histograms looks funny
though:
>
> Histogram: WebAudio.IIRFilterNode.Order recorded 7 samples, average = 11.3
(flags = 0x1)
> 0 ------------------------------------------------------------------------O
(1 = 14.3%)
> 1 ------------------------------------------------------------------------O
(1 = 14.3%) {14.3%}
> 2 ------------------------------------------------------------------------O
(1 = 14.3%) {28.6%}
> 3 ...
> 19 O
(4 = 57.1%) {42.9%}
>
>
> The numbers are correct, though. I created 1 filter of orders 0, 1, 2 and 4
filters of order 19 (the max allowed).
Hmm, maybe the UI for them is just funny; if the numbers are fine I'd expect
them to show up fine in the internal UMA dashboard.
rtoy@chromium.org changed reviewers: + isherman@chromium.org
isherman: PTAL
https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder)); Could this be a sparse histogram instead? Or is this code called in a performance-critical location? https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder)); 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. https://codereview.chromium.org/1927313002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1927313002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57845: + The order of the WebAudio IIRFilterNode. Recorded each time an IIRFilter is It would be nice to clarify what a node "order" is -- as someone unfamiliar with the feature, I can imagine a few possibilities.
https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder)); On 2016/04/29 20:51:40, Ilya Sherman wrote: > Could this be a sparse histogram instead? Or is this code called in a > performance-critical location? This is the constructor of the IIRFilterNode and is not time critical.
https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder)); On 2016/04/29 20:56:25, Raymond Toy wrote: > On 2016/04/29 20:51:40, Ilya Sherman wrote: > > Could this be a sparse histogram instead? Or is this code called in a > > performance-critical location? > > This is the constructor of the IIRFilterNode and is not time critical. Okay, so it sounds like it would be fine to use a sparse histogram then :)
https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder)); On 2016/04/29 21:01:04, Ilya Sherman wrote: > On 2016/04/29 20:56:25, Raymond Toy wrote: > > On 2016/04/29 20:51:40, Ilya Sherman wrote: > > > Could this be a sparse histogram instead? Or is this code called in a > > > performance-critical location? > > > > This is the constructor of the IIRFilterNode and is not time critical. > > Okay, so it sounds like it would be fine to use a sparse histogram then :) 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.)
https://codereview.chromium.org/1927313002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1927313002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57845: + The order of the WebAudio IIRFilterNode. Recorded each time an IIRFilter is On 2016/04/29 20:51:40, Ilya Sherman wrote: > It would be nice to clarify what a node "order" is -- as someone unfamiliar with > the feature, I can imagine a few possibilities. Done. Hope the new description is a bit better.
LGTM, thanks https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder)); On 2016/04/29 21:09:34, Raymond Toy wrote: > On 2016/04/29 21:01:04, Ilya Sherman wrote: > > On 2016/04/29 20:56:25, Raymond Toy wrote: > > > On 2016/04/29 20:51:40, Ilya Sherman wrote: > > > > Could this be a sparse histogram instead? Or is this code called in a > > > > performance-critical location? > > > > > > This is the constructor of the IIRFilterNode and is not time critical. > > > > Okay, so it sounds like it would be fine to use a sparse histogram then :) > > Yep. Working on that now and on your other comments. > > On a side note, what does the second arg (kMaxOrder) mean for the > EnumerationHistogram? 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). > 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.) 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.)
On 2016/04/29 21:42:10, Ilya Sherman wrote: > LGTM, thanks > > https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right): > > https://codereview.chromium.org/1927313002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:26: > ("WebAudio.IIRFilterNode.Order", IIRFilter::kMaxOrder)); > On 2016/04/29 21:09:34, Raymond Toy wrote: > > On 2016/04/29 21:01:04, Ilya Sherman wrote: > > > On 2016/04/29 20:56:25, Raymond Toy wrote: > > > > On 2016/04/29 20:51:40, Ilya Sherman wrote: > > > > > Could this be a sparse histogram instead? Or is this code called in a > > > > > performance-critical location? > > > > > > > > This is the constructor of the IIRFilterNode and is not time critical. > > > > > > Okay, so it sounds like it would be fine to use a sparse histogram then :) > > > > Yep. Working on that now and on your other comments. > > > > On a side note, what does the second arg (kMaxOrder) mean for the > > EnumerationHistogram? > > 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). > > > 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.) > > 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.) I think even 50 is overkill, and I would strongly oppose changing the spec to allow this. I think even 19'th order filters are very rare. But we'll know for sure what strange things people do when we have this histogram. :-) Thanks for the clarifications and review!
lgtm
The CQ bit was checked by rtoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927313002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927313002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/85034d9ae88718eac42a093c22e330cd6fe218f9 Cr-Commit-Position: refs/heads/master@{#390796}
Message was sent while issue was closed.
Description was changed from ========== Add histogram for IIRFilter orders We want to keep track of the filter orders that are used for the IIRFilterNode. BUG=607665 TEST=none ========== to ========== Add histogram for IIRFilter orders We want to keep track of the filter orders that are used for the IIRFilterNode. BUG=607665 TEST=none Committed: https://crrev.com/85034d9ae88718eac42a093c22e330cd6fe218f9 Cr-Commit-Position: refs/heads/master@{#390796} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
