Issue metadata
Sign in to add a comment
|
17.3%-71.9% regression in rendering.desktop at 587297:587355 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Aug 30
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/118538a3640000
,
Aug 31
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/118538a3640000 Managing the context was dropped. Adding back in. by herb@google.com https://skia.googlesource.com/skia/+/aca2b9eebdaf5ec45ba87a25c60510988fc54756 0.2373 → 0.3473 (+0.11) Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Aug 31
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/15b7402e640000
,
Aug 31
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/15863d5d640000
,
Aug 31
📍 Couldn't reproduce a difference. https://pinpoint-dot-chromeperf.appspot.com/job/15863d5d640000
,
Aug 31
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/15b7402e640000 Managing the context was dropped. Adding back in. by herb@google.com https://skia.googlesource.com/skia/+/aca2b9eebdaf5ec45ba87a25c60510988fc54756 0.2566 → 0.3523 (+0.09567) Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Aug 31
Hey Khushal and Mike, An interesting point. It looks like deleting and creating the context may take significant time.
,
Aug 31
I was surprised we didn't see an improvement from your initial patch that removed it, if that were the case. And I remembered that we only used to create the context if any glyphs had to be serialized (see https://skia-review.googlesource.com/c/skia/+/149041/4/src/core/SkRemoteGlyphCache.cpp#b569). But now we also do it even on a cache hit, so we're creating it at least once for every strike on a tile. Could we go back to the old way of creating it only when required?
,
Sep 4
Yeah. This sounds like a good plan. I will work on that next week. This week I'm letting all the changes I made back for M70. I'll leave this open as a reminder.
,
Sep 17
The following revision refers to this bug: https://skia.googlesource.com/skia/+/48415e96cf5c50d3ced351f0f4d3b85ea2349610 commit 48415e96cf5c50d3ced351f0f4d3b85ea2349610 Author: Herb Derby <herb@google.com> Date: Mon Sep 17 15:15:01 2018 Simplify descriptors and thighten context lifetime This should have the same context lifetimes as the orignal (Khushal's) code. This still works with the shared GPU/Remote rendering framework. BUG=chromium:879321 Change-Id: I5e346b2c2eb5868d7b37b08ba769f0e52d29f185 Reviewed-on: https://skia-review.googlesource.com/153740 Reviewed-by: Khusal Sagar <khushalsagar@chromium.org> Commit-Queue: Herb Derby <herb@google.com> [modify] https://crrev.com/48415e96cf5c50d3ced351f0f4d3b85ea2349610/src/core/SkRemoteGlyphCache.cpp [modify] https://crrev.com/48415e96cf5c50d3ced351f0f4d3b85ea2349610/src/core/SkRemoteGlyphCacheImpl.h
,
Sep 17
,
Sep 17
,
Sep 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6dde2561e844b56f4a02712e7b23f3d40f955d1c commit 6dde2561e844b56f4a02712e7b23f3d40f955d1c Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Date: Mon Sep 17 18:00:02 2018 Roll src/third_party/skia 3a039d59df1c..49ec21d7cc1b (7 commits) https://skia.googlesource.com/skia.git/+log/3a039d59df1c..49ec21d7cc1b git log 3a039d59df1c..49ec21d7cc1b --date=short --no-merges --format='%ad %ae %s' 2018-09-17 csmartdalton@google.com ccpr: Don't require flat interpolation 2018-09-17 herb@google.com Simplify descriptors and thighten context lifetime 2018-09-17 benjaminwagner@google.com Update OS on GalaxyS7. 2018-09-17 robertphillips@google.com Always reset the SkPathRef's RRect and Oval-ness flags in growForVerb and growForRepeatedVerb 2018-09-17 robertphillips@google.com Cleanup SkCornerPathEffect a bit 2018-09-17 robertphillips@google.com Make a moveTo after an SkPath::addRRect invalidate the rrect-ness of the path 2018-09-17 robertphillips@google.com Fix SkRRect::setRectXY case where rescaling results in a rect Created with: gclient setdep -r src/third_party/skia@49ec21d7cc1b The AutoRoll server is located here: https://autoroll.skia.org/r/skia-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel BUG=chromium:879321 TBR=reed@chromium.org Change-Id: I7ce2169f4f00b7f1800a8ef4dd6f793ea95310e0 Reviewed-on: https://chromium-review.googlesource.com/1228281 Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#591736} [modify] https://crrev.com/6dde2561e844b56f4a02712e7b23f3d40f955d1c/DEPS
,
Sep 21
Looks like this didn't completely recover. :/ We should verify that we are no worse here than before: https://skia-review.googlesource.com/c/skia/+/149041, since the patch in #11 should have restored us back to that behaviour.
,
Sep 21
I've attached a screenshot of one of the charts... Am I right thinking that the left of the chart was our old steady state, the first green (i) was a CL that made things fast but we couldn't live with for increased memory usage, the first black (!) is our original attempt to fix that, and the second green (i) was our final CL from #11? If so, all looks good, right?
,
Sep 21
So I'm looking at the graph for mac-10_12_laptop_low_end-perf/rendering.desktop/dynamic_cube_map and here is the timeline of patches I'm looking for: 1) Simplify lifetime of scaler context for remote cache This made it so we keep the context cached on the SkGlyphCacheState and rolled into chromium at r585752. This did cause a memory increase but I don't see any perf improvement in raster time on this benchmark from it. The range for the first improvement is 585104 - 585139. 2) Managing the context was dropped. Adding back in. This made it so we don't cache the context but caused us to recreate it for every descriptor on a tile, even if all glyphs were already cached so the context wasn't required. This wasn't the behaviour prior to 1). This rolled into chromium at r587342 and caused us to go from ~0.23 to ~0.36 ms raster_cpu_time_per_frame 3) Simplify descriptors and thighten context lifetime This was supposed to fix 2) and rolled into chromium at r591736. There is an improvement with this change, we are at ~0.27 now. But it didn't recover completely. I would've expected it to recover the regression in 1) completely. Did I miss something?
,
Sep 24
Nope, I think I was confused.
,
Sep 26
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/16d3a6d0e40000
,
Sep 26
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/16d3a6d0e40000 Managing the context was dropped. Adding back in. by herb@google.com https://skia.googlesource.com/skia/+/aca2b9eebdaf5ec45ba87a25c60510988fc54756 0.2538 → 0.3368 (+0.08295) Understanding performance regressions: http://g.co/ChromePerformanceRegressions Benchmark documentation link: https://bit.ly/rendering-benchmarks
,
Sep 26
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/11b6ab67640000
,
Sep 26
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/11b6ab67640000 Managing the context was dropped. Adding back in. by herb@google.com https://skia.googlesource.com/skia/+/aca2b9eebdaf5ec45ba87a25c60510988fc54756 0.2442 → 0.3346 (+0.0904) Understanding performance regressions: http://g.co/ChromePerformanceRegressions Benchmark documentation link: https://bit.ly/rendering-benchmarks |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Aug 30