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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDghpWLpAkM


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

android-nexus7v2

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978027441436963520

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978027371815012416

=== BISECT JOB RESULTS ===
Bisect failed for unknown reasons

Please contact the team (see below) and report the error.


Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : service_worker.service_worker_micro_benchmark
  Metric       : concurrent_1_response_90_percentile/concurrent_1_response_90_percentile


To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests service_worker.service_worker_micro_benchmark

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8978027441436963520

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5784141724909568


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

=== BISECT JOB RESULTS ===
Bisect failed for unknown reasons

Please contact the team (see below) and report the error.


Bisect Details
  Configuration: winx64intel_perf_bisect
  Benchmark    : service_worker.service_worker_micro_benchmark
  Metric       : concurrent_10_response_50_percentile/concurrent_10_response_50_percentile


To Run This Test
  src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests service_worker.service_worker_micro_benchmark

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8978027371815012416

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6357007281422336


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978023552320280080

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978023547194921072

=== BISECT JOB RESULTS ===
Bisect failed for unknown reasons

Please contact the team (see below) and report the error.


Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : service_worker.service_worker_micro_benchmark
  Metric       : concurrent_1_response_90_percentile/concurrent_1_response_90_percentile


To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests service_worker.service_worker_micro_benchmark

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8978023552320280080

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5784141724909568


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

=== BISECT JOB RESULTS ===
Bisect failed for unknown reasons

Please contact the team (see below) and report the error.


Bisect Details
  Configuration: winx64intel_perf_bisect
  Benchmark    : service_worker.service_worker_micro_benchmark
  Metric       : concurrent_10_response_50_percentile/concurrent_10_response_50_percentile


To Run This Test
  src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests service_worker.service_worker_micro_benchmark

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8978023547194921072

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6357007281422336


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8977939954499644656

Bisects were failing due to  crbug.com/728594 , restarting this.

=== Auto-CCing suspected CL author yhirano@chromium.org ===

Hi yhirano@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Yutaka Hirano
  Commit : f510d0c432ea0330c394e4a1349d6e7ee31c116e
  Date   : Mon May 29 15:30:31 2017
  Subject: Use Independent URLLoader

Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : service_worker.service_worker_micro_benchmark
  Metric       : concurrent_1_response_90_percentile/concurrent_1_response_90_percentile
  Change       : 54.74% | 9.71083333333 -> 15.0266666667

Revision             Result                   N
chromium@475352      9.71083 +- 0.236582      6      good
chromium@475354      9.60333 +- 0.170978      6      good
chromium@475355      9.59833 +- 0.614071      6      good
chromium@475356      14.7408 +- 0.879244      6      bad       <--
chromium@475360      14.4908 +- 1.32447       6      bad
chromium@475368      14.3067 +- 0.873861      6      bad
chromium@475383      15.0267 +- 2.43843       6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests service_worker.service_worker_micro_benchmark

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8977939954499644656

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=4965373989879808


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

I'm reverting the CL.

yzshen@, could you investigate the regression?

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

commit c9ad960e2f22c27c7a984ec5bdb14c612737c5ad
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Fri Jun 02 10:41:23 2017

Revert "Use Independent URLLoader"

This reverts commit f510d0c432ea0330c394e4a1349d6e7ee31c116e.

Reason for revert: Performance regression: See  crbug.com/728467 

Original change's description:
> Use Independent URLLoader
> 
> This CL removes "associated" for URLLoader in
> URLLoaderFactory::CreateLoaderAndStart.
> 
> The associated keyword was needed in order to keep the ordering guarantee
> which the current ChromeIPC provides, but it is harmful from the network
> servicification POV.
> 
> As all tests pass without the keyword (when kLoadingWithMojo is enabled),
> let's stop using the associated interface.
> 
> Bug:  706286 ,  724323 
> Change-Id: I771ee5ed35ca882cfa77c5e7ce2e862261c3f62d
> Reviewed-on: https://chromium-review.googlesource.com/513643
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#475356}

TBR=dcheng@chromium.org,kinuko@chromium.org,jam@chromium.org,yhirano@chromium.org
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug:  706286 ,  724323 ,  728467 

Change-Id: If0a91fe994b2b784020b69499a5206a41596075c
Reviewed-on: https://chromium-review.googlesource.com/522184
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476614}
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/appcache/appcache_url_loader_factory.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/appcache/appcache_url_loader_factory.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/navigation_url_loader_network_service.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_dispatcher_host_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_message_filter.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_message_filter.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_request_info_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_request_info_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/url_loader_factory_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/url_loader_factory_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/url_loader_factory_impl_unittest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/webui/web_ui_url_loader_factory.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/child/throttling_url_loader.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/child/throttling_url_loader_unittest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/child/url_loader_client_impl_unittest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/common/url_loader_factory.mojom
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/network_service_url_loader_factory_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/network_service_url_loader_factory_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/url_loader_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/url_loader_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/url_loader_unittest.cc

That seems surprising. Using independent message pipes is supposed to be positive for performance.

I will try to get a testing device and look at traces early next week.

I thought it's not independent message pipes per https://bugs.chromium.org/p/chromium/issues/detail?id=706286#c4 ?

The URLLoaderFactory is not independent message pipe, but URLLoader becomes independent with the CL.
I don't see how it could regress performance like this, though.

