Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121223002:
AudioOutputDevice authorization timeout UMA histograms (Closed)
|
Created:
4 years, 5 months ago by o1ka Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, Steven Holte Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAudioOutputDevice authorization timeout UMA histograms
BUG=615589
Committed: https://crrev.com/f3e5a2d9967dbec76d3f6d6c8241d15ce6ee1ab7
Cr-Commit-Position: refs/heads/master@{#405092}
Patch Set 1 #Patch Set 2 : timeout set to 2000, UMA stat added. #
Total comments: 1
Patch Set 3 : Moving authorization statistics to browser #
Total comments: 6
Patch Set 4 : 50 buckets #
Messages
Total messages: 40 (14 generated)
Description was changed from ========== AudioOUtputDevice authorization timeout incerease. BUG=615589 ========== to ========== AudioOUtputDevice authorization timeout incerease. BUG=615589 ==========
olka@chromium.org changed reviewers: + dalecurtis@chromium.org
As stated via e-mail I think this is too high. We should ratchet up from 1 second or add a UMA to track the actual timeout values so we can capture the greatest number.
Patchset #2 (id:20001) has been deleted
olka@chromium.org changed reviewers: + holte@chromium.org
PTAL: Timeout set to 2000 + UMA stat added. holte@: PTAL at histogram.
https://codereview.chromium.org/2121223002/diff/40001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2121223002/diff/40001/media/audio/audio_outpu... media/audio/audio_output_device.cc:330: UMA_HISTOGRAM_CUSTOM_TIMES("Media.Audio.Render.OutputDeviceAuthorizationTime", This doesn't allow us to make adjustments without binary searching across the domain. Ideally you want ignore the timed out status when reporting here. And for timeouts report when the real status actually comes back (and is short circuited by IPC_CLOSED above).
The real status won't come back, because IPC is closed after timeout (that's why IPC_CLOSED) . Remember we discussed that when timeouts were introduced? On Jul 7, 2016 7:39 PM, <dalecurtis@chromium.org> wrote: > > > https://codereview.chromium.org/2121223002/diff/40001/media/audio/audio_outpu... > File media/audio/audio_output_device.cc (right): > > > https://codereview.chromium.org/2121223002/diff/40001/media/audio/audio_outpu... > media/audio/audio_output_device.cc:330: > > UMA_HISTOGRAM_CUSTOM_TIMES("Media.Audio.Render.OutputDeviceAuthorizationTime", > This doesn't allow us to make adjustments without binary searching > across the domain. Ideally you want ignore the timed out status when > reporting here. And for timeouts report when the real status actually > comes back (and is short circuited by IPC_CLOSED above). > > https://codereview.chromium.org/2121223002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hmm, instead of this UMA, perhaps instead we want one in AudioMessageFilter which is a histogram of OnDeviceAuthorized times then.
On 2016/07/07 21:06:02, DaleCurtis wrote: > Hmm, instead of this UMA, perhaps instead we want one in AudioMessageFilter > which is a histogram of OnDeviceAuthorized times then. As soon as authorization times out, we close IPC: https://cs.chromium.org/chromium/src/media/audio/audio_output_device.cc?cl=GR... Which on browser side results in https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi... Renderer host won't send us authorization reply if it receives "Close" before authorization is processed: https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi... => it means we still do not record actual authorization times for all new devices (we rather miss many of them from the picture). And implementing this logic in AudioMessageFilter is pretty heavy: we need to have a map for stream id <-> authorization timer, and we need to clean up the map entries in case we never receive authorization (see above). To me it looks like setting timeout to some large value (2000 or 4000) and adjusting it with a little of binary search is way more easy, effective and safe. (We can even drop timeout for a wile and get the whole distribution)
Thanks for pointing that out, I didn't realize we aborted it browser side too when closing. How about putting it in AudioRendererHost then? It'll miss IPC transmit delay, but that should be very small.
On 2016/07/08 17:35:54, DaleCurtis wrote: > Thanks for pointing that out, I didn't realize we aborted it browser side too > when closing. How about putting it in AudioRendererHost then? It'll miss IPC > transmit delay, but that should be very small. It will be a map stream id <-> ElapsedTimer (accessed on IO thread only). Will we allow to never delete entries if authorization times out? Or can you suggest a way to clean those up?
On 2016/07/08 17:47:30, o1ka wrote: > On 2016/07/08 17:35:54, DaleCurtis wrote: > > Thanks for pointing that out, I didn't realize we aborted it browser side too > > when closing. How about putting it in AudioRendererHost then? It'll miss IPC > > transmit delay, but that should be very small. > > It will be a map stream id <-> ElapsedTimer (accessed on IO thread only). > Will we allow to never delete entries if authorization times out? Or can you > suggest a way to clean those up? As we discussed offline, request time can be bound to OnDeviceAuthorized here https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...
Description was changed from ========== AudioOUtputDevice authorization timeout incerease. BUG=615589 ========== to ========== AudioOutputDevice authorization timeout UMA histograms BUG=615589 ==========
PTAL. UMA is logged explicitly before Send() everywhere, so that if there is any new heavy processing logic is added in the future, its time is taken into account. However, this does not protect from the situation when new Send() paths are added, but not logged. Probably we should log the number of authorization requests to make sure it matches total authorization time record count? https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:420: UMALogDeviceAuthorizationTime(auth_start_time); This is ~zero, but I think we need to log all the cases to have proper %% https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:445: UMALogDeviceAuthorizationTime(auth_start_time); Same here https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:473: UMALogDeviceAuthorizationTime(auth_start_time); Note that here we interrupt authorization processing if authorization timed out, so we log only CheckOutputDeviceAccess() execution time.
lgtm
olka@chromium.org changed reviewers: + asvitkine@chromium.org - holte@chromium.org
Changing histogram reviewer since holte@ seems to be OOO. asvitkine@ - PTAL
https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:116: base::TimeDelta::FromMilliseconds(5000), 100); Do you really need 100 buckets? We recommend going with 50 buckets unless the granularity proves not useful.
PTAL: changed histogram to 50 buckets (100 ms precision) https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:116: base::TimeDelta::FromMilliseconds(5000), 100); On 2016/07/11 21:22:55, Alexei Svitkine wrote: > Do you really need 100 buckets? We recommend going with 50 buckets unless the > granularity proves not useful. Do I look like a man without a plan? :) Ok, I think I can survive with 50 (meaning 100 ms precision) to speed up the review. What are the reasons of 50 being a recommended value?
lgtm https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:116: base::TimeDelta::FromMilliseconds(5000), 100); On 2016/07/12 13:30:50, o1ka wrote: > On 2016/07/11 21:22:55, Alexei Svitkine wrote: > > Do you really need 100 buckets? We recommend going with 50 buckets unless the > > granularity proves not useful. > > Do I look like a man without a plan? :) Ok, I think I can survive with 50 > (meaning 100 ms precision) to speed up the review. > What are the reasons of 50 being a recommended value? Using 100 buckets makes the histogram take more resources (bandwidth/memory/backend resources). Among other things, it makes it so there's less aggregation client side (since there's likely more unique buckets reported by a client). Also note that the range is not evenly split, but uses a logarithmic algorithm with buckets having more precise range near the min value and wider range near the max value.
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2121223002/#ps80001 (title: "50 buckets")
On 2016/07/12 14:52:52, Alexei Svitkine wrote: > lgtm > > https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... > File content/browser/renderer_host/media/audio_renderer_host.cc (right): > > https://codereview.chromium.org/2121223002/diff/60001/content/browser/rendere... > content/browser/renderer_host/media/audio_renderer_host.cc:116: > base::TimeDelta::FromMilliseconds(5000), 100); > On 2016/07/12 13:30:50, o1ka wrote: > > On 2016/07/11 21:22:55, Alexei Svitkine wrote: > > > Do you really need 100 buckets? We recommend going with 50 buckets unless > the > > > granularity proves not useful. > > > > Do I look like a man without a plan? :) Ok, I think I can survive with 50 > > (meaning 100 ms precision) to speed up the review. > > What are the reasons of 50 being a recommended value? > > Using 100 buckets makes the histogram take more resources > (bandwidth/memory/backend resources). Among other things, it makes it so there's > less aggregation client side (since there's likely more unique buckets reported > by a client). > > Also note that the range is not evenly split, but uses a logarithmic algorithm > with buckets having more precise range near the min value and wider range near > the max value. Thanks for clarification!
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_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 ========== AudioOutputDevice authorization timeout UMA histograms BUG=615589 ========== to ========== AudioOutputDevice authorization timeout UMA histograms BUG=615589 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== AudioOutputDevice authorization timeout UMA histograms BUG=615589 ========== to ========== AudioOutputDevice authorization timeout UMA histograms BUG=615589 Committed: https://crrev.com/f3e5a2d9967dbec76d3f6d6c8241d15ce6ee1ab7 Cr-Commit-Position: refs/heads/master@{#405092} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f3e5a2d9967dbec76d3f6d6c8241d15ce6ee1ab7 Cr-Commit-Position: refs/heads/master@{#405092} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
