Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1564923008:
Integrate helper macros for calling histograms with different names (real-time vs screenshare and r… (Closed)
|
Created:
4 years, 11 months ago by åsapersson Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhuangzesen_agora.io, henrika_webrtc, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, fengyue_agora.io, andresp, peah-webrtc, the sun, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIntegrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics).
Sparse macro is replaced and new implementation in metrics.h is used.
BUG=webrtc:5283
Committed: https://crrev.com/c2148a50d233b7dace8043da50fc20bfd2a5de84
Cr-Commit-Position: refs/heads/master@{#11483}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : rebase #Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 28 (11 generated)
Patchset #4 (id:60001) has been deleted
Description was changed from ========== Integrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics). BUG=webrtc:5283 ========== to ========== Integrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics). BUG=webrtc:5283 ==========
asapersson@webrtc.org changed reviewers: + pbos@webrtc.org, stefan@webrtc.org
Description was changed from ========== Integrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics). BUG=webrtc:5283 ========== to ========== Integrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics). Sparse macro is replaced and new implementation in metrics.h is used. BUG=webrtc:5283 ==========
https://codereview.webrtc.org/1564923008/diff/100001/webrtc/modules/bitrate_c... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1564923008/diff/100001/webrtc/modules/bitrate_c... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:158: RTC_HISTOGRAMS_COUNTS_100000(i, kUmaRampupMetrics[i].metric_name, It's not clear to me why these are now put on different indices. What does that mean? They already have different names?
lgtm https://codereview.webrtc.org/1564923008/diff/100001/webrtc/modules/bitrate_c... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1564923008/diff/100001/webrtc/modules/bitrate_c... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:158: RTC_HISTOGRAMS_COUNTS_100000(i, kUmaRampupMetrics[i].metric_name, On 2016/02/03 10:35:42, stefan-webrtc (holmer) wrote: > It's not clear to me why these are now put on different indices. What does that > mean? They already have different names? To implement this fast we can't do the lookup on name every time this is called, so we can cache it but that requires a fixed name per invocation. This macro expands to switch(i) case 0: HISTOGRAM(name) break; case 1: HISTOGRAM(name) break; Which permits us to have a single lookup per index instead of per call. 0 must always be the same metric name across the process, same for 1. It's a bit ugly, but it prevents us from having to look for the metric every time we modify those specific stats. https://codereview.webrtc.org/1564923008/diff/100001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1564923008/diff/100001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:106: RTC_HISTOGRAMS_COUNTS_10000(kIndex, uma_prefix_ + "InputWidthInPixels", Note that this will construct uma_prefix_ + "Input..." every time this is called, even when the map is cached. If we start calling this a lot this might matter.
lgtm
https://codereview.webrtc.org/1564923008/diff/100001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1564923008/diff/100001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:106: RTC_HISTOGRAMS_COUNTS_10000(kIndex, uma_prefix_ + "InputWidthInPixels", On 2016/02/03 10:43:41, pbos-webrtc wrote: > Note that this will construct uma_prefix_ + "Input..." every time this is > called, even when the map is cached. If we start calling this a lot this might > matter. Acknowledged.
On 2016/02/03 11:01:59, åsapersson wrote: > https://codereview.webrtc.org/1564923008/diff/100001/webrtc/video/send_statis... > File webrtc/video/send_statistics_proxy.cc (right): > > https://codereview.webrtc.org/1564923008/diff/100001/webrtc/video/send_statis... > webrtc/video/send_statistics_proxy.cc:106: RTC_HISTOGRAMS_COUNTS_10000(kIndex, > uma_prefix_ + "InputWidthInPixels", > On 2016/02/03 10:43:41, pbos-webrtc wrote: > > Note that this will construct uma_prefix_ + "Input..." every time this is > > called, even when the map is cached. If we start calling this a lot this might > > matter. > > Acknowledged. I think if we remove constant_name from webrtc::metrics::HistogramAdd(histogram_pointer, constant_name, sample); in the macro then we don't have to worry here because it would only be used in factory_get_invocation, so we'd never expand the string in the fast path.
On 2016/02/03 11:10:32, pbos-webrtc wrote: > On 2016/02/03 11:01:59, åsapersson wrote: > > > https://codereview.webrtc.org/1564923008/diff/100001/webrtc/video/send_statis... > > File webrtc/video/send_statistics_proxy.cc (right): > > > > > https://codereview.webrtc.org/1564923008/diff/100001/webrtc/video/send_statis... > > webrtc/video/send_statistics_proxy.cc:106: RTC_HISTOGRAMS_COUNTS_10000(kIndex, > > uma_prefix_ + "InputWidthInPixels", > > On 2016/02/03 10:43:41, pbos-webrtc wrote: > > > Note that this will construct uma_prefix_ + "Input..." every time this is > > > called, even when the map is cached. If we start calling this a lot this > might > > > matter. > > > > Acknowledged. > > I think if we remove constant_name from > webrtc::metrics::HistogramAdd(histogram_pointer, constant_name, sample); in the > macro then we don't have to worry here because it would only be used in > factory_get_invocation, so we'd never expand the string in the fast path. On the other hand, we definitely want the DCHECK on our tryservers that the name matches. We should be able to achive both.
https://codereview.webrtc.org/1564923008/diff/120001/webrtc/system_wrappers/i... File webrtc/system_wrappers/include/metrics.h (right): https://codereview.webrtc.org/1564923008/diff/120001/webrtc/system_wrappers/i... webrtc/system_wrappers/include/metrics.h:202: RTC_NOTREACHED(); \ Maybe remove the DCHECK above and do RTC_NOTREACHED() << "Index " << index << "is out of bounds.";
On 2016/02/03 11:56:56, pbos-webrtc wrote: > https://codereview.webrtc.org/1564923008/diff/120001/webrtc/system_wrappers/i... > File webrtc/system_wrappers/include/metrics.h (right): > > https://codereview.webrtc.org/1564923008/diff/120001/webrtc/system_wrappers/i... > webrtc/system_wrappers/include/metrics.h:202: RTC_NOTREACHED(); \ > Maybe remove the DCHECK above and do RTC_NOTREACHED() << "Index " << index << > "is out of bounds."; " is..
https://codereview.webrtc.org/1564923008/diff/120001/webrtc/system_wrappers/i... File webrtc/system_wrappers/include/metrics.h (right): https://codereview.webrtc.org/1564923008/diff/120001/webrtc/system_wrappers/i... webrtc/system_wrappers/include/metrics.h:202: RTC_NOTREACHED(); \ On 2016/02/03 11:56:56, pbos-webrtc wrote: > Maybe remove the DCHECK above and do RTC_NOTREACHED() << "Index " << index << > "is out of bounds."; Removed the DCHECK.
lgtm
asapersson@webrtc.org changed reviewers: + mflodman@webrtc.org
lgtm
The CQ bit was checked by asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1564923008/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1564923008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1564923008/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by asapersson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1564923008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1564923008/160001
Message was sent while issue was closed.
Description was changed from ========== Integrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics). Sparse macro is replaced and new implementation in metrics.h is used. BUG=webrtc:5283 ========== to ========== Integrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics). Sparse macro is replaced and new implementation in metrics.h is used. BUG=webrtc:5283 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Integrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics). Sparse macro is replaced and new implementation in metrics.h is used. BUG=webrtc:5283 ========== to ========== Integrate helper macros for calling histograms with different names (real-time vs screenshare and rampup metrics). Sparse macro is replaced and new implementation in metrics.h is used. BUG=webrtc:5283 Committed: https://crrev.com/c2148a50d233b7dace8043da50fc20bfd2a5de84 Cr-Commit-Position: refs/heads/master@{#11483} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c2148a50d233b7dace8043da50fc20bfd2a5de84 Cr-Commit-Position: refs/heads/master@{#11483} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