The big regression in concurrent_1_response_90_percentile looks recovered.

Yeah, that is because the CL f510d0c432ea0330c394e4a1349d6e7ee31c116e has been reverted.

I am still looking at the issue so that we could reland that CL.

Hi, horo@ and yhirano@

Are you familiar with this service worker perf test?

IIUC the test uses 500ms as timeout of waiting for response, and when it times out 10 seconds is used. 10 seconds is such a large value that dominates the average result.

I have seen locally that the result for 90_percentile fluctuates significantly because the 90_percentile number is usually >400ms and sometimes it exceeds the 500ms threshold.

How are these numbers determined? Thanks!

Are you using Debug build binary?
I think 500 ms in 90 percentile is super slow.

shimazu@ do you remember how did you decide the number?
https://github.com/makotoshimazu/Service-Worker-Performance/commit/f6447901bc22089b01939cd8cdc7cfb5c4be13c6

Re c#23:
dominicc@ actually created the original test bench and set the timeout at the time, but when I created the microbenchmark, the 500ms timeout was practically okay because requests were not so slow in usual cases.

According to the perf dashboard, currently 99%-ile and 90%-ile with 100 concurrent responses look hitting the timeout. That would be problematic.

For reference:
the perf dashboard is here
https://chromeperf.appspot.com/report?sid=de872224189bc4998367142bd1a85e1b20a2eb8d0b3b1937591f6b021c7ca97e&start_rev=302735&end_rev=478536

RE: #23, 24:

Thanks for reply!

My android build args:
================================
is_component_build = false                                                       
use_goma = true                                                                  
is_debug = false                                                                 
target_os = "android"                                                            
target_cpu = "arm"       
================================

Locally, I get:
concurrent_1_response_90_percentile: about 10ms
concurrent_10_response_90_percentile: about 50ms
concurrent_100_response_90_percentile: about 450ms

99 percentile numbers are slightly higher than 90 percentile.

Numbers of 99%-ile and 90%-ile with 100 concurrent responses are close to the timeout.
As #24 points out, this is also an issue on perf bots. For example:
https://chromeperf.appspot.com/report?sid=80c36d9c800da8c76e3c7020419d62bfaef8f20e8dba84f1f8f0ba4c9a6e2bd5

I couldn't reproduce the regression on my local machine.

One interesting observation is that it seems we *never* use the URLLoader interface to make calls. The only place that it is used is in MojoAsyncResourceHandler (at the browser side) to set a connection error handler on it. I don't see how that would affect the results.

I noticed that in yhirano's change, the URLLoader binding in MojoAsyncResourceHandler uses the default task runner, which is ThreadTaskRunnerHandler::Get(). I thought maybe it should be set to ResourceDispatcherHostImpl::Get()->io_thread_task_runner(), because surprisingly the two are different.

But reading the code further, I think using ThreadTaskRunnerHandler::Get() is a better choice to avoid making a PostTask in mojo::SimpleWatcher.

Please let me know if anyone could reproduce this perf regression.

I tried running this locally on a Nexus 7 (I think v2?) and get the 10 second timeout result for all the concurrent_100_response_*_percentiles.

I built on ToT (without the patch https://chromium.googlesource.com/chromium/src/+/f510d0c432ea0330c394e4a1349d6e7ee31c116e)

And ran with:
tools/perf/run_benchmark service_worker.service_worker_micro_benchmark --browser=android-chromium

My build args are:
target_os = "android"
target_cpu = "arm"
is_debug = false
is_component_build = true
is_clang = true

I get:
concurrent_1_response_90_percentile: 14ms
concurrent_10_response_90_percentile: 60ms
concurrent_100_response_90_percentile: 10 seconds

It's evident that concurrent_100 is broken on the bots and locally.

But IIUC, this bug is about the regression in concurrent_1 when https://chromium.googlesource.com/chromium/src/+/f510d0c432ea0330c394e4a1349d6e7ee31c116e is applied. I will try to patch that in tomorrow. Does anyone have a patch ready that applies on ToT?

#28
Thanks for looking into it!

FWIW, I didn't use ToT for investigation, instead, I did the investigation using the same base commit as https://chromium.googlesource.com/chromium/src/+/f510d0c432ea0330c394e4a1349d6e7ee31c116e

I thought that might be less noisy considering many changes have happened since then. (But I still didn't repro the issue. )

Hm, I tried building from that base commit and couldn't view the perf benchmark results:
Uncaught (in promise) Error: Unrecognized unit
    at Function.Unit.fromJSON (results.html:13400)
    at Function.fromDict (results.html:17337)
    at HistogramSet.importDicts (results.html:18643)
    at HistogramImporter.loadSomeHistograms_ (results.html:18942)
    at HistogramImporter.importHistograms (results.html:18913)
    at <anonymous>

Can yhirano or yzshen make a patch on ToT? I started resolving conflicts but not sure I'm doing it right as it looks like a lot of code changed.

Thanks for looking, falken@!

Here is a CL rebased to Tot: https://codereview.chromium.org/2954853002/
(Still trying on the bots. There might be compilation errors that I have missed and need to fix.)

Talked with falken@.

I'm closing this bug as WontFix. I will reland the CL after the next branch is cut. Thank you for investigating the issue.