UMA shows EmbeddedWorkerInstance.Start.TimeToURLJob looks increased, and the number seems matching the amount of increase of StartWorker.Time.
https://uma.googleplex.com/timeline_v2?sid=65c9e98cfa0ff86d300781bff2e8417c

StartWorker during starting up the browser got much slower:
https://uma.googleplex.com/timeline_v2?sid=0908cde9122e420a761117f613ad8290

Scheduler or startup procedure might have changed.

The number of START_WORKER_FAILED for main resources has also increased.

https://uma.googleplex.com/timeline_v2?sid=658cfb217f56aaf0872ac7ebdbec4f5b

Is the regression on 59.0.3048.0 Canary for Win and Mac for the 95p? Do you see anything in the blame range?

I think it is affected by the PlzNavigate Finch experiment.

https://uma.googleplex.com/p/chrome/variations/?sid=b97b8cc291891be93bb2f9f707525c55

Thanks!
The experiment started from the same date with the performance regression.

// BTW, interestingly the distribution of the startup time is bimodal.

When PlzNavigate is enabled, Chrome tends to create a new renderer process for ServiceWorker.

This is a race of IPC ping pong and ServiceWorker Database.

If the browser process finishes reading the ServiceWorker Database and calls ServiceWorkerProcessManager::AllocateWorkerProcess before the browser process receives the Mojo IPC of ServiceWorkerDispatcherHost::OnProviderCreated, ServiceWorkerProcessManager creates a new renderer process for ServiceWorker.
It is because FindAvailableProcess() returns kInvalidUniqueID.

I think we should revisit the logic of ServiceWorkerProcessManager for PlzNavigate.

SW team should likely own this. Possible ways forward:
- Teach ServiceWorkerProcessManager about ServiceWorkerProviderHosts. When the only ProviderHost available has a kInvalidUniqueID, we know it's pending creation by PlzNavigate. In that case, wait until the ID is populated.
- For navigations with PlzNavigate, SWURLRequestJob should somehow wait for the ServiceWorkerProviderHost to be created before trying to start the worker.

Thank you for tracking this. We are hoping that PlzNavigate can go to stable in M59, and the NTP slowness was vexing us. If this can be fixed in the next week, that would be really appreciated. That way we can remove the old navigation code paths soon.

I intend to fix this in M59. Note that the team has a service worker face-to-face W3C meeting on Mon and Tue next week.

Thanks!

I'm investigating. I haven't been able to detect the race identified in comment 7. Actually it looks like we always create a new process for the service worker.

clamy@: Do you know in what circumstances OnProviderCreated will be sent to the browser? It seems to me it's only happening after the response starts coming in, which only happens after the sw already responded to the event.

In detail...

First, the SWControlleeRequestHandler associates the registration with the provider host, still with process id -1. This adds a process reference for |pattern| with process id of -1.
#1 content::ServiceWorkerProcessManager::AddProcessReferenceToPattern()
#2 content::ServiceWorkerProviderHost::IncreaseProcessReference()
#3 content::ServiceWorkerProviderHost::AddMatchingRegistration()
#4 content::ServiceWorkerProviderHost::AssociateRegistration()
#5
content::ServiceWorkerControlleeRequestHandler::DidLookupRegistrationForMainResource()

Then, the SW starts and responds to the fetch event. This causes the navigation to be committed.
#1 content::RenderFrameHostImpl::CommitNavigation()
#2 content::NavigationRequest::CommitNavigation()
#3 content::NavigationRequest::OnWillProcessResponseChecksComplete()
#4 content::NavigationHandleImpl::RunCompleteCallback()
#5 content::NavigationHandleImpl::WillProcessResponse()
#6 content::NavigationRequest::OnResponseStarted()
#7 content::NavigationURLLoaderImpl::NotifyResponseStarted()


On renderer-side, this causes the SWNetworkProvider to be created which sends the OnProviderCreated, and populates the process id properly.
#1 content::ServiceWorkerNetworkProvider::ServiceWorkerNetworkProvider()
#2 content::ServiceWorkerNetworkProvider::CreateForNavigation()
#3 content::RenderFrameImpl::didCreateDataSource()
#4 blink::LocalFrameClientImpl::createDocumentLoader()
#5 blink::FrameLoader::createDocumentLoader()
#6 blink::FrameLoader::startLoad()
#7 blink::FrameLoader::load()
#8 blink::WebLocalFrameImpl::load()
#9 content::RenderFrameImpl::NavigateInternal()
#10 content::RenderFrameImpl::OnCommitNavigation()

Browser-side receives the OnProviderCreated message, populates the process id of the SWProviderHost and registers the process with SWProcessManager:

#1 0x7f2335eb65c7 content::ServiceWorkerProviderHost::FinalizeInitialization()
#2 0x7f2335eb69e6 content::ServiceWorkerProviderHost::CompleteNavigationInitialized()
#3 0x7f2335e98866 content::ServiceWorkerDispatcherHost::OnProviderCreated()

