Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124503002:
UMA stats for AudioRendererSinkCache utilization. (Closed)
|
Created:
4 years, 5 months ago by o1ka Modified:
4 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, Henrik Grunell, Steven Holte Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUMA stats for AudioRendererSinkCache utilization.
BUG=586161
Committed: https://crrev.com/7fa156327c1b65b647d8f1dac59902191dc56c10
Cr-Commit-Position: refs/heads/master@{#405085}
Patch Set 1 #
Total comments: 11
Patch Set 2 : review comments addressed #
Total comments: 2
Patch Set 3 : review comments addressed #
Total comments: 4
Patch Set 4 : addressing review comments #
Messages
Total messages: 40 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
olka@chromium.org changed reviewers: + grunell@chromium.org, tommi@chromium.org
PTAL
https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... content/renderer/media/audio_device_factory.cc:132: UMA_HISTOGRAM_BOOLEAN("Media.Audio.SinkCache.UsedForSinkCreation", false); Maybe add "Renderer" to be in line with other histogram names? E.g. "Media.Audio.Renderer.SinkCache.UsedForSinkCreation". https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_sink_cache_impl.cc (right): https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:109: UMA_HISTOGRAM_BOOLEAN("Media.Audio.SinkCache.GetOutputDeviceInfoHit", false); It would be nice to have the session ID case in its own bucket. https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:126: UMA_HISTOGRAM_BOOLEAN("Media.Audio.SinkCache.UsedForSinkCreation", true); False is never recorded? https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:140: UMA_HISTOGRAM_BOOLEAN("Media.Audio.SinkCache.InfoSinkReusedForOutput", I don't understand this histogram. Seems to record true when created and cached sink is used, then records false when sink is deleted. What's the purpose here? https://codereview.chromium.org/2124503002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2124503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21343: + Whether a cached sink created to get audio output device information was This is hard to understand. I guess it explains my question in the other file, but I'm still not sure.
https://codereview.chromium.org/2124503002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2124503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21343: + Whether a cached sink created to get audio output device information was On 2016/07/04 14:25:00, Henrik Grunell wrote: > This is hard to understand. I guess it explains my question in the other file, > but I'm still not sure. For example: "When audio output device information is requested, a sink is created and queried for the information, then cached. This metric shows if such a sink is later used for audio output, or cleared from the cache and deleted without being used." Actually, this is about sinks that were only used for getting device info in their lifetime vs. sinks that were used for audio (and maybe device info). Maybe word it and name the histogram something like that?
Rs lgtm
Patchset #3 (id:80001) has been deleted
Description was changed from ========== UMA stats for AudioRendererSinkCache utilization. BUG=586161 ========== to ========== UMA stats for AudioRendererSinkCache utilization. Follow-up after https://codereview.chromium.org/2120273004/ BUG=586161 ==========
PTAL: another version, now it depends on https://codereview.chromium.org/2120273004/ which introduces default device parameters caching. https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... content/renderer/media/audio_device_factory.cc:132: UMA_HISTOGRAM_BOOLEAN("Media.Audio.SinkCache.UsedForSinkCreation", false); On 2016/07/04 14:24:59, Henrik Grunell wrote: > Maybe add "Renderer" to be in line with other histogram names? E.g. > "Media.Audio.Renderer.SinkCache.UsedForSinkCreation". Done. https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... File content/renderer/media/audio_renderer_sink_cache_impl.cc (right): https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:109: UMA_HISTOGRAM_BOOLEAN("Media.Audio.SinkCache.GetOutputDeviceInfoHit", false); On 2016/07/04 14:24:59, Henrik Grunell wrote: > It would be nice to have the session ID case in its own bucket. Done. https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:126: UMA_HISTOGRAM_BOOLEAN("Media.Audio.SinkCache.UsedForSinkCreation", true); On 2016/07/04 14:24:59, Henrik Grunell wrote: > False is never recorded? Recorded in AudioDeviceFactory https://codereview.chromium.org/2124503002/diff/40001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:140: UMA_HISTOGRAM_BOOLEAN("Media.Audio.SinkCache.InfoSinkReusedForOutput", On 2016/07/04 14:24:59, Henrik Grunell wrote: > I don't understand this histogram. Seems to record true when created and cached > sink is used, then records false when sink is deleted. What's the purpose here? Discussed offline. https://codereview.chromium.org/2124503002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2124503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21343: + Whether a cached sink created to get audio output device information was On 2016/07/04 14:40:59, Henrik Grunell wrote: > On 2016/07/04 14:25:00, Henrik Grunell wrote: > > This is hard to understand. I guess it explains my question in the other file, > > but I'm still not sure. > > For example: "When audio output device information is requested, a sink is > created and queried for the information, then cached. This metric shows if such > a sink is later used for audio output, or cleared from the cache and deleted > without being used." > > Actually, this is about sinks that were only used for getting device info in > their lifetime vs. sinks that were used for audio (and maybe device info). Maybe > word it and name the histogram something like that? Done.
Please HOLD ON for now, the approach in https://codereview.chromium.org/2120273004/ has changed, so I need to adjust this one.
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:60001) has been deleted
PTAL now :)
olka@chromium.org changed reviewers: + guidou@chromium.org - grunell@chromium.org
Changing reviewer since grunell@ is OOO. gudou@ ptal
https://codereview.chromium.org/2124503002/diff/120001/content/renderer/media... File content/renderer/media/audio_renderer_sink_cache_impl.cc (right): https://codereview.chromium.org/2124503002/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_sink_cache_impl.cc:26: // No cached sink (no cached default device parameters for default sink) are default device parameters already being cached? https://codereview.chromium.org/2124503002/diff/120001/content/renderer/media... content/renderer/media/audio_renderer_sink_cache_impl.cc:28: SINK_CACHE_MISS = 0, The name suggests that this is a superset of SINK_CACHE_MISS_CANNOT_LOOKUP_BY_SESSION_ID. Maybe a better name is SINK_CACHE_MISS_EXPLICIT_DEVICE_ID or something that makes it clear that these misses are separate from On the other hand, it can also be argued that SINK_CACHE_MISS_CANNOT_LOOKUP_BY_SESSION_ID is not really measuring cache misses since cache hits are not possible when a session ID is passed. By following this logic, maybe the names could be SINK_CACHE_MISS, SINK_SESSION_ID, and SINK_CACHE_HIT.
PTAL now
lgtm
olka@chromium.org changed reviewers: + holte@chromium.org
holte@ - PTAL at histograms tommi@ - still lgtm?
still lgtm
Description was changed from ========== UMA stats for AudioRendererSinkCache utilization. Follow-up after https://codereview.chromium.org/2120273004/ BUG=586161 ========== to ========== UMA stats for AudioRendererSinkCache utilization. BUG=586161 ==========
olka@chromium.org changed reviewers: + jwd@chromium.org - holte@chromium.org
Changing histogram reviewer, since holte@ is OOO. jwd@ - PTAL at UMA.
https://codereview.chromium.org/2124503002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2124503002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21306: + new one was created for that. Can you make the last part of this sentence a bit clearer. Maybe just change "new one" to be "new sink" (assuming that's correct). https://codereview.chromium.org/2124503002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21316: + unused after timeout expires. Can you mention when this is logged. Is it logged each time it's used, only on first use, or only when it expires?
Thanks, PTAL at updated histograms. https://codereview.chromium.org/2124503002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2124503002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21306: + new one was created for that. On 2016/07/12 14:55:08, jwd wrote: > Can you make the last part of this sentence a bit clearer. Maybe just change > "new one" to be "new sink" (assuming that's correct). Done. https://codereview.chromium.org/2124503002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:21316: + unused after timeout expires. On 2016/07/12 14:55:08, jwd wrote: > Can you mention when this is logged. Is it logged each time it's used, only on > first use, or only when it expires? Done.
lgtm
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2124503002/#ps160001 (title: "addressing review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by olka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== UMA stats for AudioRendererSinkCache utilization. BUG=586161 ========== to ========== UMA stats for AudioRendererSinkCache utilization. BUG=586161 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== UMA stats for AudioRendererSinkCache utilization. BUG=586161 ========== to ========== UMA stats for AudioRendererSinkCache utilization. BUG=586161 Committed: https://crrev.com/7fa156327c1b65b647d8f1dac59902191dc56c10 Cr-Commit-Position: refs/heads/master@{#405085} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7fa156327c1b65b647d8f1dac59902191dc56c10 Cr-Commit-Position: refs/heads/master@{#405085} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
