Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1941123002:
Add UMA for hardware buffer size and callback buffer size (Closed)
|
Created:
4 years, 7 months ago by Raymond Toy Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews, kinuko+watch, hongchan, asvitkine+watch_chromium.org, Raymond Toy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA for hardware buffer size and callback buffer size
Measure and report the buffer size recommended by the hardware (for
minimum latency) and the callback buffer size that is actually used.
BUG=608510
TEST=manually create AudioContexts and examine chrome://histograms
Committed: https://crrev.com/9e34ffef06ca462936f1d158130cff4843e943af
Cr-Commit-Position: refs/heads/master@{#391528}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments #
Total comments: 2
Messages
Total messages: 19 (7 generated)
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
PTAL.
Nits on naming but lgtm. https://codereview.chromium.org/1941123002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/1941123002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:60: DEFINE_STATIC_LOCAL(SparseHistogram, audioHardwareBufferSizeHistogram, I think these naming make more sense: hardwareBufferSize: "WebAudio.AudioDestination.HardwareBufferSize" callbackBufferSize: "WebAudio.AudioDestination.CallbackBufferSize" https://codereview.chromium.org/1941123002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:68: size_t recommendedBufferSize = Platform::current()->audioHardwareBufferSize(); Perhaps |recommendedHardwareBufferSize| is better. https://codereview.chromium.org/1941123002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:97: callbackBufferSizeHistogram.sample(m_callbackBufferSize); ditto with the first comment. https://codereview.chromium.org/1941123002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1941123002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:58379: +<histogram name="WebAudio.AudioDestination.AudioHardwareBufferSize"> WebAudio.AudioDestination.HardwareBufferSize
https://codereview.chromium.org/1941123002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/1941123002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:60: DEFINE_STATIC_LOCAL(SparseHistogram, audioHardwareBufferSizeHistogram, On 2016/05/03 20:34:49, hoch wrote: > I think these naming make more sense: > > hardwareBufferSize: "WebAudio.AudioDestination.HardwareBufferSize" > callbackBufferSize: "WebAudio.AudioDestination.CallbackBufferSize" Done. https://codereview.chromium.org/1941123002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:68: size_t recommendedBufferSize = Platform::current()->audioHardwareBufferSize(); On 2016/05/03 20:34:49, hoch wrote: > Perhaps |recommendedHardwareBufferSize| is better. Done. https://codereview.chromium.org/1941123002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:97: callbackBufferSizeHistogram.sample(m_callbackBufferSize); On 2016/05/03 20:34:49, hoch wrote: > ditto with the first comment. Done. https://codereview.chromium.org/1941123002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1941123002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:58379: +<histogram name="WebAudio.AudioDestination.AudioHardwareBufferSize"> On 2016/05/03 20:34:49, hoch wrote: > WebAudio.AudioDestination.HardwareBufferSize Done.
rtoy@chromium.org changed reviewers: + mpearson@chromium.org
mpearson: PTAL at the histogram changes.
https://codereview.chromium.org/1941123002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/1941123002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:64: DEFINE_STATIC_LOCAL(SparseHistogram, callbackBufferSizeHistogram, Sparse histograms are slow. see here for details: https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/spars... Are you sure this is the right choice of your needs? (I think the answer is probably yes, but you should be aware you're putting a call that acquires a lock on every emit.)
https://codereview.chromium.org/1941123002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/1941123002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:64: DEFINE_STATIC_LOCAL(SparseHistogram, callbackBufferSizeHistogram, On 2016/05/03 21:35:23, Mark P wrote: > Sparse histograms are slow. see here for details: > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/spars... > Are you sure this is the right choice of your needs? > > (I think the answer is probably yes, but you should be aware you're putting a > call that acquires a lock on every emit.) This is updated only when the AudioContext is created, and we don't expect more than a handful per site so speed isn't a major factor here. The values are also fairly sparse, ranging from a low of 128 up to at least ~3000 (or more?) and typically a power of two.
histograms.xml lgtm Thanks for the explanation. --mark
The CQ bit was checked by rtoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hongchan@chromium.org Link to the patchset: https://codereview.chromium.org/1941123002/#ps20001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1941123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1941123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
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/1941123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1941123002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA for hardware buffer size and callback buffer size Measure and report the buffer size recommended by the hardware (for minimum latency) and the callback buffer size that is actually used. BUG=608510 TEST=manually create AudioContexts and examine chrome://histograms ========== to ========== Add UMA for hardware buffer size and callback buffer size Measure and report the buffer size recommended by the hardware (for minimum latency) and the callback buffer size that is actually used. BUG=608510 TEST=manually create AudioContexts and examine chrome://histograms Committed: https://crrev.com/9e34ffef06ca462936f1d158130cff4843e943af Cr-Commit-Position: refs/heads/master@{#391528} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9e34ffef06ca462936f1d158130cff4843e943af Cr-Commit-Position: refs/heads/master@{#391528} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