It seems to me in the current code, we can't populate the process id of the SWProviderHost until the response starts, which can only happen after the service worker starts and responds to the event. So the service worker will always be created in a new process (unless a suitable process already existed before the navigation.)

I think we're likely creating SW process before (or maybe while) we're creating the final renderer process, it doesn't feel we can fix this only by changing SW's process selection code.  (Needs to look into code & behavior to make sure but that's my gut feeling)

Just looked this over with kinuko@. It looks like the speculative renderer for PlzNavigate is created first, then the SW process creation happens because it doesn't recognize it as a valid renderer.

#1 0x7f856be19267 content::RenderProcessHostImpl::Init()
#2 0x7f856bc54e99 content::RenderFrameHostManager::CreateSpeculativeRenderFrameHost()
#3 0x7f856bc53ff6 content::RenderFrameHostManager::GetFrameHostForNavigation()
#4 0x7f856bc53dcf content::RenderFrameHostManager::DidCreateNavigationRequest()
#5 0x7f856bc17924 content::FrameTreeNode::CreatedNavigationRequest()
#6 0x7f856bc341ab content::NavigatorImpl::RequestNavigation()
#7 0x7f856bc33810 content::NavigatorImpl::NavigateToEntry()
#8 0x7f856bc34475 content::NavigatorImpl::NavigateToPendingEntry()
#9 0x7f856bc23557 content::NavigationControllerImpl::NavigateToPendingEntryInternal()
#10 0x7f856bc1e28f content::NavigationControllerImpl::NavigateToPendingEntry()
#11 0x7f856bc1f694 content::NavigationControllerImpl::LoadURLWithParams()
#12 0x7f85700bb33b (anonymous namespace)::LoadURLInContents()
#13 0x7f85700bad15 chrome::Navigate()

Seems like the simplest path would be to teach SW to use the speculative renderer, but we'd probably also need to handle throwing away and creating new speculative renderers in case of cross-origin redirects.

There's a good chance this change won't make the M59 branch cut.

Remove Restrict-View-Google since there's nothing secret here, just UMA links that won't be viewable.

I agree with the diagnostic so far. I'll try to get a patch up asap that uses teh speculative rph for the SW.

+creis for questions about SiteInstances

Update on that bug: I tried to get the speculativeSiteInstance to bind to a url for the whole day, and it turns out that we already do that. However I noticed something: for the new tab page we have SiteInstances bound to chrome://newtab/ and chrome-search://remote-ntp/, so it seems relatively reasonable that when we attempting to get one for https://www.google.fr/_/chrome/newtab-serviceworker.js we don't actually use any of the SiteInstances we've created for the navigation.

I think we reuse one in the current code because the SW does not use the normal method of selecting a process (ie through SiteInstance) right away. Instead, it goes through a map of RPH it keeps and associates to a pattern, which I imagine must be initialized based on the RPH id contained in the ServiceWorkerProviderHost (-1 in PlzNavigate case). Is that correct?

If that's the case, I could hack something that passes the sepculative RPH id to the SWPH, but this is likely to have a certain number of edge cases. Honestly I'm quite surprised that we don't fully rely on the SiteInstance mechanism.

Can any ServiceWorker folks answer clamy's question in comment 20 about how ServiceWorkers select a process?  I'm not familiar with that logic.  (It does seem unfortunate not to use SiteInstances for this, but I don't know what the constraints were.)

I'll investigate today and have a fuller response, but a quick response before Pacific time goes to sleep:
- ServiceWorkerProcessManager was primarily written by jyasskin@ some years ago, and we haven't really had a need to investigate or change it since.
- Service worker has to play some tricks in RPH to keep the processes alive even when a tab is closed (see the "worker ref count" in RPH and RPHI). That might be involved with why we're using RPH over SiteInstance.

> Instead, it goes through a map of RPH it keeps and associates to a pattern, which I imagine must be initialized based on the RPH id contained in the ServiceWorkerProviderHost (-1 in PlzNavigate case). Is that correct?

Yes, I've observed this too.

Fyi, there was some discussion around the possibility to just use SiteInstance for SW process selection way long time ago. I don't really remember the details but here's some minutes: https://docs.google.com/document/d/1BuzC3HQfMpWN1I-4jbNeF0nddqXB-mHEGfAyjfAAySs/edit

I remember a little better now: one of the reasons we didn't use SiteInstance was that we wanted to pick the same process next to the page in the same origin as much as possible, but SiteInstance's GetProcess doesn't (didn't?) work in that way by default (and looks like it doesn't work with NTP either), and  issue 372045  was the candidate solution we came up with at the time.

creis@: do you think trying something like this https://github.com/jyasskin/chromium/commit/637823fb6ac759b462194a8f35f358d343292e03 could be still valid?  (Using special scheme feels hacky, but we could probably use additional params etc)

Thanks kinuko@! Just to aid my understanding, here's what I think the current situation is.

When starting a service worker we would ideally just do:

site_instance = SiteInstance::CreateForURL()
site_instance->GetProcess()

But Chrome is currently using process-per-site-instance, so this will create a new process every time.

The current service worker code uses SiteInstance in the cases that there is no renderer process available. As an optimization, we also keep track of renderer processes. We have a map of GURL to process id. The GURL is the service worker registration scope (“pattern”). We add to the map when a document becomes associated with a service worker, in ServiceWorkerProviderHost::IncreaseProcessReference.

I did some testing with what would happen if service worker always used SiteInstance, and used the --process-per-site flag (which should be a bigger hammer then jyasskin's patch), and saw whether sw and the resulting page were in the same process or not.

      PlzNavigate + PPS       PlzNavigate + non-PPS
A     out-of-process          out-of-process
B     same process            out-of-process
C     out-of-process          out-of-process
D     same process            out-of-process

A. open new tab page
B. from a page, click to a same-origin link to page that uses sw
C. from a page, click to a cross-origin link to page that uses sw
D. from new tab page, using the omnibox navigate to a page that uses sw

Yes the issue is that SiteInstance::CreateForURL always creates a new SiteInstance. I've been thinking about issue and am thinking about the following:
- we could pass information that allows to get to the FrameTreeNode/RenderFrameHost, so that we get the current SiteInstance of the frame that started the navigation. This way we can use SiteInstanceImpl::GetRelatedSiteInstance, which will reuse an existing SiteInstance from the BrowsingInstance if there is an appropriate one. Caveats: this is limited to one browsing instance, and plumbing this kind of info through the SW code is not easy.
- add a method that tries to find the appropriate SiteInstance among the SiteInstances of all the BrowsingInstances of the BrowserContext, and if it doesn't create a new one. I think this would more in line with what SW wants, right?

creis, do you think it's doable?

Yes the latter option sounds nicer at first glance. Since service workers can be started for reasons other than navigation, it'd be nice to have a solution that doesn't need to special case navigation.

BTW just to clarify, in comment 26 I think we want all of A through D to be "same process". The reason --process-per-site is not enough is (I think) because the SiteInstance can be bound to a URL that's not a match for the final URL the navigation ends up in. For example, for "A" it's bound to chrome://newtab/ (or chrome-search://remote-ntp/) [as noted in comment 20] and for "C" it's bound to the original page being navigated away from. So finding the appropriate SiteInstance somehow needs to look through all URLs associated with that SiteInstance.

Is it really a problem if it's not in the same process, as long as we avoid creating a new process?

Yes it's ok as long as we avoid creating a new process unnecessarily.

I thought there might be some crazy sequence of events whereby the security model demands creating a new process for the navigation, and service worker startup creates a new process that the page ends up living in. This kind of process creation for service worker would be OK, so I wanted to use "same process" and "out of process" terms for the table.

Just so we don't lose track.. what are the next steps here?

creis@ do you think using SiteInstance for this is doable?

To check for feasibility, I've created a patch at https://codereview.chromium.org/2803673006/ that tries to implement a SiteInstance lookup function that ServiceWorker can use. In order to do this I did two main changes:

1) Keep a map of BrowsingInstances. This way we can iterate over them and look for a SiteInstance that has the right site.

With this, the ServiceWorker starts in the speculative process when we create one, but we still have issues when we don't. In particular, in renderer-intiated navigations and in the first navigation in a tab, we end up creating an extra-process for the SW.

2) To fix this, I'm also keeping track of which sites we attempted to display in the SiteInstance (which I update at the beginning of navigation). I use this information to lookup a SiteInstance to use for the SW.

With this, the SW does not create a process for renderer-intiated navigations and the first navigation in a tab. It still does for the SW of the new tab page.

Charlie: would you be ok with this kind of approach? This would fix most of the issue between SW and PlzNavigate.

Seems both Charlie and Nasko are away. alexmos: Would you be able to give advice here about the proposed changes to SiteInstance?

Sorry for missing the discussion, and thanks for putting together the draft CL in comment 33!  I've looked over the approach and I have a few suggestions for an alternative.

One of the downsides with the current patch is that we'd be returning an existing SiteInstance for the ServiceWorker, which kind violates that abstraction-- it's meant to track which pages have references to each other and are same-site "enough" to need to share a process.  It'd be better to create a new SiteInstance for the ServiceWorker and have it end up in an existing process if possible, as falken@ mentions in comment 26.  This is actually how process-per-site works: we get a new SiteInstance for each NTP (etc), but behind the scenes (in RPHI) we make it end up in the same process as all the other pages from that site.

I think we want to go for a generalization of that here.  Something like:
site_instance = SiteInstance::CreateForURL(service_worker_url);
site_instance->SetUseExistingProcessIfPossible(true);
site_instance->GetProcess();

This API could then be used for ServiceWorker as well as process-per-site (e.g., things like the NTP or Settings would always set that bit to true and would get the process-per-site behavior as a result).  I'd be interested in using that API for OOPIFs as well, so that subframes could be punted into an existing process if possible.

Here's a sketch of how we might get there, which I think we should expand into a design doc to answer the open questions:

1) Keep a set of site URLs in each RenderProcessHostImpl, and update it on every navigation commit.
(Questions: Do we need to refcount and remove elements as the user navigates?  Maybe not?  Should we be tracking origins instead of sites?)
2) Add something like SetUseExistingProcessIfPossible to SiteInstance and track the bit.
3) If the bit is set when GetProcess is called, iterate over the processes in the BrowserContext and find any that already have the new SiteInstance's site URL.  From this list of candidates, pick a random one.

