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

Issue 772881 link

Starred by 5 users

Issue metadata

Status: Fixed
Merged: issue 775378
Owner:
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android , iOS
Pri: 2
Type: Bug-Regression



Sign in to add a comment

3.9%-59.4% regression in thread_times.key_mobile_sites_smooth at 506845:507022

Project Member Reported by fmea...@chromium.org, Oct 9 2017

Issue description

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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=fd5d0cf539b079f8423505f89e5fa99e94f99e2bb6e33d42c946511c40cd86a0


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

android-nexus5X
android-nexus6
android-nexus7v2
android-webview-nexus5X
android-webview-nexus6
Cc: khushals...@chromium.org
Owner: khushals...@chromium.org
Status: Assigned (was: Untriaged)

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

Hi khushalsagar@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 : Khushal
  Commit : 051a012c57a3fdfd6c933ec3df1c0e60d247a73a
  Date   : Fri Oct 06 04:42:46 2017
  Subject: perf: Enable field trial testing for image animations in cc.

Bisect Details
  Configuration: android_nexus5X_perf_bisect
  Benchmark    : thread_times.key_mobile_sites_smooth
  Metric       : thread_raster_cpu_time_per_frame/http___www.boingboing.net
  Change       : 73.77% | 1.04148901597 -> 1.80974864675

Revision             Result                    N
chromium@506844      1.04149 +- 0.0701811      6      good
chromium@506921      1.03632 +- 0.0664089      6      good
chromium@506960      1.03451 +- 0.0441411      6      good
chromium@506965      1.04375 +- 0.0188957      6      good
chromium@506968      1.04268 +- 0.0599394      6      good
chromium@506969      1.76659 +- 0.0317027      6      bad       <--
chromium@506970      1.78086 +- 0.0761966      6      bad
chromium@506979      1.77752 +- 0.0660864      6      bad
chromium@506998      1.80975 +- 0.0992913      6      bad

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 --story-filter=http...www.boingboing.net thread_times.key_mobile_sites_smooth

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8966202227610489696


For feedback, file a bug with component Speed>Bisection
 Issue 773029  has been merged into this issue.
Cc: -khushals...@chromium.org vmp...@chromium.org vmi...@chromium.org enne@chromium.org
There are 2 regressions merged into this bug.

First there is a memory regression on nexus5 and mac12. The image decode caches have more data, but this isn't actually a regression since its coming from more frames decoded for animated images. We are not blocked on the main thread for animating images anymore, they run smoother and decode more frames which are added to the cache. I verified by taking a trace and you can see the main frame running for ~300ms in some cases while the compositor thread continues to animate images. I also added a couple of more traces from images when the memory dump is taken:

1) ContentId count: ContentId is shared across individual frames of the same image. So this is the count of total number of images in the cache. It was 40 with compositor animations enabled and 39 without it.

2) Frame Count: This is the total number of unique frames across all images. It was 194 with compositor animations and 101 without it.

So the number of images is the same but we have more frames when cc is running the animation.

The second regression looks like an overall regression from more raster only on boingboing.net. I'm going to investigate it more.
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Oct 11 2017

Issue 773028 has been merged into this issue.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Oct 11 2017

Issue 773030 has been merged into this issue.
For the memory increases, tumblr, nytimes and gmail have an increase in image cache memory from more entries, which is consistent with the analysis in #5. But 2 cases still need more investigation:

load_accessibility_media_wikipedia has an increase in shared_memory:effective_size_avg. No change in the memory reported for cc.

TrivialGifPageSharedPageState has an increase in tile memory and no change in image memory.
Project Member

Comment 10 by 42576172...@developer.gserviceaccount.com, Oct 11 2017


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Khushal
  Commit : 051a012c57a3fdfd6c933ec3df1c0e60d247a73a
  Date   : Fri Oct 06 04:42:46 2017
  Subject: perf: Enable field trial testing for image animations in cc.

