avoid creating extra mojo::DataPipe for navigation preload fetch handlers |
||||||
Issue descriptionCurrently the FetchRespondWithObserver creates a mojo::DataPipe for any Response that is not backed by a BlobDataHandle. See: https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc?l=291&rcl=66bb07a77fc8687d203e47223a64115ae5d727aa This path is followed for at least these cases: 1. The Response has a javascript-defined ReadableStream body. 2. The Response was produced by the fetch() function. 3. The Response was produced by the FetchEvent.preloadResponse promise. Now, a new mojo::DataPipe is needed for case (1) as far as I can tell. The js data must be written into a native pipe. For cases (2) and (3), however, the Response body already consists of a mojo::DataPipe wrapped in a BytesConsumerForDataConsumerHandle object. It should be possible to simply pass this pipe on to RespondToFetchEventWithResponseStream() directly. This would have a few advantages: a. Less data copying. b. Less memory usage. c. Removes the possibility of an extra process hop for each data chunk in the case where the service worker is running in a separate renderer from the outer client. d. Avoids the cost of creating the mojo::DataPipe on the service worker thread. In particular, (d) could be somewhat important. AFAICT creating a mojo::DataPipe requires sync I/O. On my fast linux VM it takes ~500us in many cases, but I frequently see it take 5ms to 15ms. Removing this delay for navigation preload FetchEvents could possibly be significant. I believe this change would benefit both the legacy and s13n modes.
,
Sep 18
Maybe we can use the MOJO_HANDLE_SIGNAL_PEER_CLOSED mechanism like the network service here: https://cs.chromium.org/chromium/src/services/network/url_loader.cc?l=690&rcl=a25ce958362ec7e1efb5a9e7f33d344eec78a7b5
,
Sep 21
It turns out that there are some additional layers of abstraction for the normal pass-through case. It uses the SharedMemoryDataConsumerHandle which does not have a handle to the DataPipe. Instead there is a loader upstream that is consumer the pipe and passing buffers to the SharedMemoryDataConsumerHandle. This is going to be a lot harder to convert to a zero-copy scheme. Lets focus this bug on just the navigation preload case for now.
,
Sep 21
WIP CL that at least works for the navigation preload case: https://chromium-review.googlesource.com/c/chromium/src/+/1239394 Simple test site: https://sw-pass-through.glitch.me/
,
Sep 25
Actually, I think I figured out how to make pass-through fetch also re-use the existing mojo::DataPipe. So lets try to still do that here.
,
Sep 25
The current WIP CL shows a statistically significant improvement of 18% on the pass-through-fetch case at: https://jakearchibald.github.io/service-worker-benchmark/
,
Sep 28
I'm going to split this into two CLs. The first will allow re-using the pipe, if present, in the service worker respondWith(): https://chromium-review.googlesource.com/c/chromium/src/+/1239394 The second will make the fetch() call use a DataPipe for its body so that we can take advantage of the re-use in the pass-through case: https://chromium-review.googlesource.com/c/chromium/src/+/1251841
,
Oct 2
And a third CL for removing WebDataConsumerHandle completely: https://chromium-review.googlesource.com/c/chromium/src/+/1257963
,
Oct 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f03111ab0c26320037e340051bb66ee88ea7722 commit 8f03111ab0c26320037e340051bb66ee88ea7722 Author: Ben Kelly <wanderview@chromium.org> Date: Fri Oct 05 15:16:57 2018 Make ServiceWorker use the existing Response DataPipe if one is available. When a ServiceWorker script passes a Response back to FetchEvent.respondWith() the browser must get the resulting Response body data back to the outer network loader. For non-blob bodies this is currently done by creating a new mojo::DataPipe and copying the body into its producer handle. The pipe's consumer handle is then passed back to the outer loader. In many cases, however, there is already a mojo::DataPipe for the Response body created by the network code. Its sub-optimal to allocate a new pipe and copy data across to it in these cases. This CL will attempt to extract the underlying mojo::DataPipe from the Response and pass it directly back to the outer loader. Currently this will only trigger for cases where the FetchEvent.preloadResponse is passed to respondWith(). In the future, however, we should be able to make this work for "pass-through" cases where the Response is produced by fetch(). That work is ongoing in https://crrev.com/c/1251841. R=kinuko@chromium.org Bug: 884807 Change-Id: I5a2964ff2e9463492ef72e5b48c5f383ebf36a86 Reviewed-on: https://chromium-review.googlesource.com/c/1239394 Commit-Queue: Ben Kelly <wanderview@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#597131} [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/content/browser/service_worker/service_worker_browsertest.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/content/renderer/service_worker/service_worker_context_client.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/content/renderer/service_worker/service_worker_context_client.h [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/content/renderer/service_worker/service_worker_context_client_unittest.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/public/mojom/service_worker/service_worker_stream_handle.mojom [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/public/web/modules/service_worker/web_service_worker_context_proxy.h [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/BUILD.gn [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/fetch/BUILD.gn [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/fetch/body_stream_buffer.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/fetch/bytes_consumer.h [add] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.cc [add] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.h [add] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer_test.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/fetch/fetch_data_loader.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/core/fetch/fetch_data_loader.h [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/modules/filesystem/file_system_writer.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/modules/service_worker/fetch_event.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/modules/service_worker/fetch_event.h [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/modules/service_worker/service_worker_global_scope_proxy.cc [modify] https://crrev.com/8f03111ab0c26320037e340051bb66ee88ea7722/third_party/blink/renderer/modules/service_worker/service_worker_global_scope_proxy.h
,
Oct 12
The initial navigation preload work landed in 71. I'm going to close this bug and open a follow-on for the remaining work.
,
Oct 12
,
Oct 12
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by wanderview@chromium.org
, Sep 17