Issue metadata
Sign in to add a comment
|
3.9%-59.4% regression in thread_times.key_mobile_sites_smooth at 506845:507022 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Oct 9 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8966202227610489696
,
Oct 9 2017
=== 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
,
Oct 9 2017
Issue 773029 has been merged into this issue.
,
Oct 11 2017
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.
,
Oct 11 2017
Issue 773028 has been merged into this issue.
,
Oct 11 2017
Issue 773030 has been merged into this issue.
,
Oct 11 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8966021352154353520
,
Oct 11 2017
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.
,
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
,
Oct 11 2017
,
Oct 14 2017
,
Oct 16 2017
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.
,
Oct 17 2017
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.
,
Oct 17 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8965443811689459952
,
Oct 17 2017
The wikipedia bug seems to be fixed now. Started a bisect to figure out what fixed it.
,
Oct 18 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8965436474098063072
,
Oct 18 2017
=== 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
,
Oct 18 2017
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.
,
Oct 18 2017
,
Oct 18 2017
,
Oct 18 2017
,
Oct 18 2017
Looks like there is no way to undupe this bug now...
,
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
,
Oct 18 2017
775378 is macOS only, so this dupe seems incorrect.
,
Oct 19 2017
,
Oct 19 2017
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?
,
Oct 20 2017
,
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
,
Oct 27 2017
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 |
|||||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Oct 9 2017