Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2092563002:
Set up field trials for SSL read/write buffer sizes. (Closed)
DescriptionSet up field trials for SSL read/write buffer sizes.
These buffers can add up pretty quickly on memory constrained
Android devices.
Also add a histogram for performance of large uploads. We don't
seem to have any metric aimed at upload performance, and want to
protect ourselves against a significant regression that only affects
upload performance.
BUG=524258
Committed: https://crrev.com/1beda3d241618e0a114cd1a238f453de807948f5
Cr-Commit-Position: refs/heads/master@{#407028}
Patch Set 1 #
Total comments: 2
Patch Set 2 : K -> k #Patch Set 3 : Fix typo #
Total comments: 8
Patch Set 4 : Update comment #Patch Set 5 : Merge #Patch Set 6 : Fix NACL, merge #
Messages
Total messages: 51 (25 generated)
mmenke@chromium.org changed reviewers: + davidben@chromium.org
It looks to me like we do put field trial code directly in net/, or at least we have done so a number of times. Could put this in IOThread or ProfileIOData, and then pass it to the network session instead. Given that I don't want this to stick around for that long, and thats it not cover all NetworkSessions, seems like this approach is preferable.
lgtm https://codereview.chromium.org/2092563002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2092563002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_impl.cc:939: send_buffer_size = KDefaultOpenSSLBufferSize; (Haha. While you're here, do you mind fixing KDefaultOpenSSLBufferSize to be kDefaultOpenSSLBufferSize?)
mmenke@chromium.org changed reviewers: + jwd@chromium.org
[+jwd]: Please review histograms.xml, thanks! https://codereview.chromium.org/2092563002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2092563002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_impl.cc:939: send_buffer_size = KDefaultOpenSSLBufferSize; On 2016/06/27 18:31:48, davidben wrote: > (Haha. While you're here, do you mind fixing KDefaultOpenSSLBufferSize to be > kDefaultOpenSSLBufferSize?) I completely missed that! Done.
https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:938: &send_buffer_size)) { I'd prefer you use variation params instead of the group name for specifying the buffer size. https://codereview.chromium.org/2092563002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2092563002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27279: + processed, for requests with an upload that is >= 1 MiB. Excludes chunked Nit(/do I know how numbers work?): here it says >=, but the code compares size() > 1024 * 1024. This is different, right?
Thanks for the feedback, jwd! https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:938: &send_buffer_size)) { On 2016/06/28 15:35:12, jwd wrote: > I'd prefer you use variation params instead of the group name for specifying the > buffer size. Unfortunately, net/ can depend on components/. https://codereview.chromium.org/2092563002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2092563002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27279: + processed, for requests with an upload that is >= 1 MiB. Excludes chunked On 2016/06/28 15:35:12, jwd wrote: > Nit(/do I know how numbers work?): here it says >=, but the code compares size() > > 1024 * 1024. This is different, right? Heh. I pointed out that exact same issue on someone else's CL earlier today. :)
https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:938: &send_buffer_size)) { On 2016/06/28 18:47:21, mmenke wrote: > On 2016/06/28 15:35:12, jwd wrote: > > I'd prefer you use variation params instead of the group name for specifying > the > > buffer size. > > Unfortunately, net/ can depend on components/. can't, rather
lgtm https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:938: &send_buffer_size)) { On 2016/06/28 18:47:59, mmenke wrote: > On 2016/06/28 18:47:21, mmenke wrote: > > On 2016/06/28 15:35:12, jwd wrote: > > > I'd prefer you use variation params instead of the group name for specifying > > the > > > buffer size. > > > > Unfortunately, net/ can depend on components/. > > can't, rather Ah, ok, so in that case, I'm fine how it is. StringToInt should give you enough flexibility to rename groups and still have them parse correctly. You'll obviously just need to take some care if you ever do need to rename groups. (group renaming can be useful if you want have a new phase of experimentation and you don't want it to mix the data with the old.) If you ARE interested in using params, you can add to chrome_network_delegate.h to get access to that data.
https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:938: &send_buffer_size)) { On 2016/06/28 19:05:16, jwd wrote: > On 2016/06/28 18:47:59, mmenke wrote: > > On 2016/06/28 18:47:21, mmenke wrote: > > > On 2016/06/28 15:35:12, jwd wrote: > > > > I'd prefer you use variation params instead of the group name for > specifying > > > the > > > > buffer size. > > > > > > Unfortunately, net/ can depend on components/. > > > > can't, rather > > Ah, ok, so in that case, I'm fine how it is. StringToInt should give you enough > flexibility to rename groups and still have them parse correctly. You'll > obviously just need to take some care if you ever do need to rename groups. > (group renaming can be useful if you want have a new phase of experimentation > and you don't want it to mix the data with the old.) > > If you ARE interested in using params, you can add to chrome_network_delegate.h > to get access to that data. Unfortunately, this deep in the network stack, we don't have access to that, either. Client socket pools can actually be shared between URLRequestContexts with different NetworkDelegates, too, which can get very troublesome (And is really a bad design). Thanks for the suggestions, though. I suppose the exact size doesn't really matter, so we could just hack around that by using off-by-one values, though that seems weird. StringToInt may even be able to handle suffixes... i.e. "4096 Trial 2", though I'm not positive of that.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2092563002/#ps60001 (title: "Update comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2092563002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:938: &send_buffer_size)) { On 2016/06/28 19:18:23, mmenke wrote: > On 2016/06/28 19:05:16, jwd wrote: > > On 2016/06/28 18:47:59, mmenke wrote: > > > On 2016/06/28 18:47:21, mmenke wrote: > > > > On 2016/06/28 15:35:12, jwd wrote: > > > > > I'd prefer you use variation params instead of the group name for > > specifying > > > > the > > > > > buffer size. > > > > > > > > Unfortunately, net/ can depend on components/. > > > > > > can't, rather > > > > Ah, ok, so in that case, I'm fine how it is. StringToInt should give you > enough > > flexibility to rename groups and still have them parse correctly. You'll > > obviously just need to take some care if you ever do need to rename groups. > > (group renaming can be useful if you want have a new phase of experimentation > > and you don't want it to mix the data with the old.) > > > > If you ARE interested in using params, you can add to > chrome_network_delegate.h > > to get access to that data. > > Unfortunately, this deep in the network stack, we don't have access to that, > either. Client socket pools can actually be shared between URLRequestContexts > with different NetworkDelegates, too, which can get very troublesome (And is > really a bad design). Ah, none of my suggestions work! Oh well... > > Thanks for the suggestions, though. I suppose the exact size doesn't really > matter, so we could just hack around that by using off-by-one values, though > that seems weird. > > StringToInt may even be able to handle suffixes... i.e. "4096 Trial 2", though > I'm not positive of that. Yeah, suffixes is what I was thinking. The comments for StringToInt lead me to believe this would work.
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 mmenke@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_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 mmenke@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_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 mmenke@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_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 mmenke@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_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 mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2092563002/#ps80001 (title: "Merge")
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 mmenke@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2092563002/#ps100001 (title: "Fix NACL, merge")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mmenke@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.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Set up field trials for SSL read/write buffer sizes. These buffers can add up pretty quickly on memory constrained Android devices. Also add a histogram for performance of large uploads. We don't seem to have any metric aimed at upload performance, and want to protect ourselves against a significant regression that only affects upload performance. BUG=524258 ========== to ========== Set up field trials for SSL read/write buffer sizes. These buffers can add up pretty quickly on memory constrained Android devices. Also add a histogram for performance of large uploads. We don't seem to have any metric aimed at upload performance, and want to protect ourselves against a significant regression that only affects upload performance. BUG=524258 Committed: https://crrev.com/1beda3d241618e0a114cd1a238f453de807948f5 Cr-Commit-Position: refs/heads/master@{#407028} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1beda3d241618e0a114cd1a238f453de807948f5 Cr-Commit-Position: refs/heads/master@{#407028} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
