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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5L-lsQkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5OXytQkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5L_qvgoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5LLZoAkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICglLDHvAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICglLC2pQoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5PSMugoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5NKGowoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICglPDqqwoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICglJTyvwoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5Jb0sgoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5MGyvwoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg5JKUoQsM


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

android-nexus5X
android-nexus6

This regression can be seen on the test builds but not the reference builds, and is reproduced in different tests and on two different types of devices.

I'm very grateful for reference builds on android :-)

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

Hi agrieve@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


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


===== SUSPECTED CL(s) =====
Subject : Check for 0-length files in ResourceExtractor
Author  : agrieve
Commit description:
  
Also moves from an on-disk version timestamp file to SharedPreferences
(should be faster).

BUG= 606413 

Review URL: https://codereview.chromium.org/1920893003

Cr-Commit-Position: refs/heads/master@{#390068}
Commit  : b28607d4c72261657ea9e24ea9195bc23a472b3f
Date    : Wed Apr 27 14:44:16 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@390065  24043.2  94.9695  5  good
chromium@390067  24022.4  54.0074  5  good
chromium@390068  24953.6  69.5471  5  bad    <--
chromium@390070  24982.4  71.1393  5  bad
chromium@390075  24983.2  51.1781  5  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 607598

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests indexeddb_perf
Test Metric: vm_private_dirty_final_browser/vm_private_dirty_final_browser
Relative Change: 3.91%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2150
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9014119268453850096


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

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

Hi agrieve, does it make sense that that change could have affected memory usage on android after page loads?

Also, do you know where can we look to see if that change made some metrics faster?

Might it be OK to revert that CL to see verify whether the vm_private_dirty_final_browser numbers return to previous levels?

What's the delta here? 900kb? It's unlikely this change could increase memory usage. 

I wonder if we're extracting the resource file more often in the test environment? This would be a temporary hit and it's typically once per chrome version but perhaps because of the change for local builds this now happens eveyr time on the bots?

Sorry about the silent treatment. Finally figured out that all bug mail has been going to my spam :(.

I don't see why the change would make the file be extracted more frequently. However, there are two other significant tidbits:
1. Adds a call to SharedPreferences when there wasn't one before
2. When versionCode=1, calls: new ZipFile(mContext.getApplicationInfo().sourceDir)

I'd guess we don't bother setting a versionCode in our test environment, so that's my bet. Does this metric indicate that there's a memory leak here, or just that peak usage went up?

As for reverting: If there's any doubt that this is the culprit CL, I'm all for reverting & relanding. There should be no risk in it (should revert cleanly, code isn't in any hurry to ship)

 Issue 607599  has been merged into this issue.

As far as I understand, this metric doesn't necessarily mean a memory leak, or that maximum memory usage increased. According to https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/metrics/memory.py&l=219, it's the amount of memory that cannot be paged to disk or shared with other processes. In this case only the memory for the browser process increased, not any renderer process.

I'm not clear about how these things could affect that metric, but if we revert we can confirm whether there is an effect -- and then, ideally, we can use perf try jobs to experiment and reland with some change that will accomplish the same goal but without any performance regressions.

Speculative revert CL: https://codereview.chromium.org/1948033004/

Looks like the graphs have recovered, but I don't see the revert commit in the blamelist for the recovered point on the graph.

qyearsley - could you sanity check this for me? Should I do a speculative reland?

Just checked a couple of the graphs, and it looks like the range for the recovered point does include the revert commit; the revert was r393023; an in these few graphs the improvement point includes 393019 - 393039 :

https://chromeperf.appspot.com/report?sid=b298b60c7b289527dbf677bc5ab784cb1749ea77e36b96e2f6414592aa8c37ee&start_rev=389477&end_rev=393762

https://chromeperf.appspot.com/report?sid=71ea2172a29d75495152fa2a67719cca869f5875133d857cc3acd3bb65871b06&start_rev=387270&end_rev=394725

https://chromeperf.appspot.com/report?sid=b298b60c7b289527dbf677bc5ab784cb1749ea77e36b96e2f6414592aa8c37ee&start_rev=389477&end_rev=393762

So this looks like pretty good evidence that it recovered because of that revert, I believe.

So, if you reland it, then we should expect to get more performance alerts for about a ~2.5% change in vm_private_dirty_final_browser.

But, it may be easier to use performance try jobs to verify and test perf effects of a change (see https://www.chromium.org/developers/telemetry/performance-try-bots)

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

commit f17984d06c7a736aa2cf7aac3d8aa6dc2628ef9a
Author: agrieve <agrieve@chromium.org>
Date: Thu May 19 21:17:06 2016

Reland of Check for 0-length files in ResourceExtractor

Reason for reland:
No longer using ZipFile. Hopefully this was the reason
for the dirty memory.

TBR=yfriedman@chromium.org,torne@chromium.org,wnwen@chromium.org,qyearsley@chromium.org
BUG= 606413 , 607598 

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

[modify] https://crrev.com/f17984d06c7a736aa2cf7aac3d8aa6dc2628ef9a/base/android/java/src/org/chromium/base/ResourceExtractor.java

Whoops! Tryjobs would have been much nicer. Sorry for missing that. :(

Just spot checked the 3 graphs from #11 and 2/3 include the reland with no regression, so I think we're in the clear!

Excellent :-) Thanks!