All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=884331

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=2aa030b755e372d7d15f3078689dc1c6b7cdb6102e3592e34d8df4c661793da3


Bot(s) for this bug's original alert(s):

Android Nexus5X WebView Perf
Android Nexus6 WebView Perf
linux-perf
mac-10_12_laptop_low_end-perf
mac-10_13_laptop_high_end-perf
win-10-perf

blink_perf.owp_storage - Benchmark documentation link:
  https://bit.ly/blink-perf-benchmarks

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/17957795640000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/17957795640000

Tune the mojo::DataPipe capacity and read chunk size when reading blobs. by wanderview@chromium.org
https://chromium.googlesource.com/chromium/src/+/25b9a8e66efcad49b254d4c0a1b09cb9a9c8a6b8
17.85 → 35.8 (+17.95)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Probably part of the regression here is because the smaller chunk size results in more Read*Item calls? And since those metrics purely measure wall time spent in those methods in the browser process it might overall be better even if the metrics seem worse... Or at least this might be a case of micro-benchmark not being representative of the real world.

(that wouldn't explain the slowdown for blob-perf-tiny though)

The default chunk size should not have been changed by my landing.  It should still be 64kb chunks.

If there is overhead for creating pipes proportional to their size, then that could explain it.  I'll see about exactly sizing pipe capacity to the blob size when less than 512kb.

It looks like the posix datapipe does an ftruncate to size the shared memory file and zero it out:

https://cs.chromium.org/chromium/src/base/memory/platform_shared_memory_region_posix.cc?l=269&rcl=4a19ae010b1c420cb3650a29a72a5ef6ac93f627

If its really zero'ing memory, though, it would be somewhat surprising for it to take so many extra milliseconds.  I would expect overhead more in the microseconds...

Another possibility is that all the increased buffers are creating memory pressure.

Anyway, solution is still probably the same as #c9.

This is what the blob URL path (I think) does:

https://cs.chromium.org/chromium/src/storage/browser/blob/blob_transport_strategy.cc?l=154&rcl=4a19ae010b1c420cb3650a29a72a5ef6ac93f627

Here is a wip CL:

https://chromium-review.googlesource.com/c/chromium/src/+/1227092

It compiles and runs, but I haven't evaluated if it has any improvement.  I need to figure out how to run the perf bots on CQ.

I started a couple pinpoint jobs to test the WIP CL:

https://pinpoint-dot-chromeperf.appspot.com/job/12e679fd640000
https://pinpoint-dot-chromeperf.appspot.com/job/117aff9b640000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/17f10979640000

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/17f10979640000

All of the runs failed. The most common error (20/20 runs) was:
BuildError: Build failed: BUILD_FAILURE

Pinpoint is having trouble, but I see some nice performance improvements on a particular site from this patch locally.  I'll clean it up and get it ready for review.

FWIW, instrumentation shows that creating a pipe takes ~500us on my linux VM.  In some cases, though, it takes up to 15ms which is quite long.  It might skew towards these longer numbers when using larger pipes, but I haven't taken enough measurements to verify that.

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

commit 130518b367014dade21c360afd6bf50986fb4f79
Author: Ben Kelly <wanderview@chromium.org>
Date: Tue Sep 18 00:33:36 2018

Use exact size for mojo::DataPipes when reading smaller blobs.

In  bug 881141  we expanded the DataPipe capacity used when reading
blobs for many code paths.  The goal was to improve performance
for large blobs on hardware that would benefit from buffering.
Unfortunately, this resulted in over-allocation for many smaller
blobs.

This CL will exactly size DataPipes to the target blob's size
as long as that size is both known and less than the configured
capacity of 512kb.  Blobs with an unknown or larger size will
receive a DataPipe limited to 512kb.

R=kinuko@chromium.org, mek@chromium.org

Bug:  884331 
Change-Id: I652afc58add8e073ad121e5eaaa4dd763d39ace1
Reviewed-on: https://chromium-review.googlesource.com/1227092
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591896}
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/browser/cache_storage/cache_storage_blob_to_disk_cache.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/browser/cache_storage/cache_storage_blob_to_disk_cache.h
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/browser/cache_storage/cache_storage_blob_to_disk_cache_unittest.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/browser/service_worker/service_worker_installed_script_reader.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/browser/service_worker/service_worker_navigation_loader.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/common/service_worker/service_worker_loader_helpers.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/common/service_worker/service_worker_loader_helpers.h
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/content/renderer/service_worker/service_worker_subresource_loader.h
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/storage/browser/blob/blob_data_builder.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/storage/browser/blob/blob_data_item.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/storage/browser/blob/blob_data_item.h
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/storage/browser/blob/blob_storage_context.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/third_party/blink/common/blob/blob_utils.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/third_party/blink/public/common/blob/blob_utils.h
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/third_party/blink/renderer/core/fetch/form_data_bytes_consumer.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/third_party/blink/renderer/core/fileapi/file_reader_loader.cc
[modify] https://crrev.com/130518b367014dade21c360afd6bf50986fb4f79/third_party/blink/renderer/modules/service_worker/fetch_respond_with_observer.cc

Issue 884318 has been merged into this issue.

It seems the perf tests are still showing the regression.  I'll investigate further.

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/1000237b640000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/12faa30f640000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/15befaa8e40000

📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/1000237b640000

📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/15befaa8e40000

📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/12faa30f640000

This is an acceptable regression for our owp_storage blob metrics.

The measurement recorded here ReadBytesItem or ReadFileItem - is the time it takes to read that item. If the size is bigger, then it will be bigger. There should be less of the event happening.

Marking this as WAI because this result makes sense.

I agree with closing this bug, but I want to clarify what I think the issue is for future readers.

I don't think ReadBytesItem or ReadFileItem are reading larger chunks.  The  NetToMojoPendingBuffer::BeginWrite code limits chunks to 64kb regardless of the max capacity.  You can see that here:

https://cs.chromium.org/chromium/src/services/network/public/cpp/net_adapters.cc?l=34&rcl=a6a5e65d3926c0825fe274a47f02717c0e3846c3

I think these operations are slightly slower because they are hitting more shared memory pages over the course of the test.  Previously the test would fill the 64kb pipe in one pass, the renderer would consume it, and then the browser process would fill the same 64kb of memory again.  With the new capacity values the browser process can fill beyond 64kb and hit additional shared memory pages.  This is probably slower due dirtying cache and needing to fault these extra pages into the process.  Of course its only microseconds slower, so its only really noticeable in the cases where the test is reading a memory blob.

Ultimately I don't think we want to make the pipes smaller.  We want the ability to buffer the data to handle the case where the renderer is consuming the data slowly due to page loading, etc.  The fact that it slows down the mostly-idle case is a reasonable price to pay for better performance during page loading.

 Issue 886643  has been merged into this issue.

 Issue 887287  has been merged into this issue.