Bug 82423 – [chromium] Record the number of tiles paint-culled instead of the incorrect number of pixels

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Record the number of tiles paint-culled instead of the incorrect number of pixels

[Dana Jansens]

We currently attempt to record the number of pixels we save from uploading, however the number is wrong in two ways.
1) An early-out if everything is culled results in nothing being reported.
2) With threaded-compositor, the number of pixels uploaded in one tile can be changed by culling in another tile, making the number far too difficult to compute to be worthwhile. If a tile was going to be partial-updated, but is culled, now a new tile gets to be partial-updated instead, which affects the numbers.

[Dana Jansens]

Created attachment 134214 [details]
Patch

[Adrienne Walker]

Comment on attachment 134214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134214&action=review

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:435
> +                    occlusion->overdrawMetrics().didCullTileForUpload(tile->isDirty());

What happens if this layer OOMs and gets skipped? Is it wrong to report this during the first pass?

> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.h:45
> +    // Records that an invalid tile was culled and did not need to be painted/uploaded, and did not contribute to other tiles needing to be painted.
> +    void didCullTileForUpload(bool dirty);

I'm kind of confused about dirty as a parameter here.  How about just calling didCullTileForUpload only when it was dirty and would have needed to be painted/uploaded?

> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.h:89
> +    // Count of pixels uploaded to textures and known to be opaque.
> +    float m_pixelsUploadedOpaque;
> +    // Count of pixels uploaded to textures and not known to be opaque.
> +    float m_pixelsUploadedTranslucent;
> +    // Count of tiles that were invalidated but not uploaded.
> +    int m_tilesCulledForUpload;

This class is starting to feel overloaded.  If the main thread metrics and the impl thread metrics have different needs, do you think it'd be clearer to have different classes, rather than just depending on variables being unset to not record them on a particular thread?

[Dana Jansens]

Comment on attachment 134214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134214&action=review

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:435
>> +                    occlusion->overdrawMetrics().didCullTileForUpload(tile->isDirty());
> 
> What happens if this layer OOMs and gets skipped? Is it wrong to report this during the first pass?

Good question. I'll stick this in the second loop where we copyAndClearDirty(). This happens before the early out, so win.

>> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.h:45
>> +    void didCullTileForUpload(bool dirty);
> 
> I'm kind of confused about dirty as a parameter here.  How about just calling didCullTileForUpload only when it was dirty and would have needed to be painted/uploaded?

Ok. I kind of thought maybe in the future we'd use it or something, but we're not now. Removed.

>> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.h:89
>> +    int m_tilesCulledForUpload;
> 
> This class is starting to feel overloaded.  If the main thread metrics and the impl thread metrics have different needs, do you think it'd be clearer to have different classes, rather than just depending on variables being unset to not record them on a particular thread?

Trying hard to avoid code complexity where not needed.. altho the name is less fitting. CCFrameMetrics? I'm not sure if that matters really..

Splitting it into two classes would require two objects in all the public API of occlusion tracker, or down-casting before using it. I could stick the floats into two nested structs to group them better conceptually as "These are all used together"? Wdyt of this all?

[Adrienne Walker]

