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

Issue 706927 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

1.3%-36.6% regression in memory.top_10_mobile_stress at 460138:460215

Project Member Reported by lanwei@chromium.org, Mar 30 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Mar 31 2017

Cc: skobes@chromium.org
Owner: skobes@chromium.org

=== Auto-CCing suspected CL author skobes@chromium.org ===

Hi skobes@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 : skobes
  Commit : 56451775d90238d9def5e9663771de8d0ca7ef6f
  Date   : Tue Mar 28 18:46:38 2017
  Subject: Feed ScrollableArea::showOverlayScrollbars into ScrollbarAnimationController.

Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : memory.top_10_mobile_stress
  Metric       : memory:chrome:all_processes:reported_by_chrome:cc:effective_size_avg/foreground/https_mobile_twitter_com_justinbieber_skip_interstitial_true
  Change       : 36.20% | 45108713.3333 -> 61438100.0

Revision             Result                    N
chromium@460170      45108713 +- 7326952       6      good
chromium@460176      45436393 +- 5710038       6      good
chromium@460177      46588735 +- 14353313      6      good
chromium@460178      60203839 +- 3829980       6      bad       <--
chromium@460179      58925887 +- 10065022      6      bad
chromium@460182      61492713 +- 5579933       6      bad
chromium@460193      59898004 +- 3255761       6      bad
chromium@460215      61438100 +- 5978342       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-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests memory.top_10_mobile_stress

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8983672535679758736

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6624879371091968


| 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!
Status: Started (was: Untriaged)

Comment 5 by skobes@chromium.org, Apr 12 2017

Cc: aelias@chromium.org
aelias: I am stumped by this bug.  I can repro the regression locally and it disappears if I comment out the body of ScrollbarAnimationController::DidRequestShowFromMainThread, but there are only a handful of calls to this method and I can't figure out how it's causing an extra ~20 MB of cc renderer memory for story "https_mobile_twitter_com_justinbieber_skip_interstitial_true".  Do you have any suggestions?

Comment 6 by aelias@chromium.org, Apr 12 2017

No specific idea, that's pretty weird.  I'd suggest running memory tracing and examining what category the extra 20MB is accounted under, I might have a better idea when I have some clue to its nature.

Comment 7 by skobes@chromium.org, Apr 12 2017

How do I see the trace data?  When I load tools/perf/results.json in chrome://tracing it's just a blank window.

Comment 8 by skobes@chromium.org, Apr 12 2017

Cc: primiano@chromium.org

Comment 9 by skobes@chromium.org, Apr 12 2017

I found the trace (it writes an .html file in tools/perf with the story name).  I see about twice as many 1.9MB resource_nn objects under cc > tile_memory > provider_0, versus baseline.

