Clamps to invalidation region of a PictureLayer to the layer bounds to avoid adding unecessary bulk

Clamp invalidation regions of PictureLayer to layer bounds

Clamps the incoming invalidation regions of PictureLayer to the actual layer bounds to avoid bloating the update region data structure. It also adds an early out when the invalidation rect is empty. 

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175952

[vangelis, 2013-01-09 00:28:06]

Small patch, ready for review.

Is contentBounds() the right bounds to be clamping the invalidation to?

[danakj, 2013-01-09 00:31:57]

https://codereview.chromium.org/11824013/diff/1/cc/picture_layer.cc
File cc/picture_layer.cc (right):

https://codereview.chromium.org/11824013/diff/1/cc/picture_layer.cc#newcode54
cc/picture_layer.cc:54: if (!rect.IsEmpty()) {
you could also rect.Intersect(gfx::Rect(contentBounds()) to do this on one line.

https://codereview.chromium.org/11824013/diff/1/cc/picture_layer.cc#newcode60
cc/picture_layer.cc:60: pending_invalidation_.Union(clamped_rect);
union with an empty rect is a no-op, so you probably don't need the if()?

[vangelis, 2013-01-09 00:57:14]

Thanks for the review!

On 2013/01/09 00:31:57, danakj wrote:
> https://codereview.chromium.org/11824013/diff/1/cc/picture_layer.cc
> File cc/picture_layer.cc (right):
> 
> https://codereview.chromium.org/11824013/diff/1/cc/picture_layer.cc#newcode54
> cc/picture_layer.cc:54: if (!rect.IsEmpty()) {
> you could also rect.Intersect(gfx::Rect(contentBounds()) to do this on one
line.
> 
> 

Done!

https://codereview.chromium.org/11824013/diff/1/cc/picture_layer.cc#newcode60
> cc/picture_layer.cc:60: pending_invalidation_.Union(clamped_rect);
> union with an empty rect is a no-op, so you probably don't need the if()?

Done!

[danakj, 2013-01-09 01:00:05]

https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc
File cc/picture_layer.cc (right):

https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc#newcode54
cc/picture_layer.cc:54: if (!rect.IsEmpty()) {
i think this if() is kinda surplus too, everything should be no-ops without it
when the rect is empty, unless we're seeing some perf hit from this (seems
unlikely in this method?)

https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc#newcode56
cc/picture_layer.cc:56: rect.Intersect(gfx::Rect(contentBounds()));
Sorry to miss this last round: You want bounds() here actually, not
contentBounds(). This is all in layer space (hence layer_rect's name)

[danakj, 2013-01-09 01:01:04]

Also, description nit: can you make your description format as:

<one line single sentence>
<blank>
<longer multi-line thing>

It'll make the git log much cleaner.

[vangelis, 2013-01-09 01:50:36]

https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc
File cc/picture_layer.cc (right):

https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc#newcode54
cc/picture_layer.cc:54: if (!rect.IsEmpty()) {
On 2013/01/09 01:00:05, danakj wrote:
> i think this if() is kinda surplus too, everything should be no-ops without it
> when the rect is empty, unless we're seeing some perf hit from this (seems
> unlikely in this method?)

it is indeed a no-op but still goes though a surprising amount of code through
SkRegion with several on-stack allocations. This method gets called quite a bit
with empty rects, when the assorted layer properties are being setup. Probably
not a significant win perf-wise but since I was passing by...

https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc#newcode56
cc/picture_layer.cc:56: rect.Intersect(gfx::Rect(contentBounds()));
On 2013/01/09 01:00:05, danakj wrote:
> Sorry to miss this last round: You want bounds() here actually, not
> contentBounds(). This is all in layer space (hence layer_rect's name)

Done.

[danakj, 2013-01-09 01:51:48]

LGTM

https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc
File cc/picture_layer.cc (right):

https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc#newcode54
cc/picture_layer.cc:54: if (!rect.IsEmpty()) {
On 2013/01/09 01:50:36, vangelis wrote:
> On 2013/01/09 01:00:05, danakj wrote:
> > i think this if() is kinda surplus too, everything should be no-ops without
it
> > when the rect is empty, unless we're seeing some perf hit from this (seems
> > unlikely in this method?)
> 
> it is indeed a no-op but still goes though a surprising amount of code through
> SkRegion with several on-stack allocations. This method gets called quite a
bit
> with empty rects, when the assorted layer properties are being setup. Probably
> not a significant win perf-wise but since I was passing by...
> 

If it's an issue, maybe we should add it to cc::Region instead, so all callsites
can benefit? I'll leave that up to you.

[commit-bot: I haz the power, 2013-01-09 20:34:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vangelis@chromium.org/11824013/4002

[vangelis, 2013-01-09 20:36:16]

On 2013/01/09 01:51:48, danakj wrote:
> LGTM
> 
> https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc
> File cc/picture_layer.cc (right):
> 
>
https://codereview.chromium.org/11824013/diff/5001/cc/picture_layer.cc#newcode54
> cc/picture_layer.cc:54: if (!rect.IsEmpty()) {
> On 2013/01/09 01:50:36, vangelis wrote:
> > On 2013/01/09 01:00:05, danakj wrote:
> > > i think this if() is kinda surplus too, everything should be no-ops
without
> it
> > > when the rect is empty, unless we're seeing some perf hit from this (seems
> > > unlikely in this method?)
> > 
> > it is indeed a no-op but still goes though a surprising amount of code
through
> > SkRegion with several on-stack allocations. This method gets called quite a
> bit
> > with empty rects, when the assorted layer properties are being setup.
Probably
> > not a significant win perf-wise but since I was passing by...
> > 
> 
> If it's an issue, maybe we should add it to cc::Region instead, so all
callsites
> can benefit? I'll leave that up to you.

setNeedsDisplayRect() is somewhat unique in that it gets called with empty rects
often. I'll keep the check here instead of moving it up to cc::Region to avoid
adding a branch on all Region calls which should in theory only infrequently get
empty rects.

[commit-bot: I haz the power, 2013-01-10 00:54:59]

Change committed as 175952