Issue metadata
Sign in to add a comment
|
20.3%-230.8% regression in system_health.memory_mobile at 430035:430142 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Nov 8 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8996605660440515264
,
Nov 9 2016
===== BISECT JOB RESULTS ===== Status: failed ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@430034 5111808 0.0 5 good chromium@430088 5111808 0.0 5 good chromium@430095 5111808 0.0 5 good chromium@430102 11599872 4845899 5 bad chromium@430115 15138816 3956660 5 bad chromium@430142 16908288 0.0 5 bad Bisect job ran on: android_nexus9_perf_bisect Bug ID: 663179 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests system_health.memory_mobile Test Metric: memory:chrome:all_processes:reported_by_chrome:gpu:effective_size_avg/background_news/background_news_nytimes Relative Change: 230.77% Score: 0 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/2239 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8996605660440515264 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5554028140822528 | 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!
,
Nov 9 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8996464573793107776
,
Nov 9 2016
+perezju: Two problems here: 1) The bisect is failing on browse:social:facebook. What do you think is the best way to triage memory test failures in bisects? In the past, test failures have fallen through the cracks and I'd like to do better here, at least with actively maintained tests like the memory ones. 2) The first bisect didn't seem to run with --story-filter; wanted to check that makes sense based on it having been kicked off 37 hours ago? I re-ran and so far it appears to have the story filter, which is good because we're not trying to bisect browse:social:facebook.
,
Nov 9 2016
Re 1: I think the problem is flakiness affecting some of the stories. There is issue 655688 to track some of that, although I haven't seen anything in particular about browse:social:facebook. I think the real need here is to debug more of these flaky stories. Re 2: I don't know why it hasn't the story filter, it should have been there. Unless it was manually deleted when kicking off the bisect?
,
Nov 9 2016
=== Auto-CCing suspected CL author wangxianzhu@chromium.org === Hi wangxianzhu@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 : Remove obsolete code that invalidates document element on LayoutView resize Author : wangxianzhu Commit description: The code was needed when the document element painted viewport background. Now ViewPainter paint viewport background, and we invalidate LayoutView itself on its resize. BUG= 475115 Review-Url: https://codereview.chromium.org/2483483003 Cr-Commit-Position: refs/heads/master@{#430097} Commit : 1bddf0ae1615f5c67ca034d7e08dfdb1ea5846c4 Date : Sat Nov 05 00:47:39 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@430034 5111808 0.0 5 good chromium@430088 5111808 0.0 5 good chromium@430095 5111808 0.0 5 good chromium@430096 5111808 0.0 5 good chromium@430097 15138816 3956660 5 bad <-- chromium@430099 15138816 3956660 5 bad chromium@430102 15138816 3956660 5 bad chromium@430115 15138816 3956660 5 bad chromium@430142 16908288 0.0 5 bad Bisect job ran on: android_nexus9_perf_bisect Bug ID: 663179 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=background.news.nytimes system_health.memory_mobile Test Metric: memory:chrome:all_processes:reported_by_chrome:gpu:effective_size_avg/background_news/background_news_nytimes Relative Change: 230.77% Score: 99.5 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/2243 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8996464573793107776 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5860109857587200 | 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!
,
Nov 9 2016
The change has nothing to do with memory.
,
Nov 9 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8996454065501734720
,
Nov 9 2016
===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Remove obsolete code that invalidates document element on LayoutView resize Author : wangxianzhu Commit description: The code was needed when the document element painted viewport background. Now ViewPainter paint viewport background, and we invalidate LayoutView itself on its resize. BUG= 475115 Review-Url: https://codereview.chromium.org/2483483003 Cr-Commit-Position: refs/heads/master@{#430097} Commit : 1bddf0ae1615f5c67ca034d7e08dfdb1ea5846c4 Date : Sat Nov 05 00:47:39 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@430034 5111808 0.0 5 good chromium@430088 5111808 0.0 5 good chromium@430095 5111808 0.0 5 good chromium@430096 5111808 0.0 5 good chromium@430097 16908288 0.0 5 bad <-- chromium@430099 16908288 0.0 5 bad chromium@430102 11599872 4845899 5 bad chromium@430115 13369344 4845899 5 bad chromium@430142 16908288 0.0 5 bad Bisect job ran on: android_nexus9_perf_bisect Bug ID: 663179 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=background.news.nytimes system_health.memory_mobile Test Metric: memory:chrome:all_processes:reported_by_chrome:gpu:effective_size_avg/background_news/background_news_nytimes Relative Change: 230.77% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/2244 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8996454065501734720 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5801204716666880 | 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!
,
Nov 9 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8996446981293926672
,
Nov 9 2016
Will look into what caused the regression.
,
Nov 9 2016
Bisect failed: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/2245 Failure reason: the build has failed. Additional errors: The revision range could not be expanded, or the commit positions could not be resolved into commit hashes.
,
Nov 9 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8996437432310766240
,
Nov 9 2016
Traces at 430096: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_0-2016-11-09_10-08-03-30432.html https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_0-2016-11-09_10-08-52-48656.html Traces at 430097: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_0-2016-11-09_10-00-57-39946.html https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_0-2016-11-09_10-01-46-43778.html If you click on the (M) at the top right, you can see the memory dumps from both. There is a big difference in gpu>gl>textures. Attaching some screenshots, this is consistent in the traces from before/after.
,
Nov 9 2016
===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Remove obsolete code that invalidates document element on LayoutView resize Author : wangxianzhu Commit description: The code was needed when the document element painted viewport background. Now ViewPainter paint viewport background, and we invalidate LayoutView itself on its resize. BUG= 475115 Review-Url: https://codereview.chromium.org/2483483003 Cr-Commit-Position: refs/heads/master@{#430097} Commit : 1bddf0ae1615f5c67ca034d7e08dfdb1ea5846c4 Date : Sat Nov 05 00:47:39 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@430034 5111808 0.0 5 good chromium@430088 5111808 0.0 5 good chromium@430095 5111808 0.0 5 good chromium@430096 5111808 0.0 5 good chromium@430097 13369344 4845899 5 bad <-- chromium@430099 13369344 4845899 5 bad chromium@430102 15138816 3956660 5 bad chromium@430115 15138816 3956660 5 bad chromium@430142 13369344 4845899 5 bad Bisect job ran on: android_nexus9_perf_bisect Bug ID: 663179 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=background.news.nytimes system_health.memory_mobile Test Metric: memory:chrome:all_processes:reported_by_chrome:gpu:effective_size_avg/background_news/background_news_nytimes Relative Change: 161.54% Score: 98.0 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/2246 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8996437432310766240 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5131741856006144 | 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!
,
Nov 9 2016
The CL does not have direct memory impact. It just reduces unnecessary paint invalidation, which should be a good thing. Perhaps it just happens to affect the test in some way e.g. because garbage textures are collected at a different time. This might not be a real issue affecting users.
,
Nov 9 2016
Juan, Primiano, WDYT re comment #17?
,
Nov 10 2016
> Perhaps it just happens to affect the test in some way e.g. because garbage textures are collected at a different time. Can we pull in somebody who knows the graphics stack and can help us figure this out? > This might not be a real issue affecting users. Or it might, in which case the hit would pretty high. Sorry, I understand the situation, I am as surprised as you by looking at the CL. Unfortunately, I don't think we still have sufficient confidence to conclude whether this is real or not, and I don't think we should ignore this just because there is a chance it might be a timing issue. Correct me if I am wrong, but what I see on the table here is: some code cleanup vs the risk of shipping a multi-mb regression to our users if our speculation is wrong. If it's the case (i.e. a GC-related timing issue) would be great to understand what is the cause and and fix the benchmark so this won't happen again.
,
Nov 10 2016
Ben, can you help us find someone who knows the graphics stack to help us investigate per comment 19?
,
Nov 10 2016
I'm guessing: Before the CL: Frame n: we used several textures and cached them; Frame n+1: some optimizations and/or cleanups of old textures kicked in. Then we measured the memory usage. After the CL: Because of removal of the unnecessary paint invalidation, there might be no Frame n+1 after Frame n, and we measured the same status as Frame n before the CL. Could this be the situation?
,
Nov 10 2016
enne, danakj: if this is actually a situation where the compositor is holding on to textures longer would you be able to help prove or disprove that theory? Thanks.
,
Nov 11 2016
,
Nov 14 2016
From looking at the memory-infra traces for this issue, it appears that we have a number of "orphaned" GL textures in the GPU process. These are likely textures that have been deleted by a renderer, but the GPU process has not yet processed the delete and finished cleaning them up. This may indicate that we just need an additional GL flush somewhere to ensure this work is processed. Will try this out and see if it resolves the issue.
,
Nov 15 2016
Looking through memory regression bugs, I've just noted that issue 663744 was merged here. That was about some ~1MB *improvement* on graphics memory for WebView. So this change obviously increased memory usage on some pages, and reduced it in others. Hopefully the fix will take care of the regressions without taking away the improvements! :-)
,
Nov 15 2016
From investigating, I strongly suspect we will keep the benefits. Will keep you posted. Have a fix pending (crrev.com/2501063003)
,
Nov 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/174ef9083488b3d8528e9fa60820d0cec3badd50 commit 174ef9083488b3d8528e9fa60820d0cec3badd50 Author: ericrk <ericrk@chromium.org> Date: Tue Nov 15 23:10:52 2016 Handle compositor/worker context the same wrt. visibility When transitioning the worker context to not-visible mode and cleaning up resources, we wait for pending tile tasks to complete, as they may be using resources. Given that the resources used by tile tasks are created/destroyed on the compositor context (not the worker context), we should also delay compositor context's cleanup until all tile tasks are completed. This change modifies LayerTreeHostImpl so that both contexts have their visibility handled in the same way. Additionally, it fixes an issue where we were checking resource pool for pending cleanup in the wrong way (looking only at in-use resources, not at all resources). R=enne BUG= 663179 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2501063003 Cr-Commit-Position: refs/heads/master@{#432282} [modify] https://crrev.com/174ef9083488b3d8528e9fa60820d0cec3badd50/cc/resources/resource_pool.h [modify] https://crrev.com/174ef9083488b3d8528e9fa60820d0cec3badd50/cc/tiles/tile_manager.cc [modify] https://crrev.com/174ef9083488b3d8528e9fa60820d0cec3badd50/cc/trees/layer_tree_host_impl.cc [modify] https://crrev.com/174ef9083488b3d8528e9fa60820d0cec3badd50/cc/trees/layer_tree_host_impl.h
,
Nov 15 2016
Verification blocked on crbug.com/664521 , which has disabled the test case which showed the regression - background:news:nytimes. Locally this change addresses the issue.
,
Nov 21 2016
Issue 667258 has been merged into this issue.
,
Dec 16 2016
Verified that the metric returned to normal values in a range that includes r432282. Thanks!
,
Apr 11 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8982611958979749904
,
Apr 11 2017
=== BISECT JOB RESULTS === Perf regression found but unable to narrow commit range Build failures prevented the bisect from narrowing the range further. Bisect Details Configuration: android_nexus9_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:chrome:all_processes:reported_by_chrome:gpu:effective_size_avg/background_news/background_news_nytimes Suspected Commit Range 1 commits in range https://chromium.googlesource.com/chromium/src/+log/31922c6cd1181c77ae039adf22243cae6ca526f3..584115bcc2a6724daf09482a199ddb179f1a9cc2 Revision Result N chromium@430034 5111808 +- 0.0 5 good chromium@430088 5111808 +- 0.0 5 good chromium@430095 5111808 +- 0.0 5 good chromium@430102 11599872 +- 4845899 5 bad chromium@430115 15138816 +- 3956660 5 bad chromium@430142 16908288 +- 0.0 5 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 system_health.memory_mobile Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8982611958979749904 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5554028140822528 | 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! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by briander...@chromium.org
, Nov 8 2016