New issue
Advanced search Search tips

Issue 854905 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug-Regression

Blocking:
issue 853518
issue 859657



Sign in to add a comment

Send interface pointer for CacheStorage when service worker startup instead of using GetInterface()

Project Member Reported by shimazu@chromium.org, Jun 21 2018

Issue description

CacheStorage is a performacne sensitive API which can be used in service worker's fetch handler.
Currently, we are getting the interface pointer for CacheStorage by GetInterface() which is implemented by SWProviderHost. However, GetInterface() internally post a task to UI thread for creating a CacheStorageDispatcherHost, and post another task back to the IO thread for binding the interface request to the CacheStorageDispatcherHost.

If we can avoid the thread hopping, we may be able to avoid thread congestion.
 
Blocking: 859657
Labels: -Type-Bug Target-69 Type-Bug-Regression
This may actually be a regression in M67 due to:
https://chromium-review.googlesource.com/c/chromium/src/+/875510

Previously we didn't need an interface ptr to CacheStorage and just sent an IPC to the browser process's IO thread immediately.

We've seen this in traces and Docs is now reporting a serious regression in M67: issue 859657.

It seems we should do a quick solution for 69 or 70 where we send the CacheStorage ptr to the service worker.
I think we could do a quick solution like this:

(browser) During SW startup, after we get a RPHI in EmbeddedWorkerInstance on the UI thread. Call BindCacheStorage() to get a CacheStorage ptr. Send the ptr in ServiceWorkerProviderInfoForStartWorker to the renderer. 

(renderer) In blink/renderer/modules/cache_storage/cache_storage.cc
In ctor, if context is a SW, take the Ptr from it instead of GetInterface().

This assumes we don't make multiple CacheStorage objects for the same SW execution context.


Owner: falken@chromium.org
Status: Started (was: Available)
Cc: nhiroki@chromium.org
nhiroki: FYI I think c#2 will require plumbing a CacheStoragePtr through GlobalScopeCreationParams (like InterfaceProvider), since it has to be plumbed through to ServiceWorkerGlobalScope.
Blocking: 853518
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 4

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

commit d1d43314ace95237cea594fd77a262dfdffb1e4b
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Jul 04 12:34:50 2018

service worker: Eagerly provide CacheStoragePtr at worker startup.

Most service workers will need CacheStoragePtr in a performance
sensitive way. We discovered there is slowness using InterfaceProvider
to get the ptr on first use after worker startup.

Bug:  854905 
Change-Id: Iefcd8910d1b8b6ba1cfcc4df79c213abde0f9e3a
Reviewed-on: https://chromium-review.googlesource.com/1124130
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572544}
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/common/service_worker/service_worker_provider.mojom
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/public/browser/render_process_host.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/content/renderer/service_worker/embedded_worker_instance_client_impl.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/public/web/web_embedded_worker.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/renderer/modules/cache_storage/cache_storage.cc
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/renderer/modules/exported/web_embedded_worker_impl.cc
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/renderer/modules/exported/web_embedded_worker_impl.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.cc
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/renderer/modules/serviceworkers/service_worker_thread.cc
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/renderer/modules/serviceworkers/service_worker_thread.h
[modify] https://crrev.com/d1d43314ace95237cea594fd77a262dfdffb1e4b/third_party/blink/renderer/modules/serviceworkers/web_embedded_worker_impl_test.cc

Labels: M-69
Status: Fixed (was: Started)
Labels: -Target-69 FoundIn-67 Merge-Request-68 Target-68 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
I'd like to request merge to M68 as the change seems to be working on Canary and at least perf-neutral.

Fuller explanation:
This change was intended to fix a major performance regression (issue 859657) at the 95 percentile reported by Google Docs in M67 Stable when service worker is enabled. Chrome's generic page load metrics (over all sites) on M67 don't reflect that regression except on Chrome OS. This change hasn't yet rolled out to Chrome OS Canary, and even so Docs reports the regression only shows up in Stable channel. They might not have had dashboard data for M67 Beta, so it's unknown whether the change was visible in M67 Beta or just Stable, so it's unknown whether M68 will show the regression once it hits Stable without the merge. If we merge to M68, when it hits Stable we can know whether the issue is addressed rather than waiting until M69 Stable to know. 

From Chrome metrics on Windows Canary and Android Canary, the change (in 69.0.3482.0) seems perf-neutral for generic service worker controlled page loads.
https://uma.googleplex.com/p/chrome/timeline_v2/?sid=e4128f9d73113299e1d6b7c3105e4e93
https://uma.googleplex.com/p/chrome/timeline_v2/?sid=d73f08e9faa574692d760ace924efccb





