Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031243003:
SkPictureAnalyzer: expose the number of slow GPU commands. (Closed)
|
Created:
4 years, 6 months ago by Stephen White Modified:
4 years, 6 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPictureAnalyzer: expose the number of slow GPU commands.
BUG=614724
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031243003
Committed: https://skia.googlesource.com/skia/+/ea9767356385866263a3164dcc1fb9e1633f57ef
Patch Set 1 #Patch Set 2 : Remove early-out veto #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== SkPictureAnalyzer: expose the number of slow GPU commands. BUG=skia:614724 ========== to ========== SkPictureAnalyzer: expose the number of slow GPU commands. BUG=skia:614724 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031243003 ==========
Description was changed from ========== SkPictureAnalyzer: expose the number of slow GPU commands. BUG=skia:614724 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031243003 ========== to ========== SkPictureAnalyzer: expose the number of slow GPU commands. BUG=614724 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031243003 ==========
senorblanco@chromium.org changed reviewers: + bsalomon@google.com
Brian: PTAL. Thanks!
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031243003/1
reed@google.com changed reviewers: + fmalita@chromium.org, reed@google.com
Is this related to https://codereview.chromium.org/2000423007/ ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/03 19:37:18, reed1 wrote: > Is this related to https://codereview.chromium.org/2000423007/ ? Yep, this is me "asking". Also see attached bug.
I'm out with the new call. However, it may give misleading information. Chrome stops calling the analyzer once enough pictures have been poured into the Analyzer to deem the content not suitable. Similarly, the analyzer stops counting as soon as the threshold is hit. So tracing (or some other debugging presentation) may indicate that the page is not suitable because some particular element had 5 slow ops. However, it could be the case that the whole page is full of slow ops that weren't counted.
On 2016/06/06 18:08:13, bsalomon wrote: > I'm out with the new call. However, it may give misleading information. Chrome > stops calling the analyzer once enough pictures have been poured into the > Analyzer to deem the content not suitable. Similarly, the analyzer stops > counting as soon as the threshold is hit. So tracing (or some other debugging > presentation) may indicate that the page is not suitable because some particular > element had 5 slow ops. However, it could be the case that the whole page is > full of slow ops that weren't counted. Yeah, that's why the docs say "capped at the predicate max". I'm not sure what to do about this. In order to get an accurate count, we'd have to skip the early out, and count them all. Which could impact performance. Was this early-out in response to a measured perf hit, or was it pre-emptive? Could we try turning it off and watch the perf results?
On 2016/06/06 19:34:06, Stephen White wrote: > On 2016/06/06 18:08:13, bsalomon wrote: > > I'm out with the new call. However, it may give misleading information. Chrome > > stops calling the analyzer once enough pictures have been poured into the > > Analyzer to deem the content not suitable. Similarly, the analyzer stops > > counting as soon as the threshold is hit. So tracing (or some other debugging > > presentation) may indicate that the page is not suitable because some > particular > > element had 5 slow ops. However, it could be the case that the whole page is > > full of slow ops that weren't counted. > > Yeah, that's why the docs say "capped at the predicate max". > > I'm not sure what to do about this. In order to get an accurate count, we'd have > to skip the early out, and count them all. Which could impact performance. Was > this early-out in response to a measured perf hit, or was it pre-emptive? > Could we try turning it off and watch the perf results? It was added preemptively, so yeah, we should experiment and see what the impact is. There's also some redundancy: both DisplayItemList and SkPictureGpuAnalyzer check for the early out condition. The former was added later, to avoid the DisplayItem:analyzeForGpuRasterization() virtual dispatch when the veto answer is known. So I think it should be low risk to remove the SkPictureGpuAnalyzer checks, but we should run the DisplayItemList change through Cluster Telemetry. Somewhat related: it's unfortunate that we run the analysis indiscriminately, even when GPU rasterization is not available. But now that the logic lives in Blink, plumbing GPU info is kinda awkward.
On 2016/06/07 00:53:08, f(malita) wrote: > On 2016/06/06 19:34:06, Stephen White wrote: > > On 2016/06/06 18:08:13, bsalomon wrote: > > > I'm out with the new call. However, it may give misleading information. > Chrome > > > stops calling the analyzer once enough pictures have been poured into the > > > Analyzer to deem the content not suitable. Similarly, the analyzer stops > > > counting as soon as the threshold is hit. So tracing (or some other > debugging > > > presentation) may indicate that the page is not suitable because some > > particular > > > element had 5 slow ops. However, it could be the case that the whole page is > > > full of slow ops that weren't counted. > > > > Yeah, that's why the docs say "capped at the predicate max". > > > > I'm not sure what to do about this. In order to get an accurate count, we'd > have > > to skip the early out, and count them all. Which could impact performance. Was > > this early-out in response to a measured perf hit, or was it pre-emptive? > > Could we try turning it off and watch the perf results? > > It was added preemptively, so yeah, we should experiment and see what the impact > is. > > There's also some redundancy: both DisplayItemList and SkPictureGpuAnalyzer > check for the early out condition. The former was added later, to avoid the > DisplayItem:analyzeForGpuRasterization() virtual dispatch when the veto answer > is known. > > So I think it should be low risk to remove the SkPictureGpuAnalyzer checks, but > we should run the DisplayItemList change through Cluster Telemetry. > > Somewhat related: it's unfortunate that we run the analysis indiscriminately, > even when GPU rasterization is not available. But now that the logic lives in > Blink, plumbing GPU info is kinda awkward. Could we remove the short circuit in the SkPictureAnalyzer as part of this change? I think it would make this call clearer. I don't think it'd hurt anything in Chrome as slimming paint produces tiny pictures.
On 2016/06/07 19:34:34, bsalomon wrote: > Could we remove the short circuit in the SkPictureAnalyzer as part of this > change? I think it would make this call clearer. I don't think it'd hurt > anything in Chrome as slimming paint produces tiny pictures. Done. New patch up.
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031243003/20001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/09 18:40:40, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. lgtm
The CQ bit was checked by senorblanco@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031243003/20001
Message was sent while issue was closed.
Description was changed from ========== SkPictureAnalyzer: expose the number of slow GPU commands. BUG=614724 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031243003 ========== to ========== SkPictureAnalyzer: expose the number of slow GPU commands. BUG=614724 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031243003 Committed: https://skia.googlesource.com/skia/+/ea9767356385866263a3164dcc1fb9e1633f57ef ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/ea9767356385866263a3164dcc1fb9e1633f57ef |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
