Issue metadata
Sign in to add a comment
|
Mismatched visual rects and cull rects on various webview telemetry tests. |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
May 25 2017
=== Auto-CCing suspected CL author danakj@chromium.org === Hi danakj@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 : danakj Commit : 4188df86ebb468a6723b1c5855c75be8e4ae91a3 Date : Tue May 23 20:05:29 2017 Subject: Avoid quickReject() when rastering a cc::DrawingDisplayItem. Bisect Details Configuration: android_webview_arm64_aosp_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:webview:all_processes:reported_by_chrome:skia:effective_size_avg/load_media/load_media_youtube Change : 2.21% | 142404.0 -> 145556.0 Revision Result N chromium@473980 142404 +- 0.0 6 good chromium@474004 142404 +- 0.0 6 good chromium@474010 142404 +- 0.0 6 good chromium@474013 142404 +- 0.0 6 good chromium@474014 145556 +- 0.0 6 bad <-- chromium@474015 145556 +- 0.0 6 bad chromium@474016 145556 +- 0.0 6 bad chromium@474027 145556 +- 0.0 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=load.media.youtube system_health.memory_mobile Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8978699940310189488 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=5333488095461376 | 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!
,
May 25 2017
Issue 725947 has been merged into this issue.
,
May 25 2017
Any idea how this could result?
,
May 25 2017
Could this be due to what trchen@ noted: https://chromium-review.googlesource.com/c/506519/#message-1f8760db0695b0a039d895802644315c80a7daec Perhaps with enough of these particular test cases we raster more than we did previously. How that would lead to add'l memory utilization, though -- maybe some tiles that were previously solid color aren't any more?
,
May 25 2017
Hm, I put a DCHECK if the quickReject() would happen and ran the load:media:youtube story and it never fired.
,
May 25 2017
tools/perf/run_benchmark run system_health.memory_mobile --browser=android-webview-instrumentation --story-filter=load:media:youtube --output-format=gtest is what i ran.
,
May 25 2017
Augh ok I confirmed it's not using the webview I installed to run these tests so ok idk AUGH
,
May 25 2017
oook so i build debug android_webview_apk and ran this and now I can see it crash if I CHECK(false) always so I know it's running my code
tools/perf/run_benchmark run system_health.memory_mobile --browser=android-webview --story-filter=load:media:youtube --output-format=gtest
So I put back the CHECK that we're not quickrejecting and determined we would have quick-rejected:
05-25 13:21:25.278 27557 27557 F DEBUG : Abort message: '[FATAL:display_item_list.cc(102)] Check failed: false. =============== Would have rejected 16,448,117,16
/usr/local/google/home/danakj/s/c/src/cc/paint/display_item_list.cc:102
0000f7ab cc::DisplayItemList::Raster(SkCanvas*, SkPicture::AbortCallback*) const /usr/local/google/home/danakj/s/c/src/cc/paint/display_item_list.cc:225
000c45a9 cc::RasterSource::RasterCommon(SkCanvas*, SkPicture::AbortCallback*) const /usr/local/google/home/danakj/s/c/src/cc/raster/raster_source.cc:218
000c4807 cc::RasterSource::PlaybackToCanvas(SkCanvas*, gfx::ColorSpace const&, cc::RasterSource::PlaybackSettings const&) const /usr/local/google/home/danakj/s/c/src/cc/raster/raster_source.cc:120
000c4285 cc::RasterSource::PlaybackToCanvas(SkCanvas*, gfx::ColorSpace const&, gfx::Rect const&, gfx::Rect const&, gfx::AxisTransform2d const&, cc::RasterSource::PlaybackSettings const&) const /usr/local/google/home/danakj/s/c/src/cc/raster/raster_source.cc:84
v------> RasterizeSource /usr/local/google/home/danakj/s/c/src/cc/raster/gpu_raster_buffer_provider.cc:82
000c0ac3 cc::GpuRasterBufferProvider::PlaybackOnWorkerThread(cc::ResourceProvider::ScopedWriteLockGL*, gpu::SyncToken const&, bool, cc::RasterSource const*, gfx::Rect const&, gfx::Rect const&, unsigned long long, gfx::AxisTransform2d const&, cc::RasterSource::PlaybackSettings const&) /usr/local/google/home/danakj/s/c/src/cc/raster/gpu_raster_buffer_provider.cc:284
000c0c07 cc::GpuRasterBufferProvider::RasterBufferImpl::Playback(cc::RasterSource const*, gfx::Rect const&, gfx::Rect const&, unsigned long long, gfx::AxisTransform2d const&, cc::RasterSource::PlaybackSettings const&) /usr/local/google/home/danakj/s/c/src/cc/raster/gpu_raster_buffer_provider.cc:114
000edf25 RunOnWorkerThread /usr/local/google/home/danakj/s/c/src/cc/tiles/tile_manager.cc:132
So it's not analysis for the first one at least.
,
May 25 2017
This is interesting. The bounds of the item match the visual rect, but the RTree search says to include it, while the canvas would reject it. 05-25 13:37:20.006 399 441 E chromium: [ERROR:display_item_list.cc(225)] Visual rect 0,383 600x135 05-25 13:37:20.053 399 441 E chromium: [ERROR:display_item_list.cc(102)] =============== Would have rejected 0,383,600,135 0
,
May 25 2017
There are others that are off by one, where the visual rect is larger than the bounds: 05-25 13:37:19.425 399 441 E chromium: [ERROR:display_item_list.cc(225)] Visual rect 16,447 117x17 05-25 13:37:19.473 399 441 E chromium: [ERROR:display_item_list.cc(102)] =============== Would have rejected 16,448,117,16 <- visual rect was 1px taller 05-25 13:37:19.473 399 441 E chromium: [ERROR:display_item_list.cc(225)] Visual rect 132,447 4x17 05-25 13:37:19.520 399 441 E chromium: [ERROR:display_item_list.cc(102)] =============== Would have rejected 132,448,4,16 <- visual rect was 1px taller
,
May 25 2017
And some others that are very different. Here the visual rect is 3px wider and 51px taller. The offset may be due to a transform, idk. 05-25 13:37:19.042 399 435 E chromium: [ERROR:display_item_list.cc(225)] Visual rect 0,0 120x67 05-25 13:37:19.042 399 441 E chromium: [ERROR:display_item_list.cc(102)] =============== Would have rejected 16,448,117,16 Here the visual rect is 18px wider but 2px *shorter* which seems particularly bad. 05-25 13:37:18.916 399 435 E chromium: [ERROR:display_item_list.cc(225)] Visual rect 4,0 22x14 05-25 13:37:18.943 399 441 E chromium: [ERROR:display_item_list.cc(102)] =============== Would have rejected 132,448,4,16
,
May 25 2017
I tried printing the visual rects from WebKit in GraphicsLayer::Paint by doing GetPaintController().ShowDebugData(), and when they don't match the clip bounds, they also don't appear to match any visual rects coming from webkit, unless they are outside the scrollback range of adb logcat. Here are my exact repro steps: ninja to build out_android/Release/system_webview_apk adb install -r -d out_android/Release/apks/SystemWebview.apk tools/perf/run_benchmark run system_health.memory_mobile --browser=android-webview --story-filter=load:media:youtube --output-format=html Note that the run_benchmark claims to try to install the Debug/ webview apk but ignore that lol? I am printing the visual rect in DisplayItemList::Raster before calling RasterItem(), and when quickReject is true, I print the cullrect on the DrawingDisplayItem. The matrix does not appear to be changed in any relevant way for any bad items, and is just the matrix set in order to raster the tile. There is a bunch of raster that happens on the main thread (?) but no visual-rect vs cull rect errors happen, presumably cuz its rastering everything so nothing would get clipped by the canvas. Whereas on the worker we're rastering tiles so things outside the tile end up intersecting the tile with their visual rect causing errors to be printed. Attached are the inputs from webkit and the worker thread.
,
May 25 2017
Across the board the memory growth looks like about 3k. My guess is that this is going into skia data structures/caches. It's possible your hypothesis is correct also wkorman@ as we do raster to analyze for solid color, and if we rejected then we would not draw something. In order for that to be true it would need to be the first drawing display item returned from the rtree, because if something else was sooner (and didn't have the same problem) we would have early outted from analysis before getting to this one. So I added prints to see and that doesn't seem to be the case, as they are index 10 and 11 in the visual rect set returns from RTree::Search. I also tried printing the results of solid tile color analysis with and without the quickReject. With the reject (before my patch): 6 tiles are found solid color in the course of loading the page. No quickRejects occur during any analysis though! Without the reject: 6 tiles are found solid color still, which makes sense since no rejects were being hit during analysis. I think the result here is that we have a few web pages that show mismatched visual rects and we should fix those. wkorman@ did not feel like it was worth reverting this in the meantime though. I will assign over with this data for more debugging of the webkit vs cc visual rect mismatch and the cull rect differences. Each of the test cases in the original perf regression should be another html page that demonstrates the same issues, but for sure the youtube one does. According to https://docs.google.com/spreadsheets/d/1t15Ya5ssYBeXAZhHm3RJqfwBRpgWsxoib8_kwQEHMwI/edit#gid=0 the page it loads (in mobile UserAgent) is https://www.youtube.com/watch?v=QGfhS1hfTWw&autoplay=false
,
May 25 2017
,
May 26 2017
,
May 26 2017
,
May 26 2017
,
May 27 2017
,
May 30 2017
,
May 30 2017
Issue 727479 has been merged into this issue.
,
May 30 2017
,
Jun 9 2017
,
Jun 12 2017
I am OOO soon for a few weeks. If this should be looked at before then please find an alternate owner through chrishtr@.
,
Jun 14 2017
Issue 726920 has been merged into this issue.
,
Jun 14 2017
Issue 726919 has been merged into this issue.
,
Aug 7 2017
The involved DisplayItemList logic looks to have been notably reworked as part of related paint op buffer and display item serialization sub project work. For this bit: https://chromium-review.googlesource.com/c/506519/13/cc/paint/display_item_list.cc#b101 It now looks like the equivalent draw playback happens in DrawRecordOp::Raster. The only callsite of that is cc::Rasterizer::Raster. It's not clear to me where we would get the new equivalent of what was item.bounds to check with canvas quickReject. In the end all of the above investigation essentially shows that there is a mismatch between visual rect and cull rect in, at minimum: https://www.youtube.com/watch?v=QGfhS1hfTWw&autoplay=false Potentially only under a mobile user agent, or maybe only on an actual device. It's not clear to me whether issue was reproducible only on device or with dev tools in mobile UA mode. The worst regression in the linked perf charts is ~11% or 2.5 MB RAM on an N6 for News: https://chromeperf.appspot.com/report?sid=41a67c533638af750b52c114c766a1db098b9575a6bdd42c02c5b18254f0ba29&rev=474004 There are three YT regressions that are more like 1 - 3K as noted previously, and some others. Priority is not clear to me and I'm also not sure where best place is to explore canvas cull rect comparatively. Thought to seek checkpoint from others.
,
Aug 25 2017
Closing as WontFix since I believe we are moving away from using cull rects as we did previously, so mismatch is an obsolete issue. Given no feedback from above comment, please reopen with further commentary if warranted. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by pmeenan@chromium.org
, May 24 2017