Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2824313002:
Split JobController count histograms into three separate histograms: (Closed)
|
Created:
3 years, 8 months ago by Zhongyi Shi Modified:
3 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, net-reviews_chromium.org, Ryan Hamilton Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit JobController count histograms into three separate histograms:
1. JobController for preconnect,
2. JobController for normal connect, has pending Request,
3. JobController for normal connect, has no Request.
The third one will give us better estimation of uncleaned JobControllers
and be used to detect memory leaks in JobController layer.
BUG=704988
Review-Url: https://codereview.chromium.org/2824313002
Cr-Commit-Position: refs/heads/master@{#465808}
Committed: https://chromium.googlesource.com/chromium/src/+/6c16897074391089d36b098902192f6845a11aa5
Patch Set 1 #
Total comments: 10
Patch Set 2 : address nits #
Total comments: 2
Patch Set 3 : remove redundant DCHECKs #
Messages
Total messages: 23 (9 generated)
zhongyi@chromium.org changed reviewers: + rch@chromium.org, xunjieli@chromium.org
Description was changed from ========== Split JobController count histograms into three separate histograms: 1. JobController for preconnect, 2. JobController for normal connect, has pending Request, 3. JobController for normal connect, has no Request. The third one will give us better estimation of uncleaned JobControllers and be used to detect memory leaks in/below JobController. BUG=704988 ========== to ========== Split JobController count histograms into three separate histograms: 1. JobController for preconnect, 2. JobController for normal connect, has pending Request, 3. JobController for normal connect, has no Request. The third one will give us better estimation of uncleaned JobControllers and be used to detect memory leaks in JobController layer. BUG=704988 ==========
Nice! https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:375: size_t pending_request_count = 0; nit: s/pending_request_count/num_controllers_with_request https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:376: size_t preconnect_controller_count = 0; nit: s/preconnect_controller_count/num_controllers_for_preconnect https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:401: "Net.JobControllerSet.CountOfJobCOntroller.NonPerconnect.PendingRequest", s/COntroller/Controller s/Perconnect/Preconnect https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:405: pending_request_count); nit: can we calculate this value and DCHECK on it? DCHECK_GE(num_controllers_with_no_request, 0); UMA_HISTOGRAM...(... num_controllers_with_no_request); https://codereview.chromium.org/2824313002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2824313002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:34993: -<histogram name="Net.JobControllerSet.CountOfPendingRequest" units="requests"> I think the recommended way is to deprecate the old histogram and not remove it.
Thanks for helping the review. xunjieli@: Helen, PTAL. https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:375: size_t pending_request_count = 0; On 2017/04/19 15:12:13, xunjieli wrote: > nit: s/pending_request_count/num_controllers_with_request Done. https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:376: size_t preconnect_controller_count = 0; On 2017/04/19 15:12:13, xunjieli wrote: > nit: s/preconnect_controller_count/num_controllers_for_preconnect Done. https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:401: "Net.JobControllerSet.CountOfJobCOntroller.NonPerconnect.PendingRequest", On 2017/04/19 15:12:14, xunjieli wrote: > s/COntroller/Controller > s/Perconnect/Preconnect Done. https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:405: pending_request_count); On 2017/04/19 15:12:13, xunjieli wrote: > nit: can we calculate this value and DCHECK on it? > DCHECK_GE(num_controllers_with_no_request, 0); DCHECK_GE(job_controller_set_.size() - preconnect_controller_count, pending_request_count) does the DCHECK and prevents overflow. > UMA_HISTOGRAM...(... num_controllers_with_no_request);
https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2824313002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:405: pending_request_count); On 2017/04/19 17:38:53, Zhongyi Shi wrote: > On 2017/04/19 15:12:13, xunjieli wrote: > > nit: can we calculate this value and DCHECK on it? > > DCHECK_GE(num_controllers_with_no_request, 0); > > DCHECK_GE(job_controller_set_.size() - preconnect_controller_count, > pending_request_count) does the DCHECK and prevents overflow. > > > UMA_HISTOGRAM...(... num_controllers_with_no_request); > We can't depend on DCHECK doing the overflow check since we won't have it in release build. I think it's fine to assume there is no overflow because in one iteration, we either increment num_controllers_with_request or num_controllers_for_preconnect and not both. So |job_controller_set_.size()| should be greater or equal to sum of num_controllers_with_request and num_controller_for_preconnect. I think we should get rid of the DCHECK. https://codereview.chromium.org/2824313002/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2824313002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:397: DCHECK_GE(job_controller_set_.size(), num_controllers_for_preconnect); I think this is redundant -- we are only incrementing num_controllers_for_preconnect while iterating job_controller_set_.
I'm happy to defer to xunjieli here.
https://codereview.chromium.org/2824313002/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2824313002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:397: DCHECK_GE(job_controller_set_.size(), num_controllers_for_preconnect); On 2017/04/19 17:50:04, xunjieli wrote: > I think this is redundant -- we are only incrementing > num_controllers_for_preconnect while iterating job_controller_set_. Alright, let's get rid of all those DCHECKs.
On 2017/04/19 18:43:51, Zhongyi Shi wrote: > https://codereview.chromium.org/2824313002/diff/20001/net/http/http_stream_fa... > File net/http/http_stream_factory_impl.cc (right): > > https://codereview.chromium.org/2824313002/diff/20001/net/http/http_stream_fa... > net/http/http_stream_factory_impl.cc:397: DCHECK_GE(job_controller_set_.size(), > num_controllers_for_preconnect); > On 2017/04/19 17:50:04, xunjieli wrote: > > I think this is redundant -- we are only incrementing > > num_controllers_for_preconnect while iterating job_controller_set_. > > Alright, let's get rid of all those DCHECKs. lgtm
The CQ bit was checked by zhongyi@chromium.org
zhongyi@chromium.org changed reviewers: - rch@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zhongyi@chromium.org changed reviewers: + holte@chromium.org
holte@: Steven, ptal!
lgtm
The CQ bit was checked by zhongyi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492644888672690,
"parent_rev": "77ac96ae13745efb0300a75232dca1251cd5ada2", "commit_rev":
"6c16897074391089d36b098902192f6845a11aa5"}
Message was sent while issue was closed.
Description was changed from ========== Split JobController count histograms into three separate histograms: 1. JobController for preconnect, 2. JobController for normal connect, has pending Request, 3. JobController for normal connect, has no Request. The third one will give us better estimation of uncleaned JobControllers and be used to detect memory leaks in JobController layer. BUG=704988 ========== to ========== Split JobController count histograms into three separate histograms: 1. JobController for preconnect, 2. JobController for normal connect, has pending Request, 3. JobController for normal connect, has no Request. The third one will give us better estimation of uncleaned JobControllers and be used to detect memory leaks in JobController layer. BUG=704988 Review-Url: https://codereview.chromium.org/2824313002 Cr-Commit-Position: refs/heads/master@{#465808} Committed: https://chromium.googlesource.com/chromium/src/+/6c16897074391089d36b09890219... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6c16897074391089d36b09890219... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
