New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 457078



Sign in to add a comment

WebRTC histograms appear create too many static locals

Project Member Reported by r...@chromium.org, Jan 13 2017

Issue description

Bruce originally discovered this in https://bugs.chromium.org/p/chromium/issues/detail?id=457078#c79:

"There are also claims that the clang-cl binaries have many more duplicate globals, such as 4,550 copies of atomic_histogram_pointer. I'm not sure if these reports are real are spurious - I think VC++ sometimes makes these symbols invisible even to the PDB."

Codesearch shows that the name 'atomic_histogram_pointer' comes from skia and webrtc histograms. There are very few skia histograms, so I think these are from webrtc. Looking at the webrtc metrics header shows that WEBRTC_HISTOGRAM_COMMON_BLOCK is instantiated many times:
https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/include/metrics.h?q=RTC_HISTOGRAM_COMMON_BLOCK&sq=package:chromium&l=115&dr=C

I noticed that send_side_bandwidth_estimation.cc uses exactly one histogram, so I compiled that object file and looked at the symbols it produces. There are six copies of atomic_histogram_pointer in the resulting object file. Both clang and MSVC have six copies of this global.

Is this intended? Can this be made more efficient?
 

Comment 1 by thakis@chromium.org, Jan 13 2017

Cc: mflodman@chromium.org pbos@chromium.org asapersson@chromium.org
(webrtc folks working on metrics.h: see question at end of comment 0)
This is the second indication I have seen recently of VC++ producing global variables that don't show up in the PDB. This means that we are missing opportunities to improve the size of our binaries due to missing information.

If you come up with a theory about why these symbols are not showing up in the VC++ PDB please let me know - I'd love to improve the visibility. Another example is kCTLogList. This was moved to the read-only data segment in crrev.com/2613723005 but it never showed up in the reports of globals so this was more about luck than science.

Comment 3 by guidou@chromium.org, Jan 23 2017

Owner: pbos@chromium.org
Status: Assigned (was: Untriaged)
pbos@: are you familiar with this issue? if not, feel free to triage further or bounce back.

Comment 4 by pbos@chromium.org, Jan 23 2017

Cc: tommi@chromium.org
send_side_bandwidth_estimation.cc uses multiple histograms:

WebRTC.BWE.RampUpTimeTo500kbpsInMs
WebRTC.BWE.RampUpTimeTo100kbpsInMs
WebRTC.BWE.RampUpTimeTo2000kbpsInMs
WebRTC.BWE.InitiallyLostPackets
WebRTC.BWE.InitialRtt
WebRTC.BWE.InitialBandwidthEstimate
WebRTC.BWE.InitialVsConvergedDiff

I can see 5 RTC_HISTOGRAM_* calls in the file, so 5 atomic_histogram_pointer instances are expected at HEAD.

From what I can tell I think third_party/webrtc should overall be responsible for 153 instances of this pointer at most.

pbos@pbos7:~/webrtc/src (master)$ git grep RTC_HISTOGRAM | grep -v webrtc/system_wrappers/include/metrics.h | grep -v unittest.cc: | wc -l
153

Most of these are called from classes, so we could replace most of them with construction-time getters of webrtc::metrics::Histogram and then replace:

RTC_HISTOGRAM_COUNTS("WebRTC.BWE.InitiallyLostPackets", initially_lost_packets_, 0, 100, 50);

With:

webrtc::metrics::HistogramAdd(initially_lost_packets_histogram_, initially_lost_packets_);

This is less readable than versions with the static string, but removes the function-static pointer instance.

153 is only a small fraction of 4550 though, so I'm not sure if it's fair to say that WebRTC histograms create too many static locals. I can help out with this conversion work if it's necessary/wanted. It sounds like the code removal from removing RTC_HISTOGRAM_COUNTS code expansion is significantly larger the static locals it would remove. We could probably shave off a couple of kilobytes here and remove some atomic usage which reduces contention.

tommi@: Can you confirm that this is something we want to fix? I can chip in to help remove these instances in ~less than a day, but I'd want a nod that it's OK to do so. I'll do an example CL for send_side_bandwidth_estimation.cc.

Comment 5 by pbos@chromium.org, Jan 23 2017

Suggested alternative CL: https://codereview.webrtc.org/2654473002

Comment 6 by pbos@chromium.org, Jan 23 2017

I think any real size gains are in removing instances of this code expansion rather than the static local pointer, since the code generated for this should dwarf 8 bytes.

https://chromium.googlesource.com/external/webrtc/+/f49ff260d1843975f134416d694bd77d829e4201/webrtc/system_wrappers/include/metrics.h#123


Comment 7 by r...@chromium.org, Feb 1 2017

Owner: r...@chromium.org
Assigning to me to eventually do some PDB investigation.
Note that some of the tools referenced from here:

https://www.chromium.org/developers/windows-binary-sizes

will dump global variables listed in a PDB in various forms. The tools may be helpful in evaluating the impact. Although, there are some indications that some of the variables do not show up in the PDBs - I don't know why.
rnk@: Is this still valid?
Owner: ----
Status: Available (was: Assigned)
Looking at the code, I still think there is room to improve the code size of this pattern, but it's probably not the most efficient way to drive down Chrome's code size.

I will unassign, though, since I am not investigating this.

Is there a bug component we can file this for tracking available size optimization opportunities?
Labels: Performance-Size
We have a label.

Sign in to add a comment