ResourceDispatcherHostImpl::record_outstanding_requests_stats_timer_ is not compatible with OOPIFs |
|||||||
Issue description
I've stumbled across the following comment in content/browser/loader/resource_dispatcher_host_impl.cc:
ResourceDispatcherHostImpl::ResourceDispatcherHostImpl(
...
// Monitor per-tab outstanding requests only if OOPIF is not enabled, because
// the routing id doesn't represent tabs in OOPIF modes.
if (!SiteIsolationPolicy::UseDedicatedProcessesForAllSites() &&
!SiteIsolationPolicy::IsTopDocumentIsolationEnabled() &&
!SiteIsolationPolicy::AreIsolatedOriginsEnabled()) {
record_outstanding_requests_stats_timer_ =
std::make_unique<base::RepeatingTimer>();
}
I think having a bug to track the problem above is desirable.
I am also a bit surprised that the comment indicates that "OOPIF is not enabled" == "!--site-per-process && !--top-document-isolation && !--isolate-origins=...". OOPIFs are always enabled since --isolate-extensions became the default mode in M56.
⛆ |
|
|
,
Nov 22 2017
,
Nov 27 2017
Thank you alexmos@ and lukasza@ for pointing this out. This code is used only for reporting of Net.ResourceDispatcherHost.PeakOutstandingRequests(.MultiTabLoading) UMA histograms. The .MultiTabLoading variant is recorded only if there were loading activity from multiple route_ids. With OOPIF, this isn't equivalent to "there's multiple loading tabs". We're okay with having some noise in the histograms, from OOPIF-enabled page loads (Isolate Extensions or OOPIF-enabled isolated origins), as long as that noise is small and evenly distributed among all of our experiment groups. This code will go away once we wrap up the ResourceLoadScheduler experiment (currently running in stable channel).
,
Nov 27 2017
Thanks for the background. Please note that the number of OOPIFs seen in the wild might increase in M63 (and later) depending on the adoption of the policy implemented in issue 783842 . Hopefully this won't matter for the ResourceLoadScheduler experiment.
,
Mar 23 2018
,
Mar 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2304dfae66a8716045bdd5638fddf8ff8eb5002d commit 2304dfae66a8716045bdd5638fddf8ff8eb5002d Author: Kunihiko Sakamoto <ksakamoto@chromium.org> Date: Wed Mar 28 02:39:05 2018 Remove Net.ResourceDispatcherHost.PeakOutstandingRequests UMA This is essentially a revert of http://crrev.com/2951643003. This was a temporary UMA only to measure the impact of Loading Dispatcher v0, and it wasn't work correctly when OOPIF is enabled. Bug: 787866 ,825081 Change-Id: Icc4644d97e70a2c8a793737763a0b4ca1be55180 Reviewed-on: https://chromium-review.googlesource.com/977202 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#546370} [modify] https://crrev.com/2304dfae66a8716045bdd5638fddf8ff8eb5002d/content/browser/loader/resource_dispatcher_host_impl.cc [modify] https://crrev.com/2304dfae66a8716045bdd5638fddf8ff8eb5002d/content/browser/loader/resource_dispatcher_host_impl.h [modify] https://crrev.com/2304dfae66a8716045bdd5638fddf8ff8eb5002d/tools/metrics/histograms/histograms.xml
,
Mar 28 2018
|
||||
►
Sign in to add a comment |
|||||||
Comment 1 by lukasza@chromium.org
, Nov 22 2017Components: -Internals>Network Blink>Loader
Owner: ksakamoto@chromium.org
Status: Assigned (was: Untriaged)