Bisect Details
  Configuration: mac_10_12_perf_bisect
  Benchmark    : system_health.memory_desktop
  Metric       : memory:chrome:all_processes:reported_by_chrome:shared_memory:effective_size_avg/load_accessibility_media/load_accessibility_media_wikipedia
  Change       : 2.38% | 344746.666667 -> 352938.666667

Revision             Result                 N
chromium@506932      344747 +- 6894.6       6      good
chromium@506955      344747 +- 9004.99      6      good
chromium@506966      344747 +- 3739.12      6      good
chromium@506968      341333 +- 7478.24      6      good
chromium@506969      351573 +- 3739.12      6      bad       <--
chromium@506972      357035 +- 9004.99      6      bad
chromium@506977      354304 +- 9605.97      6      bad
chromium@507022      352939 +- 6894.6       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=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.accessibility.media.wikipedia system_health.memory_desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8966021352154353520


For feedback, file a bug with component Speed>Bisection
Project Member

Comment 11 by 42576172...@developer.gserviceaccount.com, Oct 11 2017

Cc: nzolghadr@chromium.org
 Issue 773726  has been merged into this issue.
Project Member

Comment 12 by 42576172...@developer.gserviceaccount.com, Oct 14 2017

Cc: khushals...@chromium.org
 Issue 773021  has been merged into this issue.
This change: https://chromium-review.googlesource.com/c/chromium/src/+/720245, should address the regression for thread_times and smoothness.sync_scroll on boingboing. There is still a slight increase in mean_input_event_latency, from ~42.7 to ~45 when I tested locally on N5, since there can be cases where we queue an impl-side pending tree if the main thread is taking too long and the main thread is blocked until this tree is activated. But such cases are expected and are very infrequent.
Cc: chrishtr@chromium.org
The reason for the regression on boingboing was that while cc doesn't animate offscreen images, if any instance of the image is onscreen then all instances of this image will be invalidated and re-rastered. This is because compositor's tracking of images and image locations is keyed on PaintImage, which will be shared across elements using the same src.

On boingboing, we have multiple elements using the same PaintImage. So if one of them is onscreen, all of them are invalidated. This leads to more raster work due to continuous invalidations to prepaint tiles. Its not the case when blink is driving the animation since blink's throttling of invalidations is per element, i.e., it would not invalidate an element with an animated image if its offscreen.

We discussed today whether addressing this edge case was worth the code complexity and decided against it.
The wikipedia bug seems to be fixed now. Started a bisect to figure out what fixed it.
Project Member

Comment 18 by 42576172...@developer.gserviceaccount.com, Oct 18 2017

Mergedinto: 775378
Status: Duplicate (was: Assigned)

=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Greg Kerr
  Commit : fcd7b94faa40dfc3e4e4be26f4e854dd14ef650c
  Date   : Thu Oct 12 22:48:50 2017
  Subject: Enable Finch Trial Test for MacV2Sandbox.

Bisect Details
  Configuration: mac_10_12_perf_bisect
  Benchmark    : system_health.memory_desktop
  Metric       : memory:chrome:all_processes:reported_by_chrome:shared_memory:effective_size_avg/load_accessibility_media/load_accessibility_media_wikipedia
  Change       : 2.44% | 354531.555556 -> 345884.444444

Revision             Result                 N
chromium@508397      354532 +- 16497.4      9      good
chromium@508466      354304 +- 7662.91      6      good
chromium@508501      356807 +- 12211.9      9      good
chromium@508510      354987 +- 12952.7      9      good
chromium@508512      356352 +- 10033.1      6      good
chromium@508513      344064 +- 5792.62      6      bad       <--
chromium@508514      347250 +- 9654.36      9      bad
chromium@508518      346795 +- 8192.0       9      bad
chromium@508535      345884 +- 11745.0      9      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=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.accessibility.media.wikipedia system_health.memory_desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8965443811689459952


For feedback, file a bug with component Speed>Bisection
Status: WontFix (was: Duplicate)
Unduping since the rest of the regressions on the bug are not from r508513. Marking this as WontFix since we understand all regressions now:

