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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4vnQ9QkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4qL0wQkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4tWktwoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4uXA_gkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4pWNqgsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgkrSy4wgM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4sqVoQkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4syP8wgM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgkrKgvgkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4rvK6QgM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4qGRhAkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4vum8ggM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgktKWugkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4uyepAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4qOiuQsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4vbN5gsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4rmnlAkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4u3mvQoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4rGvoQoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg4rn2sQsM


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

android-nexus5
android-nexus7v2
android-one
android-webview-nexus6

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

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

Hi benwells@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 : benwells
  Commit : 2ae9cf3f8288419c4985e4543bcd07ebf65bfd21
  Date   : Sat Apr 22 19:26:25 2017
  Subject: Don't enable the DSE geolocation setting until EULA accepted

Bisect Details
  Configuration: android_one_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:chrome:all_processes:reported_by_chrome:skia:effective_size_avg/browse_tools/browse_tools_maps
  Change       : 2610.15% | 171770.666667 -> 4655247.0

Revision             Result                  N
chromium@466538      171771 +- 10016.7       6      good
chromium@466546      172124 +- 10393.3       6      good
chromium@466547      171771 +- 10016.7       6      good
chromium@466548      4658756 +- 13594.8      6      bad       <--
chromium@466550      4659391 +- 16110.4      6      bad
chromium@466553      4655247 +- 10211.9      6      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-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.tools.maps system_health.memory_mobile

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

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


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

 Issue 714912  has been merged into this issue.

Seems unlikely that this is a regression. Instead, I think this change is affecting the behavior of the maps test case.

+sullivan, nednguyen to take a look

Ben, do you know how this change might have affected the behavior of a test case that loads maps?

No ... I can't think of anything. Are the maps hosted by google?

FWIW looking at two of the graphs this regression seems to exactly undo an earlier progression. Maybe understanding the progression will help understand what got worse?

Graph 1: https://chromeperf.appspot.com/report?sid=00eb3dcfde3a63b3464559044c08cbe24a3fa33ff4db6c0038cd961882b3adc9&start_rev=463433&end_rev=466554
Graph 2: https://chromeperf.appspot.com/report?sid=c6a0734c601edc3974d4dc0a3ad16d6189c27452215e876df7ae78b3f9132b61&rev=466806

Punting to get answers to questions in #7

ping, apparently sullivan@ is away, maybe someone else can give some guidance? I don't have much idea about what to do with this.

The maps are recorded using WPR tool (https://github.com/chromium/web-page-replay). You can look at the trace to see which memory component regressed exactly:

Before:
https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_16-2017-04-21_16-40-01-41773.html

After:
https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_16-2017-04-23_03-05-13-48535.html

Guide for understanding memory trace: https://chromium.googlesource.com/chromium/src/+/14aff5d/docs/memory-infra/README.md

I also suggest trying to revert you CL locally and run system_health.memory_mobile locally to see if you can produce the memory improvement (See go/telemetry-device-setup for how to set up your test phone).

OK. Sorry I have been travelling and was unable to run these things locally. The change isn't strictly needed anymore, so I am landing a manual revert to see if it makes the perf any better.

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

commit af4727a9a29c33197e12a7284b9ecad63422b081
Author: benwells <benwells@chromium.org>
Date: Fri May 19 20:34:59 2017

Manually revert only showing search geo disclosure after EULA accepted

Now that the disclosure is only shown when the X-Geo header is sent,
or a page uses the geolocation API, this isn't needed.

The change which prevented the disclosure being shown until after EULA
was accepted also apparently caused a perf regression. It's hard to
see how it would have, but removingt the change will determine if
it really was that change or something else.

BUG= 714863 

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

[modify] https://crrev.com/af4727a9a29c33197e12a7284b9ecad63422b081/chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java
[modify] https://crrev.com/af4727a9a29c33197e12a7284b9ecad63422b081/chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java
[modify] https://crrev.com/af4727a9a29c33197e12a7284b9ecad63422b081/chrome/browser/android/search_geolocation/search_geolocation_service.cc

nednguyen: Can you provide more details about what the memory benchmark is actually doing? In particular, what web sites is it loading?

I've landed a revert. It seems to have fixed the memory regression but not the other ones.

The two regressions it didn't fix look to be mis-attributions to my change. I think the regressions were each undoing a previous progression. If you look in the graphs that is clear.

The memory regression that was caused by my change is probably due to the website acting differently.

What would be really useful is seeing what the tests actually do. All I know is the test is called "browse_tools_maps" but I can't see any more detail than that. Most importantly for my change is what origins were being browsed.

I'd prefer to reland my change as the behavior with it is safer, but before doing so I'd like to make sure the regression isn't an actual problem.

Hi Ben, 
The list of websites it's loading can be found it bit.ly/csh-stories. Other memory regression may come from other CLs and accidentally group into the stories affected by your CL.

OK. It makes sense that the change I'd made (and have since reverted) would change things. After some thought I'll leave the change reverted. Due to other fixes it isn't really needed, and it's probably cleaner to leave it out.