New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 725949 link

Starred by 6 users

Issue metadata

Status: WontFix
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

18.8%-27.5% regression in system_health.memory_mobile at 473886:473986

Project Member Reported by pmeenan@chromium.org, May 24 2017

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=725949

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg-pSxowkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDguquSuQoM


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

android-webview-nexus6
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, May 24 2017

Cc: sadrul@chromium.org
Owner: sadrul@chromium.org

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

Comment 4 by sadrul@chromium.org, May 24 2017

Status: Assigned (was: Untriaged)

Comment 5 Deleted

Comment 6 by sadrul@chromium.org, May 25 2017

Cc: dtapu...@chromium.org tdres...@chromium.org
I can't think of a good reason for memory usage to increase because of this change.

dtapuska@/tdresser@: any thoughts?
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, May 25 2017

 Issue 725943  has been merged into this issue.
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.


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?

> 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?
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.
> 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)
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
Project Member

Comment 14 by 42576172...@developer.gserviceaccount.com, May 27 2017

Cc: briander...@chromium.org
 Issue 726934  has been merged into this issue.
Project Member

Comment 15 by 42576172...@developer.gserviceaccount.com, May 27 2017

 Issue 726932  has been merged into this issue.
Project Member

Comment 16 by 42576172...@developer.gserviceaccount.com, May 30 2017

Cc: toyoshim@chromium.org
 Issue 727481  has been merged into this issue.
Project Member

Comment 17 by bugdroid1@chromium.org, 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

Project Member

Comment 18 by 42576172...@developer.gserviceaccount.com, May 31 2017

Cc: jgruber@chromium.org
 Issue 727667  has been merged into this issue.
Project Member

Comment 19 by 42576172...@developer.gserviceaccount.com, May 31 2017

 Issue 727667  has been merged into this issue.
Project Member

Comment 20 by 42576172...@developer.gserviceaccount.com, May 31 2017

 Issue 727562  has been merged into this issue.
Project Member

Comment 21 by 42576172...@developer.gserviceaccount.com, May 31 2017

 Issue 727618  has been merged into this issue.
Cc: danakj@chromium.org
 Issue 728624  has been merged into this issue.
 Issue 728624  has been merged into this issue.
Cc: -danakj@chromium.org
Cc: jasontiller@chromium.org
 Issue 728440  has been merged into this issue.
 Issue 727515  has been merged into this issue.
 Issue 727515  has been merged into this issue.
Issue 726030 has been merged into this issue.
Cc: -jgruber@chromium.org
Project Member

Comment 30 by 42576172...@developer.gserviceaccount.com, Jun 12 2017

 Issue 725948  has been merged into this issue.
Project Member

Comment 31 by 42576172...@developer.gserviceaccount.com, Jun 12 2017

 Issue 725948  has been merged into this issue.
sadrul: what's the status here?
Status: WontFix (was: Assigned)
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