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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgjs_ZpgsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgjuiEowsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg9ojLmAsM


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

android-nexus5X
android-webview-nexus5X
android-webview-nexus6

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

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

Hi davidben@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 : David Benjamin
  Commit : af16f31649f58f6d6f99861da3cdbfaa102796e3
  Date   : Mon Jun 19 16:57:19 2017
  Subject: Don't take a mutex around certificate verifications on Android.

Bisect Details
  Configuration: android_webview_arm64_aosp_perf_bisect
  Benchmark    : memory.top_10_mobile
  Metric       : memory:webview:all_processes:reported_by_os:system_memory:java_heap:proportional_resident_size_avg/foreground/http_www_amazon_com_gp_aw_s_k_nexus
  Change       : 52.06% | 3237668.57143 -> 4923245.71429

Revision             Result                  N
chromium@480455      3237669 +- 420574       14      good
chromium@480470      3315078 +- 2231169      21      good
chromium@480471      3244203 +- 265002       6       good
chromium@480472      5433685 +- 41409.8      6       bad       <--
chromium@480473      5170290 +- 2291186      9       bad
chromium@480476      5432320 +- 37735.5      6       bad
chromium@480482      4923246 +- 3687087      14      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

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

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

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


| 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!

Hrm. I would not have expected that to increase memory. Will take a look.

I notice the benchmark says http_www_amazon_com_gp_aw_s_k_nexus and not https_something. That seems odd. This change would affect https loads.

Also, what kind of environment do these test run in? In particular, do they install test root certs? That (test-only) mechanism could increase in memory after the change, but as it's never used in production code. What are the units of the 3237668.57143 -> 4923245.71429 number? Is that bytes?

> do they install test root certs

Using the net::TestRootCerts class, that is. Doing it via the OS should not be affected by that change.

+Ned, Helen: can you answer the questions in #4 and #5?

Ah, found the units from the graph. It's in bytes.
https://chromeperf.appspot.com/group_report?bug_id=735659

And wow that is quite a visible spike! :-) I'll play with the instructions to try manually repro this.

We currently don't install test root certs yet. We bypass HTTPS by using "--ignore-certificate-error" flag

The java heap increased. Maybe the CL introduced Java references that are not cleaned up after the test finishes.

To view the trace file from dashboard, you can click on any point on the graph. There is a "trace" link in the box UI. The units are in bytes. I looked at the before and after trace files briefly, I didn't find anything useful. You can use HPROF (https://developer.android.com/studio/profile/android-monitor.html) to see what Java objects are left over.

I'm having loads of trouble reproducing this locally, so I'm just going to speculatively revert and we'll see if that fixed it. If so, go from there...

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

commit 11464f93e041d5b6ae47e2f8dfb2765ab727d7e8
Author: David Benjamin <davidben@chromium.org>
Date: Thu Jun 22 22:14:37 2017

Revert "Don't take a mutex around certificate verifications on Android."

This reverts commit af16f31649f58f6d6f99861da3cdbfaa102796e3.

Reason for revert: Speculative revert to see if it fixes crbug/735659.

Original change's description:
> Don't take a mutex around certificate verifications on Android.
> 
> We use a worker pool to verify certificates, but then undo it all in
> Android by locking around the entire operation. However, Android's
> X509TrustManager implementations are already thread-safe. We do some
> work on top which must be synchronized, mostly around reloading various
> caches.
> 
> Instead, lock only around acquiring the current instance of the caches.
> When we need to invalidate, we drop the pointer. Any pending tasks
> will continue using the old state (as they would have before this
> change as they'd be holding the lock), but all verifications after the
> invalidation will use a fresh set of state.
> 
> A completely unscientific (I checked about:histograms after startup)
> suggest that this speeds up the mean startup cert verification time
> by about 40%.
> 
> Bug: 
> Change-Id: I3cddb798f3dfd51a2f1d2e529127c39f521e93de
> Reviewed-on: https://chromium-review.googlesource.com/531973
> Reviewed-by: Matt Mueller <mattm@chromium.org>
> Commit-Queue: David Benjamin <davidben@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#480472}

TBR=davidben@chromium.org,mattm@chromium.org

Bug:  735659 
Change-Id: Ib84eb5b0a03d674565a075ad1003828904aebe24
Reviewed-on: https://chromium-review.googlesource.com/545057
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481689}
[modify] https://crrev.com/11464f93e041d5b6ae47e2f8dfb2765ab727d7e8/net/android/java/src/org/chromium/net/X509Util.java

Aaand that brought it back down, so that does seem to have been the cause. :-/

My attempts to reproduce locally this failed miserably, and the alert's tracing is lacking to say the least, so it seems this tooling just isn't usable yet. This was meant to be a free perf win on Android and I do not have time right now to chase it down, so we'll just have to live with that mutex until the infrastructure improves or someone else feels more motivated to resurrect the patch.