Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752193003:
Add histograms to track the number of job controller and job that are (Closed)
|
Created:
3 years, 9 months ago by Zhongyi Shi Modified:
3 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, net-reviews_chromium.org, ssid Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histograms to track the number of job controller and job that are
not cleaned up when HttpStreamFactoryImpl is destructed.
BUG=700617
Review-Url: https://codereview.chromium.org/2752193003
Cr-Commit-Position: refs/heads/master@{#458228}
Committed: https://chromium.googlesource.com/chromium/src/+/de52d1630afa30d255f1e773d350c758056fd2ed
Patch Set 1 #
Total comments: 11
Patch Set 2 : address Helen's comments #
Total comments: 6
Patch Set 3 : address Alexei's comments #
Messages
Total messages: 22 (9 generated)
The CQ bit was checked by zhongyi@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...
zhongyi@chromium.org changed reviewers: + asvitkine@chromium.org, xunjieli@chromium.org
I'll set up finch chirp alert for these histograms once this CL landed. PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:116: // For non-preconnects. Suggest add a DCHECK(it->HasPendingAltJob()||it->HasPendingMainJob()) here to make sure we won't have JobController hanging around when there aren't any jobs. https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:122: UMA_HISTOGRAM_COUNTS_1M("Net.JobControllerSet.CountOfJobController", This histogram is not necessary, right? We can deduce this from the other three. In other words, this histogram should strictly follow the trend of the other three. https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:126: UMA_HISTOGRAM_COUNTS_1M("Net.JobControllerSet.CountOfAltJob", alt_job_count); For someone who might not know this code, I suggest make a distinction in the name of the histograms regarding preconnect and nonpreconnect. "CountOfAltJob" -> "CountOfNonPreconnectAltJob" "CountOfMainJob" -> "CountOfNonPreconnectMainJob" https://codereview.chromium.org/2752193003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752193003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:33964: + <summary>The counts number of alternative jobs which are uncleaned.</summary> nit: "The counts number of" -> "The number of" and below https://codereview.chromium.org/2752193003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:33983: + uncleaned. nit: "uncleaned" -> "are uncleaned"
Thanks for the review. Helen, ptal! https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:116: // For non-preconnects. On 2017/03/17 00:47:56, xunjieli wrote: > Suggest add a DCHECK(it->HasPendingAltJob()||it->HasPendingMainJob()) here to > make sure we won't have JobController hanging around when there aren't any jobs. Done. https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:122: UMA_HISTOGRAM_COUNTS_1M("Net.JobControllerSet.CountOfJobController", On 2017/03/17 00:47:56, xunjieli wrote: > This histogram is not necessary, right? > We can deduce this from the other three. In other words, this histogram should > strictly follow the trend of the other three. Well, that's not exactly true. There might be JobController having both alt job and main job uncleaned up theoretically. Although we shouldn't run into that case since all the requests should be handed out to clients. https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl.h?l=169 https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:126: UMA_HISTOGRAM_COUNTS_1M("Net.JobControllerSet.CountOfAltJob", alt_job_count); On 2017/03/17 00:47:56, xunjieli wrote: > For someone who might not know this code, I suggest make a distinction in the > name of the histograms regarding preconnect and nonpreconnect. > > > "CountOfAltJob" -> "CountOfNonPreconnectAltJob" > "CountOfMainJob" -> "CountOfNonPreconnectMainJob" Done. https://codereview.chromium.org/2752193003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752193003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:33964: + <summary>The counts number of alternative jobs which are uncleaned.</summary> On 2017/03/17 00:47:56, xunjieli wrote: > nit: "The counts number of" -> "The number of" > and below Done. https://codereview.chromium.org/2752193003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:33983: + uncleaned. On 2017/03/17 00:47:56, xunjieli wrote: > nit: "uncleaned" -> "are uncleaned" Done.
lgtm https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2752193003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl.cc:122: UMA_HISTOGRAM_COUNTS_1M("Net.JobControllerSet.CountOfJobController", On 2017/03/17 02:02:29, Zhongyi Shi wrote: > On 2017/03/17 00:47:56, xunjieli wrote: > > This histogram is not necessary, right? > > We can deduce this from the other three. In other words, this histogram should > > strictly follow the trend of the other three. > > Well, that's not exactly true. There might be JobController having both alt job > and main job uncleaned up theoretically. Although we shouldn't run into that > case since all the requests should be handed out to clients. > https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl.h?l=169 Right, that's why I suggested using a DCHECK(it->HasPendingAltJob() || it->HasPendingMainJob()) so we should catch that unlikely case.
https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:109: size_t preconnect_controller_count = 0; Nit: Just use int since these are not compared with size_t types (like vector.size()). https://codereview.chromium.org/2752193003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752193003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33971: + <summary>This counts number of main jobs which are uncleaned.</summary> Please expand these descriptions. Reading this sentence doesn't make me understand what this is, when this is logged or what's it about.
https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:109: size_t preconnect_controller_count = 0; On 2017/03/20 15:51:58, Alexei Svitkine (slow) wrote: > Nit: Just use int since these are not compared with size_t types (like > vector.size()). Is this written somewhere? According to https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... , for object counts, "size_t" is recommended.
https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:109: size_t preconnect_controller_count = 0; On 2017/03/20 16:37:35, xunjieli wrote: > On 2017/03/20 15:51:58, Alexei Svitkine (slow) wrote: > > Nit: Just use int since these are not compared with size_t types (like > > vector.size()). > > Is this written somewhere? > According to > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > , for object counts, "size_t" is recommended. The normal C++ style guide suggests to use int types unless there's a reason not to: https://google.github.io/styleguide/cppguide.html#Integer_Types In Chromium, often there's a reason to use size_t because those end up being compared with standard library sizes - as I mentioned, but here it's not the case. You only end up passing the values to the histogram macros, which end up being a signed type (HistogramBase::Sample which is int32_t).
On 2017/03/20 16:56:04, Alexei Svitkine (slow) wrote: > https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... > File net/http/http_stream_factory_impl.cc (right): > > https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... > net/http/http_stream_factory_impl.cc:109: size_t preconnect_controller_count = > 0; > On 2017/03/20 16:37:35, xunjieli wrote: > > On 2017/03/20 15:51:58, Alexei Svitkine (slow) wrote: > > > Nit: Just use int since these are not compared with size_t types (like > > > vector.size()). > > > > Is this written somewhere? > > According to > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > , for object counts, "size_t" is recommended. > > The normal C++ style guide suggests to use int types unless there's a reason not > to: > https://google.github.io/styleguide/cppguide.html#Integer_Types > > In Chromium, often there's a reason to use size_t because those end up being > compared with standard library sizes - as I mentioned, but here it's not the > case. You only end up passing the values to the histogram macros, which end up > being a signed type (HistogramBase::Sample which is int32_t). Ok. That's good to know. Thanks.
Alexei, ptal, thanks! https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2752193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:109: size_t preconnect_controller_count = 0; On 2017/03/20 16:56:04, Alexei Svitkine (slow) wrote: > On 2017/03/20 16:37:35, xunjieli wrote: > > On 2017/03/20 15:51:58, Alexei Svitkine (slow) wrote: > > > Nit: Just use int since these are not compared with size_t types (like > > > vector.size()). > > > > Is this written somewhere? > > According to > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > , for object counts, "size_t" is recommended. > > The normal C++ style guide suggests to use int types unless there's a reason not > to: > https://google.github.io/styleguide/cppguide.html#Integer_Types > > In Chromium, often there's a reason to use size_t because those end up being > compared with standard library sizes - as I mentioned, but here it's not the > case. You only end up passing the values to the histogram macros, which end up > being a signed type (HistogramBase::Sample which is int32_t). Done. https://codereview.chromium.org/2752193003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752193003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33971: + <summary>This counts number of main jobs which are uncleaned.</summary> On 2017/03/20 15:51:58, Alexei Svitkine (slow) wrote: > Please expand these descriptions. > > Reading this sentence doesn't make me understand what this is, when this is > logged or what's it about. Done.
lgtm - descriptions are much clearer now, thanks!
Sure thing. Sorry about the collapsed description. Landing now.
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2752193003/#ps40001 (title: "address Alexei's comments")
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": 1490045539852710,
"parent_rev": "e682298d296d225092eb3bc9ffc98e0563365bbe", "commit_rev":
"de52d1630afa30d255f1e773d350c758056fd2ed"}
Message was sent while issue was closed.
Description was changed from ========== Add histograms to track the number of job controller and job that are not cleaned up when HttpStreamFactoryImpl is destructed. BUG=700617 ========== to ========== Add histograms to track the number of job controller and job that are not cleaned up when HttpStreamFactoryImpl is destructed. BUG=700617 Review-Url: https://codereview.chromium.org/2752193003 Cr-Commit-Position: refs/heads/master@{#458228} Committed: https://chromium.googlesource.com/chromium/src/+/de52d1630afa30d255f1e773d350... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/de52d1630afa30d255f1e773d350... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
