New issue
Advanced search Search tips

Issue 854902 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 846235



Sign in to add a comment

Avoid hopping back to main thread for ServiceWorkerSubresourceLoaderFactory

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

Issue description

Currently it's created on the main thread and CreateAndStart() is delayed when the main thread is busy.
Making the subresource loader factory on the IO thread will resolve the performance issue.
 

Comment 1 by kinuko@chromium.org, Jun 22 2018

Summary: Avoid hopping back to main thread for ServiceWorkerSubresourceLoaderFactory (was: Create ServiceWorkerSubresourceLoaderFactory for pages on IO thread)
Changing the summary.

The point is we shouldn't run SWSubresourceLoaderFactory tasks asyncly on the main thread, which can be largely delayed if there's a long-running task.  We should be able to fix this either by:

1) Create SWSubresourceLoaderFactory on a different thread (e.g. on IO thread or on a background thread. In our case this needs to be the background thread as SWSubresourceLoader needs to be able to issue sync IPCs to resolve blobs without deadlock (when Network Service is not enabled).
This is being explored by: https://chromium-review.googlesource.com/c/chromium/src/+/1109662

2) Avoid using mojo for going through ServiceWorkerSubresourceLoaderFactory, at least for document cases. (For worker cases we still need the thread-hop to avoid dead-lock with sync XHR)

I'll also look into #2

Comment 2 by kinuko@chromium.org, Jun 22 2018

Cc: falken@chromium.org bashi@chromium.org

Comment 3 by kinuko@chromium.org, Jun 22 2018

For the record for ourselves:

https://chromium-review.googlesource.com/c/chromium/src/+/1111759 is a PoC patch for #2, but measurement result was worse than approach #1 (while still better than original).

Hypothesis here is for network fallback cases we still need to come back to the main thread with #2, while not with #1.
Blocking: 846235

Comment 5 by dougt@chromium.org, Jun 26 2018

Components: -Internals>Services>Network

Comment 6 by dxie@google.com, Jun 26 2018

Labels: Hotlist-KnownIssue
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 27 2018

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

commit 3fa048671bc144726c932121d6f50e26af616112
Author: Kinuko Yasuda <kinuko@chromium.org>
Date: Wed Jun 27 00:52:14 2018

ServiceWorker S13N: always create SubresourceLoader on a background thread

So that we don't need to come back to the main thread for handling
subresource loading with SW. On a low-end device needing to post a
task on the main thread could add huge delay (due to main thread
contention).

This also changes the worker code path to create SubresourceLoader
on a background thread, but not on IO thread. (Because when NS is not
enabled we need to resolve Blob ptrs if request_body has Blobs, and that
require synchronous IPC that could introduce dead-lock if issued from
the IO thread)

Change-Id: Ibae07ae8fecd42bfcc9943ae1da129a4b062ca22
Bug:  854902 
Reviewed-on: https://chromium-review.googlesource.com/1109662
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570594}
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/base/threading/thread_restrictions.h
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/common/service_worker/controller_service_worker.mojom
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/service_worker/controller_service_worker_connector.cc
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/service_worker/controller_service_worker_connector.h
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/service_worker/service_worker_provider_context.cc
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/service_worker/service_worker_provider_context_unittest.cc
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/service_worker/worker_fetch_context_impl.h
[modify] https://crrev.com/3fa048671bc144726c932121d6f50e26af616112/content/renderer/shared_worker/embedded_shared_worker_stub.cc

Labels: M-69
Status: Fixed (was: Assigned)
I think this is done too.
Status: Started (was: Fixed)
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 5

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

commit 3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6
Author: Kinuko Yasuda <kinuko@chromium.org>
Date: Thu Jul 05 08:25:31 2018

SWS13N cleanup: Stop using mojo for cross-thread access to ControllerSWConnector

We moved SubresourceLoader on a background task runner in crrev.com/c/1109662,
but it used Mojo for in-process cross-thread communication but it wasn't
really necessary. This stops using Mojo for talking to ControllerSWConnector
and makes it non-refcounted.

Bug:  854902 
Change-Id: Ideac79e74c07bed7e0d519dc2fbabcbe39025e48
Reviewed-on: https://chromium-review.googlesource.com/1123436
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572737}
[modify] https://crrev.com/3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6/content/common/service_worker/controller_service_worker.mojom
[modify] https://crrev.com/3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6/content/renderer/service_worker/controller_service_worker_connector.cc
[modify] https://crrev.com/3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6/content/renderer/service_worker/controller_service_worker_connector.h
[modify] https://crrev.com/3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6/content/renderer/service_worker/service_worker_provider_context.cc
[modify] https://crrev.com/3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6/content/renderer/service_worker/worker_fetch_context_impl.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 7

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

commit 5176dc409d6729af4b1e0c17f487e227abb7bc48
Author: Matt Falkenhagen <falken@chromium.org>
Date: Sat Jul 07 01:10:24 2018

Revert "SWS13N cleanup: Stop using mojo for cross-thread access to ControllerSWConnector"

This reverts commit 3a6b2a4de3cd3741e5fef0bbf75e8f3f0c37fcc6.

Reason for revert: Flaky on TSAN and some bots for ServiceWorkerProviderContextTest.SetControllerServiceWorker.

Original change's description:
> SWS13N cleanup: Stop using mojo for cross-thread access to ControllerSWConnector
> 
> We moved SubresourceLoader on a background task runner in crrev.com/c/1109662,
> but it used Mojo for in-process cross-thread communication but it wasn't
> really necessary. This stops using Mojo for talking to ControllerSWConnector
> and makes it non-refcounted.
> 
> Bug:  854902 
> Change-Id: Ideac79e74c07bed7e0d519dc2fbabcbe39025e48
> Reviewed-on: https://chromium-review.googlesource.com/1123436
> Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#572737}

TBR=falken@chromium.org,kinuko@chromium.org,shimazu@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  854902 ,  860688 
Change-Id: I44abbb8cb7fcdee40dff2b4a2c6bc6848585a77c
Reviewed-on: https://chromium-review.googlesource.com/1128419
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573135}
[modify] https://crrev.com/5176dc409d6729af4b1e0c17f487e227abb7bc48/content/common/service_worker/controller_service_worker.mojom
[modify] https://crrev.com/5176dc409d6729af4b1e0c17f487e227abb7bc48/content/renderer/service_worker/controller_service_worker_connector.cc
[modify] https://crrev.com/5176dc409d6729af4b1e0c17f487e227abb7bc48/content/renderer/service_worker/controller_service_worker_connector.h
[modify] https://crrev.com/5176dc409d6729af4b1e0c17f487e227abb7bc48/content/renderer/service_worker/service_worker_provider_context.cc
[modify] https://crrev.com/5176dc409d6729af4b1e0c17f487e227abb7bc48/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/5176dc409d6729af4b1e0c17f487e227abb7bc48/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/5176dc409d6729af4b1e0c17f487e227abb7bc48/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/5176dc409d6729af4b1e0c17f487e227abb7bc48/content/renderer/service_worker/worker_fetch_context_impl.cc

Status: Fixed (was: Started)
Cc: wanderview@chromium.org kinuko@chromium.org
 Issue 882425  has been merged into this issue.

Sign in to add a comment