nick@ has already been thinking about tracking sites/origins in each process for implementing security checks, so that's an advantage as well.  While origins might have been nice, I'm guessing we might want to use site URLs-- that would probably make the NTP case work, and it's what SiteInstance would have on hand after the CreateForURL call.  We would definitely want to track site URLs for *all* commits in the process, though, not just the ones from SiteInstance::GetSiteURL (to address case C in comment 26).

(I did kind of like that Camille's CL put this list of sites/origins onto SiteInstance, but I think it probably makes more sense to track it on RPH for now.  At some point we may move that onto SiteInstance to get a better sense of what's in each one, but it doesn't seem necessary at the moment.)

Nick pointed out that there may be a few hurdles with RPH vs RPHI vs MockRPH, but we can get through those if needed.

In general, does this sound like an approach that would meet ServiceWorker's needs?  You'd be able to create a SiteInstance with |url| and it would use an existing process that contains GetSiteURL(|url|) if one exists.

(Side note: whatever we do here, it probably won't be mergeable, so let's retarget this to M60.)

> In general, does this sound like an approach that would meet ServiceWorker's needs?  You'd be able to create a SiteInstance with |url| and it would use an existing process that contains GetSiteURL(|url|) if one exists.

Yes, this sounds fine. As long as the SiteInstance abstraction does this for us we're happy.

Agree with targeting M60.

I'm curious why NTP is a special case. Since NTP is the metric that regressed (and is important) we should make sure whatever design we choose handles NTP correctly. I guess the SiteInstance is bound to chrome://newtab/ and Chrome internally maps it to a URL like https://www.google.co.jp/_/chrome/newtab? Does that work just like a HTTP redirect?

Also, I'm going away for 2-3 weeks for vacation and then Golden Week in Japan. Anyway, it looks like the core work here is in SiteInstance. I'm tentatively assigning the bug to clamy@ but kick back to me if you disagree and I'll find a SW owner while I'm away (kinuko@?).

I can pick that up. I'll try to write a design doc ASAP, so that we can agree on the finer points of detail.

Fantastic-- thanks clamy@!  I'm happy to help as needed.

And for the NTP thing, I think the conversion is probably done via GetEffectiveURL, up in ChromeContentBrowserClient.  That's why using a "site URL" might help in that case.

This is great, thank you clamy@!

I've put up a doc about the issue at https://docs.google.com/a/chromium.org/document/d/1Zujvf5jcUtm1hyh-xhXYlyfPjdO2oEplz22fOhSj660/edit?usp=sharing.

Thanks, have made some comments (mostly fyi ones).

Left some comments too, but overall it seems like a reasonable approach.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/63960c37c94873829b5937bf512809800f6eef6a

commit 63960c37c94873829b5937bf512809800f6eef6a
Author: clamy <clamy@chromium.org>
Date: Wed May 10 22:57:07 2017

Implement ProcessReusePolicy for SiteInstances

This CL is the first step in an effort to add new policies for reusing
processes when creating SiteInstances. It adds a ProcessReusePolicy that
defines the rules for reusing processes, and which is used by
RenderProcessHostImpl when attempting to create a RenderProcessHost for
the SiteInstance. As a side-effect, the code for creating a RPH has been
moved from SiteInstanceImpl::GetProcess to a static method in
RenderProcessHostImpl. See
https://docs.google.com/a/chromium.org/document/d/1Zujvf5jcUtm1hyh-xhXYlyfPjdO2oEplz22fOhSj660/edit?usp=sharing
for the motivation behind this change.

BUG= 705318 

Review-Url: https://codereview.chromium.org/2861433002
Cr-Commit-Position: refs/heads/master@{#470740}

[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/browser/browsing_instance.cc
[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/browser/media/capture/web_contents_video_capture_device_unittest.cc
[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/browser/media/session/audio_focus_manager_unittest.cc
[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/browser/site_instance_impl.cc
[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/browser/site_instance_impl.h
[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/63960c37c94873829b5937bf512809800f6eef6a/content/test/test_render_view_host_factory.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5d947f5a0c60d33fa16b6609978bd712a497cb00

commit 5d947f5a0c60d33fa16b6609978bd712a497cb00
Author: clamy <clamy@chromium.org>
Date: Wed May 24 19:51:18 2017

PlzNavigate: implement process reuse for ServiceWorkers

This Cl implements a new ProcessReusePolicy,
REUSE_CHECKING_FRAMES_AND_NAVIGATIONS to use when creating SiteInstances for
ServiceWorkers in PlzNavigate. When this policy is used, we will try to reuse a
RenderProcessHost that is hosting a frame for the site, or that is expecting a
navigation to the site.

See https://docs.google.com/a/chromium.org/document/d/1Zujvf5jcUtm1hyh-xhXYlyfPjdO2oEplz22fOhSj660/edit?usp=sharing
for the background behing this change.

BUG= 705318 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2857213005
Cr-Commit-Position: refs/heads/master@{#474395}

[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/service_worker/service_worker_process_manager.cc
[modify] https://crrev.com/5d947f5a0c60d33fa16b6609978bd712a497cb00/content/browser/site_instance_impl.h

Looking up at metrics, the last CL seems to have helped bringing the ServiceWorker start time down on PlzNavigate.
Before the CL we had:
median time without PlzNavigate: 53ms
median time with PlzNavigate: 145ms (+172%)

Following the CL, we have:
median time without PlzNavigate: 30ms
median time with PlzNavigate: 38ms (+29%)

We still have issues: the 75th and 90th percentiles still suffer with PlzNavigate. We see two reasons for that.
1) The new tab page case is still not solved. This is because we have to ensure that the new tab page SW site url is resolved to the same site url as the NTP (currently it is not).
2) Cross-process redirects are not supported by the ProcessReusePolicy.

The current plan is to focus on 1 first, and then try to address 2 if the metrics are still bad.

Thanks Camile, #47 the number looks a lot better now!  I agree that we should focus on 1) first.

Great improvement.

So for the numbers in 47, does this mean that the cl also improved the non-plznavigate numbers? If I'm reading this correctly, before this cl the non-plznavigate number was 53ms, and after this cl the plznavigate number is 38ms? If so, that means that with your change, turning on PlzNavigate wouldn't be a regression from M59 right? i.e. yes we would have had an even bigger improvement if we don't turn on plznavigate, but IMO as long as plznavigate doesn't regress that's all we need for now. We can add further improvements later.

Yes, part of the CL should also improve the non PlzNavigate situation. However, unless 1) is fixed, we still have a regression in the load time of the NTP, which is a problem for turning on PlzNavigate. I have a patch awaiting review for this at https://codereview.chromium.org/2898313003/.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4262ab9315b24197a00cc6c942474144c1e4a7f

commit a4262ab9315b24197a00cc6c942474144c1e4a7f
Author: clamy <clamy@chromium.org>
Date: Tue May 30 16:53:28 2017

Ensure the NTP ServiceWorker has the proper site URL

This CL ensures the ServiceWorker of the NTP gets recognized as such,
and that it is assigned the same effective URL as the NTP. This allows
it to run in the same process as the NTP when PlzNavigate is enabled,
fixing a regression on the PLT of the NTP.

BUG= 705318 

Review-Url: https://codereview.chromium.org/2898313003
Cr-Commit-Position: refs/heads/master@{#475566}

[modify] https://crrev.com/a4262ab9315b24197a00cc6c942474144c1e4a7f/chrome/browser/search/search.cc
[modify] https://crrev.com/a4262ab9315b24197a00cc6c942474144c1e4a7f/chrome/browser/search/search_unittest.cc
[modify] https://crrev.com/a4262ab9315b24197a00cc6c942474144c1e4a7f/chrome/common/search/search_urls.cc
[modify] https://crrev.com/a4262ab9315b24197a00cc6c942474144c1e4a7f/chrome/common/search/search_urls.h

This is great. 

Where are the numbers in comment 47 from?

How did the CL improve non-PlzNavigate?

I think this was a measurement error (presumably the population was different between the two days considered). I have started this doc to try to track measurements of the SW start time in various canaries (https://docs.google.com/a/chromium.org/spreadsheets/d/1aIUVS7ROMtNiPjWjPhWILD6I72OKlDwZ5C5T4vIxu4Y/edit?usp=sharing) - I'm using 28 days aggregate to get as much data for each version as possible.

Basically, the CL in #46 seems to have improved the median and 75th percentile for PlzNavigate, though it is still not at the level of non-PlzNavigate behavior. They both go up afterwards, but considering that this is also the case without PlzNavigate, I suspect I don't have enough data for the latest canaries yet.

What remains to be seen is the impact of #51, which hasn't made it to Canary yet. I'll update the doc and the bug when that happens. Once that's done, this should give us better insight on the need to address the cross-process redirect case.

So based on the preliminary results coming from the latest Canary, the NTP regression has been fixed \o/. However, we're still seeing a regression on ServiceWorker start time. So we'll have to look at the cross-site redirect case to fix this issue.

Awesome!

request merge approval then?

As per offline meeting, let's wait for full day worth of data (not partial) to make a call on this one. It looks positive so far, but I don't think partial data is trustworthy.

There was a spike of the error rate of ServiceWorker.FetchEvent.MainResource.Status with PlzNavigate after 60.0.3110.0 especially on Android.
See the  issue 728030  for the details.

I think it was caused by 5d947f5a0c60d33fa16b6609978bd712a497cb00 which landed in 60.0.3110.0. 

Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d60a32d7e2aa25fef5daf100bfa49990672fc6d

commit 8d60a32d7e2aa25fef5daf100bfa49990672fc6d
Author: clamy <clamy@chromium.org>
Date: Tue Jun 06 12:56:25 2017

Ensure the NTP ServiceWorker has the proper site URL

This CL ensures the ServiceWorker of the NTP gets recognized as such,
and that it is assigned the same effective URL as the NTP. This allows
it to run in the same process as the NTP when PlzNavigate is enabled,
fixing a regression on the PLT of the NTP.

BUG= 705318 

Review-Url: https://codereview.chromium.org/2898313003
Cr-Original-Commit-Position: refs/heads/master@{#475566}
Review-Url: https://codereview.chromium.org/2925833002 .
Cr-Commit-Position: refs/branch-heads/3112@{#179}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/8d60a32d7e2aa25fef5daf100bfa49990672fc6d/chrome/browser/search/search.cc
[modify] https://crrev.com/8d60a32d7e2aa25fef5daf100bfa49990672fc6d/chrome/browser/search/search_unittest.cc
[modify] https://crrev.com/8d60a32d7e2aa25fef5daf100bfa49990672fc6d/chrome/common/search/search_urls.cc
[modify] https://crrev.com/8d60a32d7e2aa25fef5daf100bfa49990672fc6d/chrome/common/search/search_urls.h

Since the start time of SWs with PlzNavigate still seems pretty long, I have a CL with metrics at https://codereview.chromium.org/2931633002/. This will allow us to check how often the mechanism landed above leads to a process being reused when we instantiate the SW. I suspect the proportion of successful reuses is not high enough. This is likely due to cross-process redirects.

I think the increase of ServiceWorker.StartWorker.Time is not a bug now.

The reason of the increase is that the service workers are tend to be started in the launching renderer process when PlzNavigate is enabled.

https://uma.googleplex.com/p/chrome/variations/?sid=2bd346798ff5cb83d972e36c584810f6
The decreased total count of ServiceWorker.StartWorker.Time_ExistingProcess is almost same as the increased total count of ServiceWorker.StartWorker.Time_NewProcess.
And ServiceWorker.StartWorker.Time_NewProcess looks improved.
I think they are caused by the case of the launching renderer processes which have already been created by PlzNavigate.

PageLoad.Clients.ServiceWorker.ParseTiming.NavigationToParseStart looks improved by PlzNavigate.

Win: https://uma.googleplex.com/p/chrome/variations/?sid=399c64a6df6d6e01b0c17700c5fbd732
Mac: https://uma.googleplex.com/p/chrome/variations/?sid=251a352448597a74396c33e33785f285
Android: https://uma.googleplex.com/p/chrome/variations/?sid=3109e12e82d2c3e683d9003f28343340

#c63 - is it possible to break those down by new nagivations vs back/forward ones by any chance? We are seeing back/forward being slower with PlzNavigate, so wanted to understand whether the SW metrics will confirm this. Poking at the histograms, I couldn't find it.

#c64: I don't believe we have page load or service worker UMA for back/forward navigations (+bcmquade).

However back/forward navigation is indeed a known perf issue caused by service worker. Chromium performs a back/forward navigation with some flag like "prefer using the cache" so responses usually come from the disk cache. But when service worker is used, it has to startup and send the requests to the service worker, which is slower.

Actually there is PageLoad.PaintTiming.NavigationToFirstContentfulPaint.LoadType.ForwardBackNavigation and maybe others with "ForwardBack" in the name.

We are already tracking the various PageLoad.*.LoadType.ForwardBackNavigation metrics, I was just curious if the ServiceWorker ones are split out.

Thanks for the insight about SW and the cache flag interaction. It is definitely an aspect I wasn't aware of. However, with field trials and equally sized groups, I'd expect to see similar distribution of SW cases to non-SW cases, so we can assume that any regression in our trial is likely not dependent on the SW loaded navigations ignore the cache flag. I might be wrong though, so any ideas on how to drill into data to find out are appreciated, or possibly extra metrics we can add.

nasko: Would it help if we added PageLoad.Clients.ServiceWorker.ParseTiming.NavigationToParseStart.LoadType.ForwardBackNavigation and PageLoad.Clients.NoServiceWorker.ParseTiming.NavigationToParseStart.LoadType.ForwardBackNavigation so we can look at these UMAs with PlzNavigate enabled vs disabled?

I don't know if we can drill into data with the existing metrics.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/014fa484cfd3972bfae6296776502369750a6ed2

commit 014fa484cfd3972bfae6296776502369750a6ed2
Author: clamy <clamy@chromium.org>
Date: Wed Jun 14 16:15:08 2017

Add a metric to check how often SiteInstances can reuse processes

We introduced a mechanism that allows SiteInstances to try to reuse
RenderProcessHosts in https://codereview.chromium.org/2857213005. This
CL adds a metric to check how often
ProcessReusePolicy::REUSE_CHECKING_FRAMES_AND_NAVIGATIONS leads to the
reuse of an existing RenderPorcessHost.

BUG= 705318 

Review-Url: https://codereview.chromium.org/2931633002
Cr-Commit-Position: refs/heads/master@{#479407}

[modify] https://crrev.com/014fa484cfd3972bfae6296776502369750a6ed2/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/014fa484cfd3972bfae6296776502369750a6ed2/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/014fa484cfd3972bfae6296776502369750a6ed2/tools/metrics/histograms/histograms.xml

falken@: I think it will be good to have those broken down, at least for consistency with the rest of the metrics. It will allow us to have the same break downs and reason whether SW is contributor to changes or not.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/429a81302fb5e00aba20e364db624c8eb110f881

commit 429a81302fb5e00aba20e364db624c8eb110f881
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Jun 22 23:36:18 2017

service worker: UMA for ForwardBack navigations.

Help debugging for PlzNavigate regression. We want to see if SW-controlled loads
make an outsized contribution to the PlzNavigate FowardBack regression.
See bug, comment 64.

Bug:  705318 
Change-Id: I28136ee2a1e220a6d9584b168fe13308434cc55f
Reviewed-on: https://chromium-review.googlesource.com/541257
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481721}
[modify] https://crrev.com/429a81302fb5e00aba20e364db624c8eb110f881/chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.cc
[modify] https://crrev.com/429a81302fb5e00aba20e364db624c8eb110f881/chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer.h
[modify] https://crrev.com/429a81302fb5e00aba20e364db624c8eb110f881/chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/429a81302fb5e00aba20e364db624c8eb110f881/tools/metrics/histograms/histograms.xml

nasko@: I added some UMA. FWIW I haven't compared with the generic loads, but preliminary results from Windows Canary don't seem to show an outsized contribution to the regression from service worker controlled loads:


https://uma.googleplex.com/p/chrome/histograms/?endDate=20170625&dayCount=7&histograms=PageLoad.Clients.ServiceWorker.PaintTiming.NavigationToFirstContentfulPaint.LoadType.ForwardBackNavigation%2CPageLoad.Clients.ServiceWorker.ParseTiming.NavigationToParseStart.LoadType.ForwardBackNavigation&fixupData=true&showMax=true&analysis=0.1%200.2%200.3%200.4%200.5%200.6%200.7%200.75%200.8%200.9%200.95%200.98%200.99%200.995&filters=platform%2Ceq%2CW%2Cchannel%2Ceq%2C1%2Cfinch%2Cstgr_filter%2C3330980259%7C1859949255%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

https://uma.googleplex.com/p/chrome/histograms/?endDate=20170625&dayCount=7&histograms=PageLoad.Clients.ServiceWorker.PaintTiming.NavigationToFirstContentfulPaint.LoadType.ForwardBackNavigation%2CPageLoad.Clients.ServiceWorker.ParseTiming.NavigationToParseStart.LoadType.ForwardBackNavigation&fixupData=true&showMax=true&analysis=0.1%200.2%200.3%200.4%200.5%200.6%200.7%200.75%200.8%200.9%200.95%200.98%200.99%200.995&filters=platform%2Ceq%2CW%2Cchannel%2Ceq%2C1%2Cfinch%2Cstgr_filter%2C3330980259%7C529291635%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

I prefer a different view of the data, as it lets us look at the two groups side by side: https://uma.googleplex.com/p/chrome/variations/?sid=0e407da76f08efaaff0bfe26acbf3196. So far the number of samples is too small to have any viable conclusion, but it looks like the tail end is worse.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0304a310bb32a007c09041384cd58f5a38d18e47

commit 0304a310bb32a007c09041384cd58f5a38d18e47
Author: clamy <clamy@chromium.org>
Date: Fri Jun 30 16:26:22 2017

PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects

This CL is a follow up of the implementation of
ProcessReusePolicy::REUSE_COMMITTED_OR_PENDING_SITE
(https://codereview.chromium.org/2857213005/). It adds cross-site
redirects that should be committed by an existing SiteInstance to the
list of expected sites tracked per RenderProcessHost in
RenderProcessHostImpl.

BUG= 705318 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2954623003
Cr-Commit-Position: refs/heads/master@{#483726}

[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/frame_host/navigation_handle_impl_unittest.cc
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/frame_host/render_frame_host_manager.h
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/loader/navigation_resource_throttle.cc
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/0304a310bb32a007c09041384cd58f5a38d18e47/content/public/test/navigation_simulator.cc

clamy@ are there any remaining known issues or work planned here?

Here's my understanding:
* We expect start worker time to regress with PlzNavigate, because SW startup now happens before a process is ready. That's fine.
* We expect navigation time to be about the same when SW is used, especially if the site is using Navigation Preload. That's the important bit.
* We had bugs in earlier versions where processes were not being reused. clamy@ fixed these bugs in comment 45 and 46 (process reuse, landed in 60) comment 60 (NewTabPage, merged to 60), comment 74 (cross-site redirects, landed in 61).

It's a weird time to look at data since 61 is not yet in Beta, but the latest data looks in line with the above. In fact there might be a slight improvement on page load time.
Windows Beta 60: https://uma.googleplex.com/p/chrome/variations/?sid=1314270a14fcf0ff0fd8c19a7657cda0
Windows Canary 62: https://uma.googleplex.com/p/chrome/variations/?sid=546078eee0cf92a72da8720a1c4f021f
Android Canary 62: https://uma.googleplex.com/p/chrome/variations/?sid=02dae79a8e475788333c577f49a27522

If there are no known issues left, it seems find to close this as Fixed and open a new bug if something new is discovered with PlzNavigate + SW interaction.

I agree that the performance seems ok by now. We don't have any other improvements planned, so I'm closing the bug.

Apologies, applied the wrong component in bulk.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0e4b4840bff841de8ae65d01e1b3b1482da8c77

commit b0e4b4840bff841de8ae65d01e1b3b1482da8c77
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Jan 12 07:48:57 2018

service worker: Remove manual process management.

ServiceWorkerProcessManager had a code path that manually
tracked renderer processes that was only used in non-PlzNavigate.
The PlzNavigate code path uses SiteInstance for process reuse
for service workers implemented for  issue 705318 .

Since non-PlzNavigate is not supported, we can remove the
manual tracking.

Bug:  705318 , 789577
Change-Id: I66d51e4534e983cddf9f94bb0b9e688c0103ea95
Reviewed-on: https://chromium-review.googlesource.com/863005
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528905}
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/service_worker_process_manager.cc
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/service_worker_process_manager.h
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/service_worker_process_manager_unittest.cc
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/service_worker_provider_host_unittest.cc
[modify] https://crrev.com/b0e4b4840bff841de8ae65d01e1b3b1482da8c77/content/browser/service_worker/service_worker_version_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b93d16f6215f223d4482acfd6cf934eee8a7dc19

commit b93d16f6215f223d4482acfd6cf934eee8a7dc19
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jan 15 03:39:08 2018

service worker: Remove obsolete code needed for manual process management.

Follow-up to https://chromium-review.googlesource.com/863005.
This patch removes the now no-op functions from ServiceWorkerProviderHost
and the tree of callsites to them.

Bug:  705318 , 789577
Change-Id: I0618a106237bff0e33d1e8ed4c19d3d417b8a968
Reviewed-on: https://chromium-review.googlesource.com/863864
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529195}
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/background_fetch/background_fetch_test_base.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/background_sync/background_sync_manager_unittest.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/background_sync/background_sync_service_impl_unittest.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/notifications/platform_notification_context_unittest.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/payments/payment_app_content_unittest_base.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_context_core.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_context_core.h
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_context_unittest.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_context_watcher_unittest.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_context_wrapper.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_job_coordinator.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_job_coordinator.h
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_job_unittest.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_register_job.cc
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_register_job.h
[modify] https://crrev.com/b93d16f6215f223d4482acfd6cf934eee8a7dc19/content/browser/service_worker/service_worker_registration_object_host.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c

commit ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jan 15 04:43:38 2018

service worker: Remove SimulateAddProcessToPattern

Follow-up to https://chromium-review.googlesource.com/863005

Bug:  705318 , 789577
Change-Id: Idd57bc8215c41880c5c33c06022d24a68b850e98
Reviewed-on: https://chromium-review.googlesource.com/864462
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529200}
[modify] https://crrev.com/ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c/content/browser/service_worker/service_worker_context_unittest.cc
[modify] https://crrev.com/ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
[modify] https://crrev.com/ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c/content/browser/service_worker/service_worker_handle_unittest.cc
[modify] https://crrev.com/ce52ae6fecebeb9be485e8a4d81cbe03c6721c3c/content/browser/service_worker/service_worker_version_unittest.cc