Issue metadata
Sign in to add a comment
|
1.5%-11.6% regression in memory.top_10_mobile at 424384:424405 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Oct 12 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8999004657274839040
,
Oct 12 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8999004588725860768
,
Oct 12 2016
Bisect failed: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_one_perf_bisect/builds/1710 Failure reason: the build has failed due to infrastructure failure.
,
Oct 13 2016
=== Auto-CCing suspected CL author heimbuef@google.com === Hi heimbuef@google.com, 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 : Pool implementation for zone segments Author : heimbuef Commit description: BUG=v8:5409 Committed: https://crrev.com/37c688a24578e787d3d8941093563ed049c3497e Committed: https://crrev.com/316669f62ea3834395bf4caab7bc3d7c32f6bbc6 Review-Url: https://codereview.chromium.org/2335343007 Cr-Original-Original-Commit-Position: refs/heads/master@{#39631} Cr-Original-Commit-Position: refs/heads/master@{#40044} Cr-Commit-Position: refs/heads/master@{#40137} Commit : f29f3e320831f7eec8703b083dbbb91d88c65dca Date : Mon Oct 10 18:03:55 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@424389 33889884 756147 8 good chromium@424395 33956956 533586 8 good chromium@424398 33954396 461075 8 good chromium@424398,v8@b884a51ff2 33522882 353974 5 good chromium@424398,v8@f29f3e3208 35411138 604751 5 bad <-- chromium@424398,v8@a96c2129af 35340687 106078 5 bad chromium@424398,v8@085a445775 35331676 290340 5 bad chromium@424398,v8@6e95a8f0a6 35437353 190528 5 bad chromium@424398,v8@127798897f 35493058 375478 5 bad chromium@424399 35271772 340422 8 bad chromium@424400 35321026 757581 5 bad chromium@424401 35528284 872944 8 bad Bisect job ran on: android_one_perf_bisect Bug ID: 655129 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests memory.top_10_mobile Test Metric: memory:chrome:all_processes:reported_by_os:system_memory:private_dirty_size_avg/foreground/http_yandex_ru_touchsearch_text_science Relative Change: 4.44% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_one_perf_bisect/builds/1711 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8999004588725860768 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5896465736531968 | 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!
,
Oct 18 2016
heimbuef, your CL appears to have caused some large regressions on Android. You should be able to reproduce by running locally with a N5 device: Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=html --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.media.youtube system_health.memory_mobile Or on a trybot: tools/perf/run_benchmark try android-nexus5 --story-filter=load.media.youtube system_health.memory_mobile And look for the metric: memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_media/load_media_youtube
,
Oct 18 2016
Yes, working on a fix for it. My plan is to constrain the zone segment pool size on devices with low memory. That way it wont improve performance there anymore, but also not hit memory so badly.
,
Oct 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/ef690ca387c116569a2f5dbd71c6f0733a070cc6 commit ef690ca387c116569a2f5dbd71c6f0733a070cc6 Author: heimbuef <heimbuef@google.com> Date: Thu Oct 20 14:48:13 2016 Constrain the zone segment pool size Added a size constraint to the configuration to limit the segment pool. This will likely fix the memory alerts from small android devices. BUG= chromium:655129 Review-Url: https://chromiumcodereview.appspot.com/2424393002 Cr-Commit-Position: refs/heads/master@{#40476} [modify] https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6/include/v8.h [modify] https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6/src/api.cc [modify] https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6/src/zone/accounting-allocator.cc [modify] https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6/src/zone/accounting-allocator.h [modify] https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6/test/unittests/BUILD.gn [modify] https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6/test/unittests/unittests.gyp [add] https://crrev.com/ef690ca387c116569a2f5dbd71c6f0733a070cc6/test/unittests/zone/segmentpool-unittest.cc
,
Oct 31 2016
CL in #8 rolled into chromium at r426611, but there are no significant memory changes around that time, see e.g.: https://chrome-health.googleplex.com/health-plan/android-chrome/memory/nexus5/?from_commit=426000&to_commit=426999 Note that the regression appears on all device types, not only low end devices, and a regression this large is quite likely to become a release blocker.
,
Oct 31 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8997280178046186160
,
Oct 31 2016
,
Oct 31 2016
Adding Ben Henry as an FYI
,
Oct 31 2016
Please ignore #11, that was a bisect for issue 655711 that ended up in the wrong bug.
,
Nov 1 2016
What's the plan to address this since the fix didn't actually address the memory regression? FYI - 56 Beta is targeted for the first week of December, so we have a couple weeks to resolve this. What's stopping a revert of the offending CL?
,
Nov 1 2016
It's intentional that the CL increases memory consumption; it therefore increases the performance of tasks that very frequently create new small zones like parsing and optimizing. The amount of memory consumed by the pool can be adjusted using the ResourceConstraints.
,
Nov 1 2016
heimbuef@, can you link a benchmark that improved due to this change?
,
Nov 2 2016
+Speed Leads
,
Nov 3 2016
Take a look at go/v8-startup. At 2016-10-06T18:04:49.000Z it was first introduced, just to get reverted at 2016-10-08T00:26:17.000Z again due to a compiler error with Chromium. At 2016-10-10T21:14:31.000Z it got reintroduced. For some pages it is harder, but with most it is easy to see the initial speedup, the regression on the first revert and the again increased performance. Instagram and Wikipedia are good examples for that.
,
Nov 11 2016
+Toon Friendly perf-sheriff ping. What is the status here. From looking at the graphs this seems to be giving <3% improvement on a couple of pages (e.g Instagram or Wikipedia where I looked) in V8 startup (so it would be an even lower improvement on overall page startup) but is trading this off for a ~3-5% regression in memory usage on Android. Personally this doesn't seem like a good trade-off, but I might not have the necessary background to make that call.
,
Nov 15 2016
I'm going to aggressively lower the limit to 8kb on all devices. I presume that most perf benefits come from many compilations of small functions anyway, rather than compilation of large scripts. Those small jobs should still benefit from a small "pool". We could tweak it later to be larger up to an acceptable limit; like 64kb or so if it still seems beneficial. In the long run this should possibly be replaced by PartitionAlloc altogether.
,
Nov 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/c3a60837c075dd7809a08f07125466bf4c46a77e commit c3a60837c075dd7809a08f07125466bf4c46a77e Author: verwaest <verwaest@chromium.org> Date: Tue Nov 15 17:28:30 2016 Reduce zone segment pool size on all devices to 8kb BUG= chromium:655129 Review-Url: https://codereview.chromium.org/2504673002 Cr-Commit-Position: refs/heads/master@{#41005} [modify] https://crrev.com/c3a60837c075dd7809a08f07125466bf4c46a77e/src/zone/accounting-allocator.h
,
Nov 16 2016
#22 rolled into chrome at r432283. Will keep an eye to see the effects it has on memory.
,
Nov 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/c3a60837c075dd7809a08f07125466bf4c46a77e commit c3a60837c075dd7809a08f07125466bf4c46a77e Author: verwaest <verwaest@chromium.org> Date: Tue Nov 15 17:28:30 2016 Reduce zone segment pool size on all devices to 8kb BUG= chromium:655129 Review-Url: https://codereview.chromium.org/2504673002 Cr-Commit-Position: refs/heads/master@{#41005} [modify] https://crrev.com/c3a60837c075dd7809a08f07125466bf4c46a77e/src/zone/accounting-allocator.h
,
Nov 18 2016
,
Nov 18 2016
There is a big improvement, but this still creates a ~1MB regression: https://chromeperf.appspot.com/report?sid=6e57649773b97d360f90a53c1ae98133cf4af766bb04b42d954d7bba3d461231&start_rev=420855&end_rev=433024 Is this expected? With the talk of an 8kb segment pool I was primed to see almost no regression here at all :(
,
Nov 18 2016
Does that regression come from this issue? I don't expect it given that the limit is indeed 8kb now. There could be a bug of course.
,
Nov 21 2016
When #5 landed (at r424398) we saw: Chrome Phone: +1.6MB pss, native heap, private dirty https://chrome-health.googleplex.com/health-plan/android-chrome/memory/nexus5/?from_commit=424122&to_commit=424509 WebView Phone: +1.6MB pss, native heap, private dirty https://chrome-health.googleplex.com/health-plan/android-webview/memory/nexus5/?from_commit=424104&to_commit=424664 Chrome Low End Phone: +1.4MB pss, native heap, private dirty (foreground only) https://chrome-health.googleplex.com/health-plan/android-chrome/memory/nexus4-svelte/?from_commit=424116&to_commit=424707 When #22 landed (at r432283) we saw: Chrome Phone: -0.5MB native heap, private dirty. No change in pss https://chrome-health.googleplex.com/health-plan/android-chrome/memory/nexus5/?from_commit=431793&to_commit=433418 WebView Phone: -1MB pss, native heap, private dirty https://chrome-health.googleplex.com/health-plan/android-webview/memory/nexus5/?from_commit=431793&to_commit=433418 Chrome Low End Phone: No significant changes https://chrome-health.googleplex.com/health-plan/android-chrome/memory/nexus4-svelte/?from_commit=431793&to_commit=433418 This still leaves us with: - Some +1MB - +1.5MB regressions in Chrome Phone - Some +0.5MB regressions in WebView Phone - Some +1.5MB regressions in Chrome Low End Phone Particularly on Chrome, this is looking like a release-blocking regression.
,
Nov 21 2016
This fix was in #24?
,
Nov 21 2016
Ah that's the same for whatever reason. Whatever :) Still, I don't see how there would be a >=0.5mb regression from an 8kb cache. Are you certain this doesn't come from another intermediate regression?
,
Nov 21 2016
How disruptive would it be to revert the original CL and watch the bots for a few iterations to see if the entire regression disappears? It would be nice to know if we are looking at a single CL being responsible or a multiple CLs that are hiding behind each other, confusing the bisect.
,
Nov 21 2016
Reverting is very tricky. Instead I'm going to verify whether the size limiting mechanism that was implemented actually works ...
,
Nov 21 2016
Doh! The size limiting mechanism is indeed broken. It did indeed keep the cache at ~0.5mb. I'll properly limit it to 8kb...
,
Nov 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/4097c8503e4c033713e4b07ee01072896cbfd381 commit 4097c8503e4c033713e4b07ee01072896cbfd381 Author: verwaest <verwaest@chromium.org> Date: Mon Nov 21 13:21:50 2016 [zone] Fix zone segment pooling size limits BUG= chromium:655129 Review-Url: https://codereview.chromium.org/2520903002 Cr-Commit-Position: refs/heads/master@{#41138} [modify] https://crrev.com/4097c8503e4c033713e4b07ee01072896cbfd381/src/zone/accounting-allocator.cc
,
Nov 21 2016
#33 is fantastic news! Thanks for taking the time to check. Fingers crossed for your fix.
,
Nov 23 2016
r433607 appears to have given only some moderate ~300KiB improvements in Chrome background [1], and ~500KiB improvements in WebView both foreground/background [2]. WebView is mostly fine at this point (even at branch point), but still leaves us with ~1MB regression in Chrome. Leaving to picksi, amineer, kerz decide whether this will actually block the release, and whether or not it's worth to cherry-picking #34 into M56. [1]: https://chrome-health.googleplex.com/health-plan/android-chrome/memory/nexus5/?from_commit=433448&to_commit=433839 [2]: https://chrome-health.googleplex.com/health-plan/android-webview/memory/nexus5/?from_commit=433448&to_commit=433839
,
Nov 23 2016
FWIW As we are still close to the branch point, and are looking gloomy on a number of other dimensions, I would favor cherry-picking this.
,
Nov 28 2016
Issue 669057 has been merged into this issue.
,
Dec 5 2016
Let's get this merged to M56. Marking as RB-Stable since system health plan was approved, but I still want this fixed for 56 assuming the CL in #34 fixes things.
,
Dec 5 2016
,
Jan 12 2017
[Bulk edit] Greetings from the release team! This issue is marked as a stable release blocker for M56 on Android. We're planning to ship our final beta release on Jan 25, so you have less than *two weeks* to fix this issue on trunk and merge the change back to the M56 branch. Please prioritize working on this ASAP! Sure this bug shouldn't block the release? Remove the ReleaseBlock-Stable tag. Don't think this bug should block the release, but not 100% sure? CC me to the bug and let me know! Cheers, Alex
,
Jan 13 2017
Please also merge the follow-up fix to 5.6.
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/b20f865fad9f13959fac42b48822c85d461c2ef2 commit b20f865fad9f13959fac42b48822c85d461c2ef2 Author: Toon Verwaest <verwaest@chromium.org> Date: Fri Jan 13 10:38:29 2017 Merged: [zone] Fix zone segment pooling size limits Revision: 4097c8503e4c033713e4b07ee01072896cbfd381 BUG= chromium:655129 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=hablich@chromium.org Review-Url: https://codereview.chromium.org/2627713010 . Cr-Commit-Position: refs/branch-heads/5.6@{#75} Cr-Branched-From: bdd3886218dfe76e8560eb8a18401942452ae859-refs/heads/5.6.326@{#1} Cr-Branched-From: 879f6599eee6e1dfcbe9a24bf688b261c03e9558-refs/heads/master@{#41014} [modify] https://crrev.com/b20f865fad9f13959fac42b48822c85d461c2ef2/src/zone/accounting-allocator.cc
,
Jan 13 2017
,
Jan 16 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 19 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 14 2017
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by tdres...@chromium.org
, Oct 12 2016