Project Member

Comment 9 by sheriffbot@chromium.org, Jul 11

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Thanks for the explanation in comment#8. I would like to know if this change is finch gated so that in case something bad happens, that code path can be disabled server side.
It's not, unfortunately. That didn't occur to me. I should be able to add it simply though.
Please do, so that we can merge the fix safely.
I've added the feature flag so it can be disabled remotely if needed.

Requesting merge for d1d43314ace95237cea594fd77a262dfdffb1e4b and c25fea4b813655a45ff64a1d6f523504733b9e02 to M68.
Labels: -Hotlist-Merge-Review -Merge-Review-68 Merge-Approved-68
Please merge this as soon as possible. M68 stable cut is today.
Thanks. I assume M68 is 3440 as per the "M68 Branch is here" email. Will merge to 3440.
Current status: Fixed merge conflicts on the first patch. Trying to build locally then will merge if it looks good.
Ran into an unexpected build error due to a code bug that looks like it was fixed in https://chromium-review.googlesource.com/c/chromium/src/+/1081092 at revocable_interface_ptr.h that isn't in M68.

Now have to undo some Onion Soup we did moving content::ServiceWorkerStatusCode to blink::ServiceWorkerStatusCode.
Merged one patch in dc24fa1e3665197b62fd47fd12c2e507bcc614b2.

Will do the second patch after I arrive at work.
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 17

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc24fa1e3665197b62fd47fd12c2e507bcc614b2

commit dc24fa1e3665197b62fd47fd12c2e507bcc614b2
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jul 17 21:33:14 2018

M68: service worker: Eagerly provide CacheStoragePtr at worker startup.

Most service workers will need CacheStoragePtr in a performance
sensitive way. We discovered there is slowness using InterfaceProvider
to get the ptr on first use after worker startup.

(cherry picked from commit d1d43314ace95237cea594fd77a262dfdffb1e4b)

Bug:  854905 
Change-Id: Iefcd8910d1b8b6ba1cfcc4df79c213abde0f9e3a
Reviewed-on: https://chromium-review.googlesource.com/1124130
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#572544}
TBR: shimazu, kinuko
Reviewed-on: https://chromium-review.googlesource.com/1140337
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#704}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/common/service_worker/service_worker_provider.mojom
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/public/browser/render_process_host.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/content/renderer/service_worker/embedded_worker_instance_client_impl.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/public/web/web_embedded_worker.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/modules/cache_storage/cache_storage.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/modules/exported/web_embedded_worker_impl.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/modules/exported/web_embedded_worker_impl.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/modules/serviceworkers/service_worker_thread.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/modules/serviceworkers/service_worker_thread.h
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/modules/serviceworkers/web_embedded_worker_impl_test.cc
[modify] https://crrev.com/dc24fa1e3665197b62fd47fd12c2e507bcc614b2/third_party/blink/renderer/platform/mojo/revocable_interface_ptr.h

sigh, I implemented the feature flag in blink::features which doesn't exist in M68.
Project Member

Comment 22 by bugdroid1@chromium.org, Jul 17

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

commit a91947265e8883b9e444c4c96c9907e1b8f51c69
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jul 17 22:32:01 2018

M68: service worker: Gate eager CacheStorage setup behind a feature flag.

This will enable us to disable the optimization via a field trial kill
switch if needed.

(cherry picked from commit c25fea4b813655a45ff64a1d6f523504733b9e02)

Bug:  854905 
Change-Id: Ic312cbedd7685761a45fee986061767b14295afa
Reviewed-on: https://chromium-review.googlesource.com/1139457
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#575566}
TBR: kinuko
Reviewed-on: https://chromium-review.googlesource.com/1140338
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#705}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/a91947265e8883b9e444c4c96c9907e1b8f51c69/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/a91947265e8883b9e444c4c96c9907e1b8f51c69/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/a91947265e8883b9e444c4c96c9907e1b8f51c69/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/a91947265e8883b9e444c4c96c9907e1b8f51c69/third_party/blink/common/BUILD.gn
[add] https://crrev.com/a91947265e8883b9e444c4c96c9907e1b8f51c69/third_party/blink/common/features.cc
[modify] https://crrev.com/a91947265e8883b9e444c4c96c9907e1b8f51c69/third_party/blink/public/common/BUILD.gn
[add] https://crrev.com/a91947265e8883b9e444c4c96c9907e1b8f51c69/third_party/blink/public/common/features.h

Labels: -M-69 M-68
Merged both patches to M68 now. And I verified that the feature flag works on a local build of M68.

Sign in to add a comment