The extra ones(?) have values in the "free_size" column that are equal to effective_size.  Does this mean they are in some sort of intermediate state?
trace.baseline.html
3.3 MB View Download
trace.html
3.3 MB View Download
trace.screenshot.png
139 KB View Download
Cc: ericrk@chromium.org
Eric, what does it mean for a tile to be in "free_size"?
"free_size" indicates that the tile is not currently in use, but is being cached in case we want to use it in the future (partial raster, relies on this, but also it just cuts down allocations, which can be expensive). Note that we very aggressively purge free tiles, so if something is showing up here, it means it has been used within the last 1 second (https://cs.chromium.org/chromium/src/cc/resources/resource_pool.cc?rcl=cceb830695c05acfa0c6a4fbcba8ecf8efb9551e&l=27).

Not sure why your change is impacting this... I can take a quick look tomorrow to see if anything jumps out at me.


Owner: ericrk@chromium.org
Status: Assigned (was: Started)
Thanks ericrk, mind if I route this to you since it sounds like an issue with tile management?  I can provide more context on the scrollbar changes if necessary.
It appears that the change results in more rasterization, which means more tiles are used / kept alive. Still debugging, but can you think of any reason why this change would lead to additional invalidations or raster?
ScrollbarAnimationController drives opacity animation on scrollbar layers, and r460178 added a new codepath to trigger the animation.

I expected it to be a behavior nop, but maybe there's some edge case where we show scrollbars now and didn't before?

Even if there is such a case, I don't think a scrollbar layer should consume 20MB of tile memory.
Project Member

Comment 15 by 42576172...@developer.gserviceaccount.com, Apr 18 2017

Cc: benhenry@google.com
 Issue 712453  has been merged into this issue.
It looks like removing the ShowOverlayScrollbars() calls from both ScrollableArea::ContentsResized and FrameView::ViewportSizeChanged fixes this bug and  issue 712453 .  The first of these was already planned for  issue 606395 .

We could also remove the show trigger from ViewportSizeChanged, but only for Android.  (Desktop should probably retain it for window resizing.)

But in the meantime I'd like to land a patch that makes SAC::DidRequestShowFromMainThread a nop, so that we can merge it into 59.
Project Member

Comment 17 by bugdroid1@chromium.org, Apr 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a4d4531daca4b9a1f6dc1224bcb7134dc053ff4

commit 8a4d4531daca4b9a1f6dc1224bcb7134dc053ff4
Author: skobes <skobes@chromium.org>
Date: Mon Apr 24 19:19:10 2017

Temporarily ignore ScrollbarAnimationController::DidRequestShowFromMainThread.

BUG= 706927 , 712453 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2834703004
Cr-Commit-Position: refs/heads/master@{#466712}

[modify] https://crrev.com/8a4d4531daca4b9a1f6dc1224bcb7134dc053ff4/cc/input/scrollbar_animation_controller.cc

Labels: Merge-Request-59
Owner: skobes@chromium.org
Please add applicable OSs.
Labels: OS-Android
Project Member

Comment 21 by sheriffbot@chromium.org, Apr 24 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 22 by bugdroid1@chromium.org, Apr 24 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dddc56692b829eb3d12e23d47e82549381557d06

commit dddc56692b829eb3d12e23d47e82549381557d06
Author: Steve Kobes <skobes@chromium.org>
Date: Mon Apr 24 23:51:54 2017

Temporarily ignore ScrollbarAnimationController::DidRequestShowFromMainThread.

BUG= 706927 , 712453 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2834703004
Cr-Commit-Position: refs/heads/master@{#466712}
(cherry picked from commit 8a4d4531daca4b9a1f6dc1224bcb7134dc053ff4)

Review-Url: https://codereview.chromium.org/2836243002 .
Cr-Commit-Position: refs/branch-heads/3071@{#184}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/dddc56692b829eb3d12e23d47e82549381557d06/cc/input/scrollbar_animation_controller.cc

It turns out r466712 didn't actually move the graphs from this issue or  issue 712453 .

The first_gesture_scroll_update_latency metric actually returned to its original levels between 466539 - 466552 (though nothing in the range looks likely):

https://chromeperf.appspot.com/report?sid=97e645b929da551dde7f530a4424d4f81dcd6ff1bc8eec73ed76f0acddaba156

The cc memory regression got better and then worse and then even worse and then better and then worse again for some other reasons:

https://chromeperf.appspot.com/report?sid=b2bf4017eb609d82437a3edb8f1f723fcdfe51849c8187e2c29a92d5f5eaa228&start_rev=443961&end_rev=467113

There's also  issue 715279  which is somewhat mysterious.
Project Member

Comment 24 by bugdroid1@chromium.org, Apr 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66aacfa671360982f234c0fbd3b186efe21ee763

commit 66aacfa671360982f234c0fbd3b186efe21ee763
Author: skobes <skobes@chromium.org>
Date: Wed Apr 26 23:40:39 2017

Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread.

Disabling it didn't fix  http://crbug.com/706927 , but caused a new regression in
 http://crbug.com/715279 , while  http://crbug.com/712453  recovered for some other
reason.

BUG= 606395 ,  706927 ,  712453 ,  715279 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2834393003
Cr-Commit-Position: refs/heads/master@{#467513}

[modify] https://crrev.com/66aacfa671360982f234c0fbd3b186efe21ee763/cc/input/scrollbar_animation_controller.cc

The cc memory graph seems to have partially recovered:

https://chromeperf.appspot.com/report?sid=b2bf4017eb609d82437a3edb8f1f723fcdfe51849c8187e2c29a92d5f5eaa228&start_rev=443961&end_rev=473864

I'm closing this since I'm not sure what else to do here.
Status: WontFix (was: Assigned)

Sign in to add a comment