(In reply to comment #4)
> (From update of attachment 134214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134214&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.h:89
> >> +    int m_tilesCulledForUpload;
> > 
> > This class is starting to feel overloaded.  If the main thread metrics and the impl thread metrics have different needs, do you think it'd be clearer to have different classes, rather than just depending on variables being unset to not record them on a particular thread?
> 
> Trying hard to avoid code complexity where not needed.. altho the name is less fitting. CCFrameMetrics? I'm not sure if that matters really..
> 
> Splitting it into two classes would require two objects in all the public API of occlusion tracker, or down-casting before using it. I could stick the floats into two nested structs to group them better conceptually as "These are all used together"? Wdyt of this all?

Hmm, good point.  For the sake of being able to use the same overdraw metrics class all over, I think I'd be ok with any increased clarity about which functions are ok to call in which contexts.  Grouping would help, either as a a struct or with better comments and separation in the header.  Maybe leaving MetricsType as an explict flag would also help, rather than just implicitly uploading anything that was non-zero?

[Dana Jansens]

(In reply to comment #5)
> Hmm, good point.  For the sake of being able to use the same overdraw metrics class all over, I think I'd be ok with any increased clarity about which functions are ok to call in which contexts.  Grouping would help, either as a a struct or with better comments and separation in the header.  Maybe leaving MetricsType as an explict flag would also help, rather than just implicitly uploading anything that was non-zero?

Thanks. Kept the MetricsType, I think that's a great idea. The code is mode clear with that. And separated the methods/member variables with comments.

[Dana Jansens]

Created attachment 134668 [details]
Patch

[Adrienne Walker]

Comment on attachment 134668 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134668&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.cpp:133
> +    // This gives approximately 100x the percentage of tiles to fill the viewport once, if all tiles were 256x256.
> +    float tileNormalization = 10000.f / (layerTreeHost->viewportSize().width() / 256.f * layerTreeHost->viewportSize().height() / 256.f);

This is somewhat bikeshedding, but is there a reason not to normalize this with the same multiplier as the above? It seems reasonable that somebody is going to compare them?

(Also, if you're going to record pixel counts here, maybe you should just pass in tile size in didCullTileForUpload in case we hypothetically ever change the tile size and wanted to compare histogram values across time? Or to be slightly more accurate for layers that don't use that tile size?)

[Dana Jansens]

Comment on attachment 134668 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134668&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.cpp:133
>> +    float tileNormalization = 10000.f / (layerTreeHost->viewportSize().width() / 256.f * layerTreeHost->viewportSize().height() / 256.f);
> 
> This is somewhat bikeshedding, but is there a reason not to normalize this with the same multiplier as the above? It seems reasonable that somebody is going to compare them?
> 
> (Also, if you're going to record pixel counts here, maybe you should just pass in tile size in didCullTileForUpload in case we hypothetically ever change the tile size and wanted to compare histogram values across time? Or to be slightly more accurate for layers that don't use that tile size?)

Comparing the number of tiles to the number of pixels can't be done 1:1 anyhow. I made the tile number an order of magnitude larger because painting is often going to be only a fraction of the viewport, rather than always filling at least the whole viewport. And I wanted to prevent numbers from being cut off. In this case, 100 is the min value, so 1% of the tiles to fill the viewport being culled will still be recorded (as 100).

I am hesitant to record record pixels here because they would be wrong and I don't want them to be misconstrued. In a partial update, the whole tile was not going to get updated, but the whole tile's pixel count would be recorded. It reports distorted numbers for which the distortion is non-obvious. At least in counting tiles it gives something we can actually use.

[David Reveman]

Rather than trying to record the number of pixels or tiles saved/added by some specific feature like culling, per-tile painting or atomic commits. Would it make more sense to just record something a bit more high level that would give us an idea of how the overall system performs, like the number of pixels painted and pixels invalidated?

And if you're interested in the effect of a specific feature you compare the metrics from when it's turned on vs off.

[Dana Jansens]

(In reply to comment #10)
> Rather than trying to record the number of pixels or tiles saved/added by some specific feature like culling, per-tile painting or atomic commits. Would it make more sense to just record something a bit more high level that would give us an idea of how the overall system performs, like the number of pixels painted and pixels invalidated?
> 
> And if you're interested in the effect of a specific feature you compare the metrics from when it's turned on vs off.

Yes, we're already recording pixels painted. The idea for this is just to tell if culling is happening on a page when looking at the trace. Not really for performance, so I think counting tiles is pretty okay.

[Adrienne Walker]

Comment on attachment 134668 [details]
Patch

Ok, sure.  If it's just meant to give a rough idea of how much culling is occurring, then that seems reasonable.  The value's a little confusing, but I agree that there's not a great answer of what that value should be.

[WebKit Review Bot]

Comment on attachment 134668 [details]
Patch

Rejecting attachment 134668 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
nk #14 succeeded at 1219 (offset -188 lines).
Hunk #15 succeeded at 1233 (offset -188 lines).
Hunk #16 succeeded at 1245 (offset -188 lines).
Hunk #17 succeeded at 1259 (offset -188 lines).
Hunk #18 succeeded at 1273 (offset -188 lines).
Hunk #19 succeeded at 1312 (offset -188 lines).
Hunk #20 succeeded at 1328 (offset -188 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adrienne W..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12264426

[Dana Jansens]

Created attachment 135873 [details]
Patch

rebased

[WebKit Review Bot]

Comment on attachment 135873 [details]
Patch

Clearing flags on attachment: 135873

Committed r113372: <http://trac.webkit.org/changeset/113372>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.