Issue metadata
Sign in to add a comment
|
1.3%-9.6% regression in memory.top_10_mobile at 471896:471943 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
May 16 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8979426804332057216
,
May 18 2017
=== Auto-CCing suspected CL author fgorski@chromium.org === Hi fgorski@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 : fgorski Commit : 7c52dcabdaefff4b583999c786afbefa907564fb Date : Mon May 15 22:15:20 2017 Subject: [Android] Adding scheduling through GcmNetworkManager 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/background/after_http_en_m_wikipedia_org_wiki_Science Change : 9.97% | 1438208.0 -> 1581568.0 Revision Result N chromium@471895 1438208 +- 13410.1 6 good chromium@471912 1444352 +- 4031.49 6 good chromium@471916 1477291 +- 3739.12 6 good chromium@471918 1444352 +- 4031.49 6 good chromium@471919 1585664 +- 5369.9 6 bad <-- chromium@471920 1592661 +- 40771.8 6 bad chromium@471928 1581568 +- 5369.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-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/8979426804332057216 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=4655855636578304 | 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!
,
May 18 2017
Issue 723017 has been merged into this issue.
,
May 18 2017
Issue 723018 has been merged into this issue.
,
May 19 2017
Issue 724033 has been merged into this issue.
,
May 19 2017
This looks weird. The code that was added is not yet invoked. And even when it will get invoked it would happen very rarely. What OS the webview bots in question running?
,
May 25 2017
Specific OS? The main perf bots are running M. Bisectors also M (you can see on the device_status step)
,
May 25 2017
Great.
Most of the code does not even run then, because I am adding parts that are every used in pre-M (else clause below):
private static BackgroundTaskSchedulerDelegate getSchedulerDelegate() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
return new BackgroundTaskSchedulerJobService();
} else {
return new BackgroundTaskSchedulerGcmNetworkManager();
}
}
The only change that is affecting Chrome memory in run-time would perhaps be adding an extra GCMTaskService, but that was not added to manifest until later:
https://chromium.googlesource.com/chromium/src/+/df49c6da32ca2a0ed87e1d5f87f4032d9c4fdc12
I also don't understand how this would affect webview.
,
May 25 2017
Comment repeated from issue 724087 This patch is first in a cycle of refactorings that attempt to centralize background scheduling. Because most of the newly added code is not even run (with exception of storing information in shared preferences), there is no more impact on Heap then class metadata (4 classes) and a few static variables: BackgroundTaskGcmTaskService private static final String TAG = "BkgrdTaskGcmTS"; BackgroundTaskGcmTaskService::Waiter private static final long MAX_TIMEOUT_SECONDS = 179; BackgroundTaskSchedulerGcmNetworkManager private static final String TAG = "BkgrdTaskSchedGcmNM"; static final String BACKGROUND_TASK_CLASS_KEY = "_background_task_class"; static final String BACKGROUND_TASK_EXTRAS_KEY = "_background_task_extras"; BackgroundTaskSchedulerPrefs private static final String TAG = "BTSPrefs"; private static final String KEY_SCHEDULED_TASKS = "bts_scheduled_tasks"; private static final String KEY_LAST_OS_VERSION = "bts_last_os_version"; private static final String KEY_LAST_APP_VERSION = "bts_last_app_version"; private static final String ENTRY_SEPARATOR = ":"; I suspect (don't know enough about Perm Gen allocation) that if Heap is growing in blocks, this CL caused a new block of memory to be added to what Chrome was already using ( issue 722858 ). Same thing probably happened to apk sizes (issue 724087). The following CL is the first one that takes advantage of refactoring, and therefore will hopefully start paying for it: https://codereview.chromium.org/2830843002/ It's delta is at least 5 classes cut, and a lot of duplicated functionality removed. I recommend closing this bug as a won't fix. Thoughts?
,
May 25 2017
,
May 26 2017
So, could we justify this as an intermediate state of some refactoring that's going to reap some benefits down the line? If so, that's ok with me, but what is the ETA on the full set of patches?
,
May 26 2017
The code adding possibility to schedule tasks using GCMNetworkManager (on pre-M versions of Android) has already been landed in 4 patches: https://codereview.chromium.org/2779753002/ <-- original offending patch https://codereview.chromium.org/2814653003/ https://codereview.chromium.org/2825783003/ https://codereview.chromium.org/2819703002/ The next patch mentioned in #10 is going to be the first one taking advantage of the common code: https://codereview.chromium.org/2830843002/
,
May 26 2017
Adding Tommy to this bug as well, as he is on board with the current plan as well, per https://bugs.chromium.org/p/chromium/issues/detail?id=724087#c8
,
May 27 2017
Issue 726888 has been merged into this issue.
,
Jun 1 2017
Revision 476352 is expected to have positive impact on the problem. I'll be following up on this.
,
Aug 17 2017
fgorski: is this fixed?
,
Aug 17 2017
We have yet to move various components to background_task_scheduler. There is not going to be an explicit fix for this issue however, as there is really nothing to fix.
,
Sep 19 2017
,
Jan 5 2018
I actually see more memory usage in r476352, but overall the regressions from this issue are less than 1MiB and per #18 it doesn't have an explicit fix, so marking WontFix. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by lanwei@chromium.org
, May 16 2017