Issue metadata
Sign in to add a comment
|
18.8%-27.5% regression in system_health.memory_mobile at 473886:473986 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
May 24 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8978699877523977296
,
May 24 2017
=== Auto-CCing suspected CL author sadrul@chromium.org === Hi sadrul@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 : sadrul Commit : a6251cb3214b42349e7208d285a5e2e01276fe6f Date : Tue May 23 16:06:27 2017 Subject: input: Dispatch synthesized events at regular intervals. Bisect Details Configuration: android_webview_nexus6_aosp_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:webview:all_processes:reported_by_os:system_memory:java_heap:proportional_resident_size_avg/browse_social/browse_social_twitter Change : 61.50% | 4227072.0 -> 6826666.66667 Revision Result N chromium@473885 4227072 +- 236489 6 good chromium@473911 4557995 +- 238169 6 good chromium@473918 4914517 +- 187382 6 good chromium@473921 4986197 +- 169840 6 good chromium@473922 8018603 +- 213202 6 bad <-- chromium@473923 7937365 +- 258173 6 bad chromium@473924 7474517 +- 306868 6 bad chromium@473936 6919509 +- 134286 6 bad chromium@473986 6826667 +- 172170 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 --story-filter=browse.social.twitter system_health.memory_mobile Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8978699877523977296 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5040456435499008 | 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 24 2017
,
May 25 2017
I can't think of a good reason for memory usage to increase because of this change. dtapuska@/tdresser@: any thoughts?
,
May 25 2017
Issue 725943 has been merged into this issue.
,
May 25 2017
Yeah, a bunch of those regressions are expected, but some are unintuitive. The change in noise is much less than I predicted, perhaps due to rAF aligned input or something? Why does CPU time per frame increase? It's possible CPU time and memory consumption are connected here, and I don't understand why either of them are occurring.
,
May 25 2017
For kicks would it make sense to submit a change that increased the timer to be 16.6 - 2? I wonder if the 2ms offset is the worst case scenario (before we would be in the best case scenario). And adjusting the offset to be 14.6 or so might put us back in the best case scenario?
,
May 25 2017
> The change in noise is much less than I predicted, perhaps due to rAF aligned > input or something? I am pleasantly surprised too! We don't technically do raf aligned input anymore. Maybe it's the fixed offset we start the timer at that is helping keeping the noise low? > ... And adjusting the offset to be 14.6 or so might put us back in the best case > scenario? But that's not going to be a useful metric though, right? The change is explicitly something that never affects the user, so artificially getting the numbers down just for the test does not seem all that useful? As long as the noise remains stable, we should probably be OK with the higher numbers for these metrics?
,
May 25 2017
I believe tdresser@ was talking about the rAF alignment we do in the renderer that it is hiding any noise. I was wondering if we changed the value whether only how would that change adjust the metrics; whether up or down. If we know the value of the offset changes then we should probably pick one that is in the middle to represent an average. Not worst or best case. But if changing the number has no bearing then we leave it at 2.
,
May 26 2017
> I believe tdresser@ was talking about the rAF alignment we do in the renderer > that it is hiding any noise. Ah, interesting! I was actually wondering if we should remove the offset altogether, and just start the timer immediately, instead of waiting for begin-frame + offset. The rAF alignment in the renderer will probably still take care of reducing noise for latency. I am not sure what kind of an effect it would have on cpu/memory use though (especially memory, mostly because I don't understand how changing the timing of the event dispatch can affect that at all)
,
May 26 2017
I have put up a CL [1] to remove the begin-frame alignment altogether. The smoothness test results do not seem to be affected that much by it [2, 3]. I am now running thread_times and system_health tests to see how they are affected. [1] https://codereview.chromium.org/2910513006/ [2] https://00e9e64bacd8038e96b9dcdd97fb50c4ea0a9f8d8b361e8c2d-apidata.googleusercontent.com/download/storage/v1/b/chromium-telemetry/o/html-results%2Fresults-2017-05-26_13-44-16?qk=AD5uMEsb_hB7dkaZSzLfCZH2YgoYTV_Ii-8fOXvTrUf3wSqXzVDA_4heV97NXtPB-BoNNqdbkXGO8OmCyMhcnFkIbTKuNBaRMa6rNDQ64YECDugfxrFxzPwJbU8xCecfMbdAlEy6BRYT28yL58ZViS0DUP6M0yhL50bxkCOpF46LB383o8-NEQe4AJ6IdMM26wmxgsCUgq90HyDPxNAX2zbRaWam-H-4dCfvsdDQGrk28nsNAIyOYwiGx851Piym5c1PF9SztmkWBm86NO7QfivFx6TaazpAdzEpUcfAVR1P2UbxRdTabRgNgwSJoeCK9oPbaiGyQGBuZ8y_f3C6N8aWBL3eXQ3MA1J-CGWrUUqWHjxQpwASfSKPypdJrM6SfZdmcW_tZ5P9uZVvOaPXTa6Bpjj9azdKTdWNVZQXyoVB3XYn1C1bXD-8Uh4ho0btKEEhvWWlxPeIChXoPMQnPTYXVpnt_PSMt-R-cRCemTfgfpCJrAKWB3pOfEONnChnX0YupwIioOnbjmR86WGT_nMlOfBB4WuDtoMbN3RjJqLH8chiOM_V37lauNS0gilDtoHy9ov0UqoX2cgEALBces4NnX6z1OX1ynnVRYG2N7C5v0syW5CNnHijnPZcOWL-cRF7GyUBUw0TinntLkxdUAre5ETtgBX8OpI3hsE3O6FpIFl4SjyjlADbz1zZJ64tvmznMp5OcXV5J8Fj0tV24YcmUVku5jWnAjo6JUw02PE0SrFQvC12u-M1cgtzx6xN5w0SS9d_goiFJLrJPcrqqkbXOoQw5-Tr8RX1HBR9VJNOtlHKoCgeUkViJsGc7ze_AO-UaaQjlqM1 [3] https://00e9e64bac67d940ea7aa2f470eac8a95a5138bb57e3787236-apidata.googleusercontent.com/download/storage/v1/b/chromium-telemetry/o/html-results%2Fresults-2017-05-26_13-33-48?qk=AD5uMEuTkSGaQL-TLaHbQBCIgz4vdX0dU3w4qRdbzbb81C2Pj6gw6g9p8KkrvwSUtKXuPO42lenmQFT6MwGqaGl5uSW5q911cLHC8aUWZLeQzm6mCaVuSPOhhok1qj3H0wJKgHQm38vC0Nc21hvwS0RZUz4IPrtftMweXfRw-RaQzhdRvCLG3NaT6ZiSlspt78ODILYjXiOWeUcs4OOnLXZqz0prWb_s7KWrVHZ6ijbXDgMi_NHrvP6ge9QGfyEXcySbggvmsTWIr9GdiwfkV_4XgDokRvf2bWbBKrk632fXxKe8rR4G364PtdOHhe4_EuXkNgXd8b9fG1nAO9qV3P81iVfh6A-RoISkx5HeVMGaX44mFSYxLPErnem_OI-_Rcw1eIfoJAHScu2bMJgtd4cojw63v594splMBOx3AtSSQs_8Wm2paC3kGwsEQqn6hGzkbS0rHJy0XkCf3omdlxX4J-6nx49r3876fpFeZt4TnJbX_aMUlYHCRsLp16_y47a3DKh4R9iQpQRLJxaKkdwUl4-wXy-wMilP96ViOBIGOWh0WzwujJu1342WaG1fJ0ORexXoBwEObu5K8UO9cHdnoUpRQTDUgLGSyXcbglA9pxC7wIXytMQHk6i8PRVa0AAZTyfpvM8aMO9WaTcQG5YamG33SpI9mmb3zb-lWMrcCI0QMtg3LDhQbkF2JXMJXVhUcU-TWxKr_6y6IGbn2WoEgFZqdPHrTkxDz9nSanRO_DwPs5t-5zuoGY0bwEVj0p9n4Ms1W-rMTZytmLNECnZ8F42Y5S6Kq_K4bzSYLvyqQjdcHklJ64H-ZY_ovQcZk8JBirr60yLT
,
May 27 2017
,
May 27 2017
Issue 726932 has been merged into this issue.
,
May 30 2017
,
May 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e437543b5a4c01773a489e26659a9b05b7d7c365 commit e437543b5a4c01773a489e26659a9b05b7d7c365 Author: sadrul <sadrul@chromium.org> Date: Tue May 30 23:39:32 2017 input: Immediately start timer for synthetic event dispatch. The timer to send synthesized input events during telemetry tests is currently started at begin-frame + fixed-offset. The reason for doing this is to minimize the noise in the results. However, main thread already does rAF alignment for input event dispatch, which likely means that we can change the browser to always immediately start the timer, and still not affect the noise. So do that in this CL, which simplifies the code too. BUG= 725949 , 722921 Review-Url: https://codereview.chromium.org/2910513006 Cr-Commit-Position: refs/heads/master@{#475692} [modify] https://crrev.com/e437543b5a4c01773a489e26659a9b05b7d7c365/content/browser/renderer_host/input/synthetic_gesture_controller.cc [modify] https://crrev.com/e437543b5a4c01773a489e26659a9b05b7d7c365/content/browser/renderer_host/input/synthetic_gesture_controller.h
,
May 31 2017
,
May 31 2017
Issue 727667 has been merged into this issue.
,
May 31 2017
Issue 727562 has been merged into this issue.
,
May 31 2017
Issue 727618 has been merged into this issue.
,
Jun 1 2017
,
Jun 1 2017
Issue 728624 has been merged into this issue.
,
Jun 1 2017
,
Jun 2 2017
,
Jun 2 2017
Issue 727515 has been merged into this issue.
,
Jun 2 2017
Issue 727515 has been merged into this issue.
,
Jun 2 2017
Issue 726030 has been merged into this issue.
,
Jun 6 2017
,
Jun 12 2017
Issue 725948 has been merged into this issue.
,
Jun 12 2017
Issue 725948 has been merged into this issue.
,
Aug 17 2017
sadrul: what's the status here?
,
Aug 18 2017
The relevant changes here only affected tests, and not production code. So the regressions do not reflect real regression the users would experience. One concern with the change here was that the data in the tests could become too noisy, but that has not happened. So no additional changes to be made here. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by pmeenan@chromium.org
, May 24 2017