1) Memory regressions for tumblr, imgur, nytimes are from the animations running smoother and putting more cached frames in the image decode cache.

2) boingboing raster and sync_scroll regression is from invalidations of prepaint tiles. But this is an edge case where multiple instances of the same image exist on the page and only some are visible. We have decided against handling such cases to avoid prepaint invalidations.

3) Memory regression on TrivialGifPageSharedSet seems like a timing issue. I checked a few traces after the one that introduced the spike and I see tile memory back to the pre-regression value in many of them.
Status: Assigned (was: WontFix)
Status: Duplicate (was: Assigned)
Status: Assigned (was: Duplicate)
Status: WontFix (was: Assigned)
Looks like there is no way to undupe this bug now...
Project Member

Comment 24 by 42576172...@developer.gserviceaccount.com, Oct 18 2017


=== BISECT JOB RESULTS ===
NO Perf regression found

Bisect Details
  Configuration: mac_10_12_perf_bisect
  Benchmark    : memory.desktop
  Metric       : memory:chrome:all_processes:reported_by_chrome:malloc:effective_size_avg/TrivialGifPageSharedPageState

Revision             Result                  N
chromium@506932      25719797 +- 661174      21      good
chromium@506991      25695263 +- 774920      21      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=TrivialGifPageSharedPageState memory.desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8965436474098063072


For feedback, file a bug with component Speed>Bisection
Labels: OS-iOS
775378 is macOS only, so this dupe seems incorrect.
Project Member

Comment 26 by 42576172...@developer.gserviceaccount.com, Oct 19 2017

Cc: chiniforooshan@chromium.org
 Issue 776365  has been merged into this issue.
Labels: OS-Android
Status: Started (was: WontFix)
Bisect says https://chromeperf.appspot.com/group_report?sid=c2f3aec23ea3661e86583a698bce8445ab34dda20823133259c1755fe287e4f1 is due to chromium@506969.

Should we wait for https://chromium-review.googlesource.com/c/chromium/src/+/720245 to be landed and then mark this as fixed if that patch brings the graph down again?
Project Member

Comment 28 by 42576172...@developer.gserviceaccount.com, Oct 20 2017

Cc: pmeenan@chromium.org
 Issue 776735  has been merged into this issue.
Project Member

Comment 29 by bugdroid1@chromium.org, Oct 23 2017

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

commit 71ac7cd0994f5688c0677386c2354e79f7a09ca2
Author: Khushal <khushalsagar@chromium.org>
Date: Mon Oct 23 22:50:05 2017

cc: Use Region for tracking positions of images in DiscardableImageMap.

The position tracking for images maintains a union of rects for all
instances of this image. This means that if the same image is used by
multiple elements, an image invalidation invalidates a unioned rect for
all these images. In the case of animated images this can lead to
continuous expensive repaints of offscreen content.

In order to avoid this, maintain a Region instead of a unioned rect for
images in the DiscardableImageMap. But clamp the vector size
maintained to 256. The reason for reverting to rect from region was a
regression from the overhead of this tracking on motion mark
anonimeter.

R=chrishtr@chromium.org, vmpstr@chromium.org

Bug:  772881 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ie868f66d473625003b398ea6d55ae0f40440193f
Reviewed-on: https://chromium-review.googlesource.com/720245
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510948}
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/base/invalidation_region.cc
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/base/invalidation_region.h
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/layers/picture_layer_impl.cc
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/paint/discardable_image_map.cc
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/paint/discardable_image_map.h
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/paint/discardable_image_map_unittest.cc
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/raster/raster_source.cc
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/raster/raster_source.h
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/71ac7cd0994f5688c0677386c2354e79f7a09ca2/cc/trees/layer_tree_impl.cc

Status: Fixed (was: Started)
The regression on boingboing is fixed (from the bots that picked up the change, the rest are still catching up). Rest of the regressions are still WontFix.

Sign in to add a comment