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

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


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

chromium-rel-win7-dual

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

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

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

=== PERF REGRESSION ===


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

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


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : ResourceLoader:  Fix a bunch of double-cancellation/double-error notification cases.
Author  : mmenke
Commit description:
  
There are a number of cases where ResourceLoader ends up notifying ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG= 669709 , 670400 

Review-Url: https://codereview.chromium.org/2542843006
Cr-Commit-Position: refs/heads/master@{#437057}
Commit  : 94f1bd9459e17972e8e3f6f51a3d10fbf4a94df7
Date    : Wed Dec 07 21:17:39 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev    N  Good?
chromium@436997  4.21417  0.0947145  6  good
chromium@437035  4.1875   0.122219   6  good
chromium@437054  4.2      0.0966954  6  good
chromium@437056  4.18583  0.263194   6  good
chromium@437057  4.55333  0.169804   6  bad    <--
chromium@437059  4.51333  0.131085   6  bad
chromium@437064  4.45083  0.0708578  6  bad
chromium@437073  4.51583  0.20167    6  bad

Bisect job ran on: win_x64_perf_bisect
Bug ID: 673625

Test Command: 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
Test Metric: concurrent_10_response_90_percentile/concurrent_10_response_90_percentile
Relative Change: 7.16%

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_x64_perf_bisect/builds/1553
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8993424241870248176


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5218374777307136

| 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 Tests>AutoBisect.  Thank you!

===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : ResourceLoader:  Fix a bunch of double-cancellation/double-error notification cases.
Author  : mmenke
Commit description:
  
There are a number of cases where ResourceLoader ends up notifying ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG= 669709 , 670400 

Review-Url: https://codereview.chromium.org/2542843006
Cr-Commit-Position: refs/heads/master@{#437057}
Commit  : 94f1bd9459e17972e8e3f6f51a3d10fbf4a94df7
Date    : Wed Dec 07 21:17:39 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev    N  Good?
chromium@437045  4.30083  0.161774   6  good
chromium@437053  4.24667  0.0378594  6  good
chromium@437055  4.2675   0.044017   6  good
chromium@437056  4.205    0.176352   6  good
chromium@437057  4.52     0.104642   6  bad    <--
chromium@437061  4.55167  0.121997   6  bad
chromium@437076  4.57333  0.128776   6  bad
chromium@437106  4.51417  0.132366   6  bad

Bisect job ran on: win_perf_bisect
Bug ID: 673625

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests service_worker.service_worker_micro_benchmark
Test Metric: concurrent_10_response_90_percentile/concurrent_10_response_90_percentile
Relative Change: 4.96%

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/7072
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8993424365803942336


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5037628460630016

| 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 Tests>AutoBisect.  Thank you!

===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : ResourceLoader:  Fix a bunch of double-cancellation/double-error notification cases.
Author  : mmenke
Commit description:
  
There are a number of cases where ResourceLoader ends up notifying ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG= 669709 , 670400 

Review-Url: https://codereview.chromium.org/2542843006
Cr-Commit-Position: refs/heads/master@{#437057}
Commit  : 94f1bd9459e17972e8e3f6f51a3d10fbf4a94df7
Date    : Wed Dec 07 21:17:39 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev    N   Good?
chromium@437045  4.28583  0.173121   6   good
chromium@437053  4.25611  0.0699206  9   good
chromium@437055  4.31679  0.783218   14  good
chromium@437056  4.225    0.111803   6   good
chromium@437057  4.54667  0.107238   9   bad    <--
chromium@437061  4.50917  0.215339   6   bad
chromium@437076  4.56333  0.189693   6   bad
chromium@437106  4.53167  0.0947804  6   bad

Bisect job ran on: win_perf_bisect
Bug ID: 673625

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests service_worker.service_worker_micro_benchmark
Test Metric: concurrent_10_response_90_percentile/concurrent_10_response_90_percentile
Relative Change: 5.74%

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/7073
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8993424228205637072


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5862819990339584

| 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 Tests>AutoBisect.  Thank you!

I may not get to this until January.  I do think it's worth investigating, but because of the magnitude of the regression, I don't think that's a problem, unless there's an indication there's a bug somewhere.  Wouldn't surprise me if there was an extra PostTask in some non-cancellation/error case.  There is one in some error cases, I believe, but I don't think the benchmark would hit that.

Investigation is appreciated. This is just a microbenchmark that's really sensitive to any code change, so given the regression magnitude I agree it's likely not a problem.

Note that this is still on my radar, still doing work here, wanted to get it done and then look at perf of this test, see if it's still regressed, and figure out why.  I'm on break next month, but I do intent to look at this when I get back, honest.  :)

 Issue 687743  has been merged into this issue.

[+rdsmith, +csharrison]:  FYI (And with the second CL, there was another, larger regression in this test).  I'll spend some time digging before I'm off, but probably won't have time to make a whole lot of progress until I get back.

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

commit 9f855ec8d57996af189f479bbb5f37a378921250
Author: mmenke <mmenke@chromium.org>
Date: Thu Feb 02 22:42:22 2017

Remove ResourceLoader PostTask after reading headers.

https://codereview.chromium.org/2526983002 incidentally introduced a
PostTask when ResourceHandler::OnResponseStarted completed
synchronously, and the following body read did as well. This may have
resulted in a regression in some ServiceWorker perf tests.

In general, it's unclear how aggressively we should be posting tasks
while reading the body, this CL is just to restore the earlier behavior,
and (hopefully) make the tests happy again.

BUG= 673625 

Review-Url: https://codereview.chromium.org/2669243006
Cr-Commit-Position: refs/heads/master@{#447863}

[modify] https://crrev.com/9f855ec8d57996af189f479bbb5f37a378921250/content/browser/loader/resource_loader.cc

We only have 3 data points since the patch in #14 landed, but it looks likely to have fixed the second regression.

Still looking at the first, but it looks like we're tearing down requests a little earlier, so we delete more requests before other requests are issued.  So it's possible that requests are being delayed by those earlier destructor calls, though I'm not confident that's the cause rather than a symptom.  I'm also not sure why we're calling the destructors earlier.

Think this may just be WontFix - I'm having trouble figuring out how things are different, other than that we don't second-notification PostTasks on sync eof reads (Or cancellation / failure).  On success, those posted tasks don't even do anything.

We also don't call ReadMore synchronously on cancellation any more, which makes teardown async in that case instead of sync, but I'm not seeing that happen at all when running tests locally - none of the requests are canceled.

Just printing out creation/destruction order of ResourceLoaders, the pattern is the same before/after the patch, until we get to around request 195, which also serves to make digging into this a bit more difficult.

Going to keep this bug open until I get back in a month, and re-evaluate things there, but I am leaning strongly towards just closing it, assuming the more recent regression is as fixed as it seems.

Going to mark this WontFix - don't think it's worth further investigation.  Not sure what's going on.  The second regression has indeed been fixed (Though looks like that other benchmark has regressed against in the last month).