Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2951643003:
Add UMA that periodically logs number of outstanding requests (Closed)
|
Created:
3 years, 6 months ago by Kunihiko Sakamoto Modified:
3 years, 5 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, rdsmith+watch_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA that periodically logs number of outstanding requests
This adds UMA to track the peak number of outstanding requests in
ResourceDispatcherHostImpl. The "MultiTabLoading" variant is logged
only when there are outstanding requests from multiple processes.
These metrics are logged periodically every 60 seconds.
BUG=729951
Review-Url: https://codereview.chromium.org/2951643003
Cr-Commit-Position: refs/heads/master@{#481838}
Committed: https://chromium.googlesource.com/chromium/src/+/aa00afae68c5ae7364b91fb1cfd2a4769bc9c863
Patch Set 1 #
Total comments: 10
Patch Set 2 : comments addressed #
Total comments: 6
Patch Set 3 : MultiTabLoading -> MultipleLoadingProcesses #Patch Set 4 : per-tab stats #
Total comments: 8
Patch Set 5 : comments addressed #
Total comments: 4
Patch Set 6 : Msec -> Sec and style fix #
Total comments: 1
Messages
Total messages: 49 (25 generated)
The CQ bit was checked by ksakamoto@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.
Description was changed from ========== WIP BUG= ========== to ========== Add UMA that periodically logs number of outstanding requests This adds UMA to track the peak number of outstanding requests in ResourceDispatcherHostImpl. The "MultiTabLoading" variant is logged only when there are outstanding requests from multiple processes. These metrics are logged periodically every 60 seconds. BUG=729951 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
ksakamoto@chromium.org changed reviewers: + toyoshim@chromium.org
Toyoshima-san, would you take a first look?
https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1990: if (outstanding_requests_stats_map_.size() >= 2 && This will report if there is one background tab that has a long-polling request, and it will be a very common case. Probably, we want to check if each map entry isn't in the "network quiet" state. https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2528: peak_outstanding_request_count_ = 0; this will not report at the next time if there are no inc/dec events in the next timer period. we may want to keep the current outstanding value? https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:516: // Records statistics about outstanding requests since last call, and reset s/last/the last/ ? https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:774: // Largest number of outstanding requests seen since last call to the last https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:779: // requests from two or more processes, since last call to the last
https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1990: if (outstanding_requests_stats_map_.size() >= 2 && On 2017/06/20 07:39:21, Takashi Toyoshima wrote: > This will report if there is one background tab that has a long-polling request, > and it will be a very common case. Probably, we want to check if each map entry > isn't in the "network quiet" state. Done. https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2528: peak_outstanding_request_count_ = 0; On 2017/06/20 07:39:21, Takashi Toyoshima wrote: > this will not report at the next time if there are no inc/dec events in the next > timer period. > we may want to keep the current outstanding value? Good catch, thanks! Done. https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:516: // Records statistics about outstanding requests since last call, and reset On 2017/06/20 07:39:22, Takashi Toyoshima wrote: > s/last/the last/ ? Done. https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:774: // Largest number of outstanding requests seen since last call to On 2017/06/20 07:39:22, Takashi Toyoshima wrote: > the last Done. https://codereview.chromium.org/2951643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:779: // requests from two or more processes, since last call to On 2017/06/20 07:39:21, Takashi Toyoshima wrote: > the last Done.
lgtm
ksakamoto@chromium.org changed reviewers: + isherman@chromium.org, yhirano@chromium.org
+yhirano for resource_dispatcher_host_impl +isherman for histograms PTAL, thanks!
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
drive-by: Let me add Helen to cc who added Net.ResourceDispatcherHost.OutstandingRequests.* metrics which seem very similar to these. What is the difference between "largest outstanding requests" and "peak outstanding requests"? It is confusing to me.
On 2017/06/21 03:24:11, on gerrit - ping aggressively wrote: > drive-by: Let me add Helen to cc who added > Net.ResourceDispatcherHost.OutstandingRequests.* metrics which seem very similar > to these. > > What is the difference between "largest outstanding requests" and "peak > outstanding requests"? It is confusing to me. IIUC, the counters for those existing metrics never get reset, so these metrics essentially represent the largest number during the browser session. We're interested in multi-tab opening user experiences in general (not only the worst case), so wanted to add new metrics that reports peak number in periods.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2528: peak_outstanding_request_count_ = num_in_flight_requests_; Intuitively I feel this should be reset to 0 because the current num_in_flight_requests is already rolled into the previous shot? https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2533: "Net.ResourceDispatcherHost.PeakOutstandingRequests.MultiTabLoading", The map we rely on groups requests per-process rather than per-tab, so calling this 'MultiTab' feels a bit misleading...
https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2528: peak_outstanding_request_count_ = num_in_flight_requests_; On 2017/06/21 04:00:10, kinuko wrote: > Intuitively I feel this should be reset to 0 because the current > num_in_flight_requests is already rolled into the previous shot? My intuition is that if there are n outstanding requests at the beginning of the time interval, the peak of the interval shouldn't be less than n. (Edge case: if there's no inc/dec event in the period, the peak should be n instead of 0.) https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2533: "Net.ResourceDispatcherHost.PeakOutstandingRequests.MultiTabLoading", On 2017/06/21 04:00:10, kinuko wrote: > The map we rely on groups requests per-process rather than per-tab, so calling > this 'MultiTab' feels a bit misleading... Yeah, you're right. Renamed the suffix to 'MultipleLoadingProcesses'.
https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2528: peak_outstanding_request_count_ = num_in_flight_requests_; On 2017/06/21 05:25:32, Kunihiko Sakamoto wrote: > On 2017/06/21 04:00:10, kinuko wrote: > > Intuitively I feel this should be reset to 0 because the current > > num_in_flight_requests is already rolled into the previous shot? > > My intuition is that if there are n outstanding requests at the beginning of the > time interval, the peak of the interval shouldn't be less than n. > (Edge case: if there's no inc/dec event in the period, the peak should be n > instead of 0.) Hm, ok I think that makes sense. https://codereview.chromium.org/2951643003/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2533: "Net.ResourceDispatcherHost.PeakOutstandingRequests.MultiTabLoading", On 2017/06/21 05:25:32, Kunihiko Sakamoto wrote: > On 2017/06/21 04:00:10, kinuko wrote: > > The map we rely on groups requests per-process rather than per-tab, so calling > > this 'MultiTab' feels a bit misleading... > > Yeah, you're right. Renamed the suffix to 'MultipleLoadingProcesses'. We could use routing_id to gather per-tab stats, how much more code would we need if we want to have that data? Clicking to open multiple tabs from single tab could easily end up with having all tabs within the same process (e.g. if rel=noopener is specified). I assume we need to add similar map code to the per-process one then. (We could wait for csharrison/Helen's response to see their gut feeling as well)
Please let's not use (render view) routing id, we're trying to rip it out of the code base for OOPIF. In OOPIF modes the routing id doesn't represent tabs, since there's a RVH per process. Per frame would be more reasonable, but it isn't quite as actionable data.
I am not sure how useful the peak stats will be, given that the upload interval is quite arbitrary. Are you expecting the numbers to decrease with the field trial? What other metrics you will be using with these new ones? An alternative you could consider (I am not sure if it's feasible) is to use metrics::MetricsProvider to report the peak stats rather than using a 60s timer. The main benefit is that you will be able to associate with other stats (e.g the memory UMAs) that are uploaded at the same interval.
On 2017/06/21 17:44:02, Charlie Harrison wrote: > Please let's not use (render view) routing id, we're trying to rip it out of the > code base for OOPIF. In OOPIF modes the routing id doesn't represent tabs, since > there's a RVH per process. Yes I can see your points. We could also try something like: record such stats only when OOPIF is disabled (so the code can just go away when it's enabled) with a clear TODO to remove it after a few milestones, as how we record stats with abstracted tab/frame/process concepts would anyway need to be resolved differently with servicification.
On 2017/06/21 17:53:12, xunjieli wrote: > I am not sure how useful the peak stats will be, given that the upload interval > is quite arbitrary. The interval was arbitrarily chosen from our local testing, but the gist is that we want to record the stats periodically so that we can get sampling distribution so I don't think that matters a lot? > Are you expecting the numbers to decrease with the field > trial? What other metrics you will be using with these new ones? This doc has more info about what stats we're interested in collecting: https://docs.google.com/document/d/1mf3pMcl4mvi8ew0h0KVAMVATQipNV3ERSp9WrtlPG... > An alternative you could consider (I am not sure if it's feasible) is to use > metrics::MetricsProvider to report the peak stats rather than using a 60s timer. > The main benefit is that you will be able to associate with other stats (e.g the > memory UMAs) that are uploaded at the same interval.
Thanks all for the suggestions and comments. As per kinuko's suggestion, changed to gather per-tab stats using routing_id only when OOPIF is not enabled. Also added TODO to remove once we finished collecting data for the experiment. I think we could use MetricsProvider, but I feel it may be overkill for temporary metric collection like this.
Thanks! This looks good to me as far as the code is temporary (and it's explicitly noted in the comment), but let's wait to see if it's acceptable by Charlie/Helen.
The CQ bit was checked by ksakamoto@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.
Generally looks good. Do you have a milestone for when this will be removed (a guess is fine). https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:346: peak_outstanding_request_count_(0), These aren't necessary if they are declared in the h file. https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:370: record_outstanding_requests_stats_timer_.reset(new base::RepeatingTimer()); nit: prefer base::MakeUnique to avoid bare new. https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1941: // No-op in OOPIF modes, because the routing id doesn't represent tabs. Can we just not initialize the timer in these cases? Checking command line flags more often than necessary is a personal pet peeve :) https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:795: int peak_outstanding_request_count_; = 0
Metrics LGTM, thanks.
On 2017/06/22 12:33:47, Charlie Harrison wrote: > Generally looks good. Do you have a milestone for when this will be removed (a > guess is fine). Assuming we could gather conclusive finch results from M61 stable, then we can remove this. So, around M63? https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:346: peak_outstanding_request_count_(0), On 2017/06/22 12:33:46, Charlie Harrison wrote: > These aren't necessary if they are declared in the h file. Done. https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:370: record_outstanding_requests_stats_timer_.reset(new base::RepeatingTimer()); On 2017/06/22 12:33:46, Charlie Harrison wrote: > nit: prefer base::MakeUnique to avoid bare new. Done. https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1941: // No-op in OOPIF modes, because the routing id doesn't represent tabs. On 2017/06/22 12:33:46, Charlie Harrison wrote: > Can we just not initialize the timer in these cases? Checking command line flags > more often than necessary is a personal pet peeve :) Done. https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2951643003/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:795: int peak_outstanding_request_count_; On 2017/06/22 12:33:46, Charlie Harrison wrote: > = 0 Done.
lgtm, thanks! (And thanks for reviewing Charlie as well) https://codereview.chromium.org/2951643003/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:173: const int kRecordOutstandingRequestsStatsIntervalMsec = 60000; nit: maybe use Sec rather than Msec (and TimeDelta::FromSeconds below) https://codereview.chromium.org/2951643003/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1955: } nit: no {} needed for single-line body here and below, per the convention in this file (and in chromium)
The CQ bit was checked by ksakamoto@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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Thanks! CQ-ing, if you have additional comments, let me address in a follow-up CL. https://codereview.chromium.org/2951643003/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:173: const int kRecordOutstandingRequestsStatsIntervalMsec = 60000; On 2017/06/23 04:35:06, kinuko wrote: > nit: maybe use Sec rather than Msec (and TimeDelta::FromSeconds below) Done. https://codereview.chromium.org/2951643003/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1955: } On 2017/06/23 04:35:06, kinuko wrote: > nit: no {} needed for single-line body here and below, per the convention in > this file (and in chromium) Done.
The CQ bit was checked by ksakamoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from toyoshim@chromium.org, isherman@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2951643003/#ps160001 (title: "Msec -> Sec and style fix")
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": 160001, "attempt_start_ts": 1498208902677530,
"parent_rev": "0a2e609cdc6ec3678f8340915b0ebc18c61b6e30", "commit_rev":
"aa00afae68c5ae7364b91fb1cfd2a4769bc9c863"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA that periodically logs number of outstanding requests This adds UMA to track the peak number of outstanding requests in ResourceDispatcherHostImpl. The "MultiTabLoading" variant is logged only when there are outstanding requests from multiple processes. These metrics are logged periodically every 60 seconds. BUG=729951 ========== to ========== Add UMA that periodically logs number of outstanding requests This adds UMA to track the peak number of outstanding requests in ResourceDispatcherHostImpl. The "MultiTabLoading" variant is logged only when there are outstanding requests from multiple processes. These metrics are logged periodically every 60 seconds. BUG=729951 Review-Url: https://codereview.chromium.org/2951643003 Cr-Commit-Position: refs/heads/master@{#481838} Committed: https://chromium.googlesource.com/chromium/src/+/aa00afae68c5ae7364b91fb1cfd2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/aa00afae68c5ae7364b91fb1cfd2...
Message was sent while issue was closed.
alexmos@chromium.org changed reviewers: + alexmos@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2951643003/diff/160001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2951643003/diff/160001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:373: !SiteIsolationPolicy::AreIsolatedOriginsEnabled()) { Just saw this CL, and this check seems problematic. OOPIFs have already been shipped and enabled by default for Isolate Extensions, so this isn't going to avoid all OOPIF scenarios. This also doesn't check for oopif-based webview (features::kGuestViewCrossProcessFrames), which relies on some OOPIF plumbing and is an experiment on canary/dev/beta and likely to ship in M61. This will also be problematic when we start baking certain OOPIF-enabled isolated origins into Chrome, which will likely happen within a few weeks. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
