Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
693433003:
Add a new finch experiment to control the system buffer size. (Closed)
|
Created:
6 years, 1 month ago by guoweis_left_chromium Modified:
6 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd a new finch experiment to control the system buffer size.
Control UDP Send Socket buffer size along with a new UMA to track how long the time spent in browser process.
BUG=427555
Committed: https://crrev.com/2996830edd11a448f906df4353242b64358725da
Cr-Commit-Position: refs/heads/master@{#303326}
Patch Set 1 : #Patch Set 2 : change from uL to uLL for 64 bit number #
Total comments: 17
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 29 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
guoweis@chromium.org changed reviewers: + juberti@chromium.org, mallinath@chromium.org
Justin, please review this change. mallinath@, please OWNER review files under p2p.
guoweis@chromium.org changed reviewers: + asvitkine@chromium.org, isherman@chromium.org, jar@chromium.org, jwd@chromium.org, mpearson@chromium.org
asvitkine@, isherman@, jar@, jwd@, mpearson@ Sorry for the spamming. I'm trying to get this change in by tonight. Any chance that I could get anyone of you to OWNER review histograms.xml?
lgtm
lgtm
On 2014/11/04 05:32:18, mallinath2 wrote:
> lgtm
Here is the function which generates the packet_id in
content/renderer/p2p/socket_client_impl.cc
uint64 GetUniqueId(uint32 random_socket_id, uint32 packet_id) {
uint64 uid = random_socket_id;
uid <<= 32;
uid |= packet_id;
return uid;
}
lgtm with nits, although OK to address them later https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:92: if (base::FieldTrialList::FindFullName("WebRTC-SystemUDPSendSocketSize") == Seems like we should read FindFullName into a string and then compare that string against the various values, instead of duping the FindFullName call each time. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:103: if (send_buffer_size > 0) { Probably want to invalidate the experiment if this happens, i.e. not record a result https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:285: uint64 call_record = For the future, I think it would be better to allocate a structure rather than peer into the internals of the packet id. (Yes, this was my suggestion, but I didn't realize the assumptions the code would end up making.)
On 2014/11/04 05:28:13, guoweis_chromium wrote: > asvitkine@, isherman@, jar@, jwd@, mpearson@ > > Sorry for the spamming. I'm trying to get this change in by tonight. > > Any chance that I could get anyone of you to OWNER review histograms.xml? asvitkine@, isherman@, jar@, jwd@, mpearson@, if anyone of you could lgtm for me, could you also please click on Commit on my behalf? Just in case if I'm not online.
https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:92: if (base::FieldTrialList::FindFullName("WebRTC-SystemUDPSendSocketSize") == On 2014/11/04 06:00:18, juberti2 wrote: > Seems like we should read FindFullName into a string and then compare that > string against the various values, instead of duping the FindFullName call each > time. Agreed. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:96: "WebRTC-SystemUDPSendSocketSize") == "64K") { I would normally suggest to use variation params here (go/finch-params), but perhaps that API isn't exposed to the code in content/, in which I guess using the experiment name is OK. However, my suggestion would be to check StartsWith(name, "64K"). That way, you can have multiple groups that specify the same behavior, which gives you more flexibility in the config. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:100: send_buffer_size = 262144; Nit: It would be more readable to have these be 256 * 1024; https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:365: 100 /* bucket_count */); I suggest just using UMA_HISTOGRAM_TIMES() here, with the only difference is that version has a 10s max instead of 1s. Since the histogram is exponential, you still get a lot of resolution in the lower buckets, see for e.g.: https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=11-03-2014... https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_udp.h (right): https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.h:51: static uint64 PackCallRecord(const uint64 packet_id, const uint64 ticks_now); Nit: Usually primitive params aren't marked const. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.h:108: uint64 random_socket_id_prefix_; Nit: I think for new code, uint64_t is preferred (i.e. types with _t suffix).
Since Alexei is taking the lead from the metrics side for reviewing this, I'm removing all the other metrics folks as reviewers.
mpearson@chromium.org changed reviewers: - isherman@chromium.org, jar@chromium.org, jwd@chromium.org, mpearson@chromium.org
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
guoweis@google.com changed reviewers: + guoweis@google.com
PTAL https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:92: if (base::FieldTrialList::FindFullName("WebRTC-SystemUDPSendSocketSize") == On 2014/11/04 15:17:50, Alexei Svitkine wrote: > On 2014/11/04 06:00:18, juberti2 wrote: > > Seems like we should read FindFullName into a string and then compare that > > string against the various values, instead of duping the FindFullName call > each > > time. > > Agreed. Done. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:96: "WebRTC-SystemUDPSendSocketSize") == "64K") { On 2014/11/04 15:17:50, Alexei Svitkine wrote: > I would normally suggest to use variation params here (go/finch-params), but > perhaps that API isn't exposed to the code in content/, in which I guess using > the experiment name is OK. > > However, my suggestion would be to check StartsWith(name, "64K"). That way, you > can have multiple groups that specify the same behavior, which gives you more > flexibility in the config. Variation is not allowed so following the pattern https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_cache..., I'm parsing the group name and get the buffer size. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:100: send_buffer_size = 262144; On 2014/11/04 15:17:50, Alexei Svitkine wrote: > Nit: It would be more readable to have these be 256 * 1024; Done. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:103: if (send_buffer_size > 0) { On 2014/11/04 06:00:18, juberti2 wrote: > Probably want to invalidate the experiment if this happens, i.e. not record a > result Alexei, is there a way to invalidate this instance of experiment? looking at field_trial.h but can't find one. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:285: uint64 call_record = On 2014/11/04 06:00:18, juberti2 wrote: > For the future, I think it would be better to allocate a structure rather than > peer into the internals of the packet id. (Yes, this was my suggestion, but I > didn't realize the assumptions the code would end up making.) Changed to allocate memory. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.cc:365: 100 /* bucket_count */); On 2014/11/04 15:17:50, Alexei Svitkine wrote: > I suggest just using UMA_HISTOGRAM_TIMES() here, with the only difference is > that version has a 10s max instead of 1s. > > Since the histogram is exponential, you still get a lot of resolution in the > lower buckets, see for e.g.: > > https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=11-03-2014... Done. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... File content/browser/renderer_host/p2p/socket_host_udp.h (right): https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.h:51: static uint64 PackCallRecord(const uint64 packet_id, const uint64 ticks_now); On 2014/11/04 15:17:50, Alexei Svitkine wrote: > Nit: Usually primitive params aren't marked const. Code removed. https://codereview.chromium.org/693433003/diff/80001/content/browser/renderer... content/browser/renderer_host/p2p/socket_host_udp.h:108: uint64 random_socket_id_prefix_; On 2014/11/04 15:17:50, Alexei Svitkine wrote: > Nit: I think for new code, uint64_t is preferred (i.e. types with _t suffix). Code removed.
https://codereview.chromium.org/693433003/diff/140001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/693433003/diff/140001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:105: &send_buffer_size) && I don't think checking the return value of this function is correct - because I think it will return false when there's trailing characters. Instead, move it above the if and just check send_buffer_size > 0. https://codereview.chromium.org/693433003/diff/140001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:288: &P2PSocketHostUdp::OnSend, base::Unretained(this), call_record)); I think you should be able to bind to the params directly without needing call_record, i.e. base::Bind(&P2PSocketHostUdp::OnSend, base::Unretained(this), packet.id, base::TimeTicks::Now().ToInternalValue()) should work, if OnSend is modified to take those params.
lgtm % nits https://codereview.chromium.org/693433003/diff/140001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/693433003/diff/140001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:99: unsigned send_buffer_size = 0; unsigned int https://codereview.chromium.org/693433003/diff/140001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:101: // In the finch experiment, groups will be named like "2K", "64K", OOC, would it make more sense to just name the groups "2048", "65535", etc?
Patchset #4 (id:160001) has been deleted
Couple more nits in addition to my previous comment about the parsing code. https://codereview.chromium.org/693433003/diff/180001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (left): https://codereview.chromium.org/693433003/diff/180001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:47: Nit: Don't remove an empty line here. https://codereview.chromium.org/693433003/diff/180001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host_udp.cc (right): https://codereview.chromium.org/693433003/diff/180001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host_udp.cc:306: Nit: Looks like you add an extra new line here. Please revert back.
LGTM % my nits from earlier comment, thanks!
The CQ bit was checked by guoweis@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/693433003/220001
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2996830edd11a448f906df4353242b64358725da Cr-Commit-Position: refs/heads/master@{#303326} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
