Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039373003:
Temporarily disable ClipPathDisplayItem accounting for GPU veto (Closed)
|
Created:
4 years, 6 months ago by f(malita) Modified:
4 years, 4 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, bsalomon_chromium, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, reed1, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTemporarily disable ClipPathDisplayItem accounting for GPU veto
The GPU veto is persistent per root layer. Reverting
http://crrev.com/2013723002 while we sort some regressions out.
BUG=603969
R=senorblanco@chromium.org,chrishtr@chromium.org
Committed: https://crrev.com/4e4d5e1f086786afff93d0d7e0a6eb5f5fcb276f
Cr-Commit-Position: refs/heads/master@{#398371}
Patch Set 1 #Patch Set 2 : disable the unit test also #
Messages
Total messages: 16 (4 generated)
Description was changed from ========== Temporarily disable ClipPathDisplayItem accounting for GPU veto The GPU veto is persistent per root layer. Reverting http://crrev.com/2013723002 while we sort some regressions out. BUG=603969 R=senorblanco@chromium.org,chrishtr@chromium.org ========== to ========== Temporarily disable ClipPathDisplayItem accounting for GPU veto The GPU veto is persistent per root layer. Reverting http://crrev.com/2013723002 while we sort some regressions out. BUG=603969 R=senorblanco@chromium.org,chrishtr@chromium.org ==========
LGTM
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039373003/20001
Message was sent while issue was closed.
Description was changed from ========== Temporarily disable ClipPathDisplayItem accounting for GPU veto The GPU veto is persistent per root layer. Reverting http://crrev.com/2013723002 while we sort some regressions out. BUG=603969 R=senorblanco@chromium.org,chrishtr@chromium.org ========== to ========== Temporarily disable ClipPathDisplayItem accounting for GPU veto The GPU veto is persistent per root layer. Reverting http://crrev.com/2013723002 while we sort some regressions out. BUG=603969 R=senorblanco@chromium.org,chrishtr@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Temporarily disable ClipPathDisplayItem accounting for GPU veto The GPU veto is persistent per root layer. Reverting http://crrev.com/2013723002 while we sort some regressions out. BUG=603969 R=senorblanco@chromium.org,chrishtr@chromium.org ========== to ========== Temporarily disable ClipPathDisplayItem accounting for GPU veto The GPU veto is persistent per root layer. Reverting http://crrev.com/2013723002 while we sort some regressions out. BUG=603969 R=senorblanco@chromium.org,chrishtr@chromium.org Committed: https://crrev.com/4e4d5e1f086786afff93d0d7e0a6eb5f5fcb276f Cr-Commit-Position: refs/heads/master@{#398371} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4e4d5e1f086786afff93d0d7e0a6eb5f5fcb276f Cr-Commit-Position: refs/heads/master@{#398371}
Message was sent while issue was closed.
I think we could consider re-enabling this change now that the veto is less "sticky": https://codereview.chromium.org/2108033004/ Note that there is some latency in switching the veto off (currently 60 frames), so we should investigate how this behaves in the problematic test cases.
Message was sent while issue was closed.
On 2016/07/21 14:57:45, Stephen White wrote: > I think we could consider re-enabling this change now that the veto is less > "sticky": https://codereview.chromium.org/2108033004/ > > Note that there is some latency in switching the veto off (currently 60 frames), > so we should investigate how this behaves in the problematic test cases. Makes sense, thanks for the reminder. I ran a quick test, and unfortunately the behavior has changed: turning the clip analyzer on seems to make things worse now (not vetoing anymore?). Not sure what's going on yet, but I noticed there was CL which shuffled the analyzer logic around (https://codereview.chromium.org/2107103002). I'll look into it.
Message was sent while issue was closed.
On 2016/07/22 14:46:54, f(malita) wrote: > On 2016/07/21 14:57:45, Stephen White wrote: > > I think we could consider re-enabling this change now that the veto is less > > "sticky": https://codereview.chromium.org/2108033004/ > > > > Note that there is some latency in switching the veto off (currently 60 > frames), > > so we should investigate how this behaves in the problematic test cases. > > Makes sense, thanks for the reminder. > > I ran a quick test, and unfortunately the behavior has changed: turning the clip > analyzer on seems to make things worse now (not vetoing anymore?). > > Not sure what's going on yet, but I noticed there was CL which shuffled the > analyzer logic around (https://codereview.chromium.org/2107103002). I'll look > into it. If the clip paths are volatile, it might be Brian's change which made the tessellating path renderer skip paths which don't have a key (e.g., volatile paths): https://codereview.chromium.org/2064753003 I originally thought this was a good idea, but there are actually a number of cases where it outperforms, even when tessellating, e.g., filled paths in canvas. This might be one of them.
Message was sent while issue was closed.
Interesting... any way to identity cases where volatile paths are worth tessellating? On Fri, Jul 22, 2016 at 11:43 AM <senorblanco@chromium.org> wrote: > On 2016/07/22 14:46:54, f(malita) wrote: > > On 2016/07/21 14:57:45, Stephen White wrote: > > > I think we could consider re-enabling this change now that the veto is > less > > > "sticky": https://codereview.chromium.org/2108033004/ > > > > > > Note that there is some latency in switching the veto off (currently 60 > > frames), > > > so we should investigate how this behaves in the problematic test > cases. > > > > Makes sense, thanks for the reminder. > > > > I ran a quick test, and unfortunately the behavior has changed: turning > the > clip > > analyzer on seems to make things worse now (not vetoing anymore?). > > > > Not sure what's going on yet, but I noticed there was CL which shuffled > the > > analyzer logic around (https://codereview.chromium.org/2107103002). > I'll look > > into it. > > If the clip paths are volatile, it might be Brian's change which made the > tessellating path renderer skip paths which don't have a key (e.g., > volatile paths): https://codereview.chromium.org/2064753003 > > I originally thought this was a good idea, but there are actually a number > of > cases where it outperforms, even when tessellating, e.g., filled paths in > canvas. This might be one of them. > > https://codereview.chromium.org/2039373003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Interesting... any way to identity cases where volatile paths are worth tessellating? On Fri, Jul 22, 2016 at 11:43 AM <senorblanco@chromium.org> wrote: > On 2016/07/22 14:46:54, f(malita) wrote: > > On 2016/07/21 14:57:45, Stephen White wrote: > > > I think we could consider re-enabling this change now that the veto is > less > > > "sticky": https://codereview.chromium.org/2108033004/ > > > > > > Note that there is some latency in switching the veto off (currently 60 > > frames), > > > so we should investigate how this behaves in the problematic test > cases. > > > > Makes sense, thanks for the reminder. > > > > I ran a quick test, and unfortunately the behavior has changed: turning > the > clip > > analyzer on seems to make things worse now (not vetoing anymore?). > > > > Not sure what's going on yet, but I noticed there was CL which shuffled > the > > analyzer logic around (https://codereview.chromium.org/2107103002). > I'll look > > into it. > > If the clip paths are volatile, it might be Brian's change which made the > tessellating path renderer skip paths which don't have a key (e.g., > volatile paths): https://codereview.chromium.org/2064753003 > > I originally thought this was a good idea, but there are actually a number > of > cases where it outperforms, even when tessellating, e.g., filled paths in > canvas. This might be one of them. > > https://codereview.chromium.org/2039373003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/07/22 17:23:32, bsalomon_chromium wrote: > Interesting... any way to identity cases where volatile paths are worth > tessellating? In the Animometer canvas suite, many path-related tests got faster with MSAA (and tessellation) on, and no tests got slower. I believe all the paths are volatile, since they use the traditional, non-persistent-path-based API in canvas. I haven't retried those tests since the above change landed, but my guess is that the performance has regressed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
