Issue metadata
Sign in to add a comment
|
2.6%-82.2% regression in system_health.memory_mobile at 476026:476079 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 1 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8977954065405023392
,
Jun 3 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 : khushalsagar Commit : 4e1be6ae8914f0eeed1df004b74a467bcb93241b Date : Wed May 31 21:45:49 2017 Subject: cc: Enable perf testing for checker-imaging. Bisect Details Configuration: android_webview_arm64_aosp_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:webview:all_processes:reported_by_chrome:cc:effective_size_avg/browse_social/browse_social_facebook Change : 85.21% | 75479154.6667 -> 139797682.667 Revision Result N chromium@476025 75479155 +- 2251315 6 good chromium@476039 78116844 +- 6868531 6 good chromium@476040 77943393 +- 10861536 6 good chromium@476041 141086760 +- 11449726 6 bad <-- chromium@476043 143946373 +- 19926677 6 bad chromium@476046 135027733 +- 50862353 6 bad chromium@476052 139513875 +- 7289776 6 bad chromium@476079 139797683 +- 9433451 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.facebook system_health.memory_mobile Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8977954065405023392 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6011693253001216 | 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!
,
Jun 3 2017
,
Jun 3 2017
Issue 729230 has been merged into this issue.
,
Jun 5 2017
Issue 729218 has been merged into this issue.
,
Jun 5 2017
The decode speed regression is from the decode being scheduled to run in parallel with the checker-imaged tiles. The duration for the decode itself is still the same, but it gets de-scheduled more. Earlier it would have blocked all raster from starting at all (traces attached). From the memory trace, it looks while some decodes were deferred to the decode thread, a few long decodes were not checkered and scheduled on the task graph with blocking activation. So the checkered images have to wait longer for invalidation and keep the memory for the decodes locked for longer duration. In the meanwhile, this was switched on in canary only. I'm turning it off for now.
,
Jun 5 2017
+enne, vmiura FYI. I don't think there is much we can do with the decode timing regression. It was expected to some extent.
,
Jun 5 2017
Yeah, basically we knew that scheduling a decode on a lower priority thread would result in longer decodes. From looking at traces in #7 with khushalsagar@, it looks like the feature is behaving as expected. We get two reasonably quick frames with a decode happening between pending trees. Without checker imaging, it's a single pending tree that takes as long as the decode+raster.
,
Jun 6 2017
,
Jun 6 2017
Issue 729231 has been merged into this issue.
,
Jun 6 2017
Issue 729232 has been merged into this issue.
,
Jun 6 2017
,
Jun 6 2017
,
Jun 6 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8977552544121290832
,
Jun 6 2017
=== BISECT JOB RESULTS === NO Perf regression found Bisect Details Configuration: android_nexus5X_perf_bisect Benchmark : v8.mobile_infinite_scroll_tbmv2 Metric : v8-gc-total_avg/discourse Revision Result N chromium@476023 0.876156 +- 0.31118 21 good chromium@476161 0.850988 +- 0.349406 21 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=discourse v8.mobile_infinite_scroll_tbmv2 Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8977552544121290832 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5847218822578176 | 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!
,
Jun 6 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8977544149990697296
,
Jun 6 2017
=== BISECT JOB RESULTS === NO Perf regression found Bisect Details Configuration: android_nexus5X_perf_bisect Benchmark : v8.mobile_infinite_scroll_tbmv2 Metric : v8-gc-total_avg/discourse Revision Result N chromium@476023 0.911681 +- 0.289035 21 good chromium@476161 0.877223 +- 0.369155 21 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=discourse v8.mobile_infinite_scroll_tbmv2 Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8977544149990697296 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5847218822578176 | 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!
,
Jun 6 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8977518809653167504
,
Jun 6 2017
=== BISECT JOB RESULTS === NO Perf regression found Bisect Details Configuration: android_nexus5X_perf_bisect Benchmark : v8.mobile_infinite_scroll_tbmv2 Metric : v8-gc-total_avg/discourse Revision Result N chromium@476023 0.865713 +- 0.388793 21 good chromium@476161 0.847067 +- 0.347258 21 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=discourse v8.mobile_infinite_scroll_tbmv2 Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8977518809653167504 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6157216878428160 | 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!
,
Jun 6 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8977510171550118144
,
Jun 6 2017
Per comment #9, this sounds like WAI, and WontFix. khushalsagar@, if you agree please resolve this bug. :)
,
Jun 6 2017
=== BISECT JOB RESULTS === NO Perf regression found Bisect Details Configuration: android_nexus5X_perf_bisect Benchmark : v8.mobile_infinite_scroll_tbmv2 Metric : v8-gc-total_avg/discourse Revision Result N chromium@476023 0.933886 +- 0.438167 21 good chromium@476161 0.943248 +- 0.490018 21 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=discourse v8.mobile_infinite_scroll_tbmv2 Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8977510171550118144 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6157216878428160 | 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!
,
Jun 6 2017
There are 3 regressions that have been merged in this bug: 1) system_health.memory_mobile: This is actionable. I have a patch up which should fix this (https://codereview.chromium.org/2928433003/). 2) image_decoding.image_decoding_measurement: This is working as intended. 3) thread_times.GPU/raster/other_cpu_timer_per_frame: The increase in other is explained by the fact that the decode is happening on a background thread. But we shouldn't be seeing an increase in raster/GPU thread. It understandable if the raster time per frame is the same because image decodes don't block pre-paint work, but it should not have regressed. vmpstr@ and I looked it and we have an interaction where for pre-decode images are being checkered so now we unnecessarily raster those tiles twice while the pre-decodes still run on the background tile worker. We are looking into the scheduling improvements here.
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98125cbd86d41a4f9b7915ac2b67197f20b130b2 commit 98125cbd86d41a4f9b7915ac2b67197f20b130b2 Author: khushalsagar <khushalsagar@chromium.org> Date: Thu Jun 08 04:45:22 2017 cc: Add scaling for checkered images. Currently checkered images are decoded at original size. Since the GPU image decode cache assumes that any decodes it has seen will also be used with raster, it uploads and keeps this decode both in discardable and in GPU memory, even if each use of this image at raster is downscaled, thereby increasing memory usage. This change has checker-imaging use the max raster scale and quality seen for an image before starting the decode, which has the max possibility of the image being used by the GPU cache. BUG= 728796 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2928433003 Cr-Commit-Position: refs/heads/master@{#477891} [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/checker_image_tracker.cc [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/checker_image_tracker.h [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/checker_image_tracker_unittest.cc [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/decoded_image_tracker.cc [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/decoded_image_tracker_unittest.cc [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/image_controller.cc [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/image_controller.h [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/image_controller_unittest.cc [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/tiles/tile_manager.cc [modify] https://crrev.com/98125cbd86d41a4f9b7915ac2b67197f20b130b2/cc/trees/layer_tree_host_unittest.cc
,
Jun 8 2017
,
Jun 9 2017
So the memory perf regression looks fixed with the change above(both desktop and mobile): https://chromeperf.appspot.com/group_report?rev=477891. And the thread_gpu_cpu_time_per_frame also recovered: https://chromeperf.appspot.com/report?sid=6617c23e053b434d69026027bd431a7762aae539fca07200123d1b2768a47f61&rev=477891 Because we upload the scaled image instead of the original decode we were doing earlier. A minor increase is expected because we have more raster work for replacing checker-imaged tiles. The only one left is raster_cpu_time_per_frame. https://codereview.chromium.org/2924233002/ fixes that.
,
Jun 13 2017
Issue 728814 has been merged into this issue.
,
Jun 13 2017
Issue 728797 has been merged into this issue.
,
Jun 19 2017
Issue 734568 has been merged into this issue.
,
Jun 19 2017
Is the image decode regression is only from the lower priority thread? Specifically, it is timing only the image decode, not the time-to-presented. If it is timing time-to-presented then we may be adding a lot of variance to the measurements. This is all a step in the right direction, but it will make that perf less clear.
,
Jun 19 2017
Its not measuring the cpu time for the decode but the wall time, which is why a change in thread priority and the fact that raster work may be happening in parallel with the decode is causing the metric to regress.
,
Jun 19 2017
,
Jun 22 2017
Issue 729220 has been merged into this issue.
,
Jun 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de93e30d4aa72f1c1a5c88e2aafba0021697b351 commit de93e30d4aa72f1c1a5c88e2aafba0021697b351 Author: khushalsagar <khushalsagar@chromium.org> Date: Fri Jun 23 23:03:46 2017 cc: Move pre-decoding for checkerable images to image worker. Currently images for pre-decoded but not pre-painted tiles are decoded on the background tile worker, even for checkerable images. This has the following downsides: 1) It creates cases where decoding of images for lower priority pre-decode tiles starts pre-empting a higher priority decode running on the image worker. 2) Since the checker image tracking is not aware of pre-decoded images, we still checker them when these tiles are first rasterized, even while the image could be currently locked in the decode cache. 3) If the image is in both pre-paint and pre-decode region, then we could be decoding them on both, the raster worker and image worker. This change moves decoding of checkerable images for these tiles to the image worker, similar to the rasterized tiles. While invalidating is not neccesary for these images, we use the same mechanism in order to generate tasks for uploading these images in the GPU cache. BUG= 728796 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2924233002 Cr-Commit-Position: refs/heads/master@{#482062} [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/paint/draw_image.cc [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/paint/draw_image.h [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/test/fake_layer_tree_host_impl_client.cc [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/test/fake_layer_tree_host_impl_client.h [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/test/layer_tree_test.cc [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/test/test_hooks.h [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/tiles/checker_image_tracker.cc [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/tiles/checker_image_tracker.h [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/tiles/checker_image_tracker_unittest.cc [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/tiles/tile_manager.cc [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/tiles/tile_manager.h [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/tiles/tile_manager_unittest.cc [modify] https://crrev.com/de93e30d4aa72f1c1a5c88e2aafba0021697b351/cc/trees/layer_tree_host_unittest_checkerimaging.cc
,
Jun 26 2017
raster_cpu_time_per_frame is fixed with the change above: https://chromeperf.appspot.com/group_report?rev=482062 It shows up as a regression in other_cpu_time_per_frame because that decode moved off the raster thread to the image worker thread. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by jasontiller@chromium.org
, Jun 1 2017