ui: Cache the output of View::OnPaint when the View isn't invalid.

ui: Cache the output of View::OnPaint when the View isn't invalid.

Store the output of View::OnPaint as a PaintCache object, and when the
view has not been invalidated, use that cache output instead of doing
work to build a recording.

Performance data as follows when the loading spinner is going:

Before impl-side (TOT): 0.13ms / paint
With impl-side (no slimming paint): 0.22ms / paint
With impl-side and slimming paint (this patch): 0.17ms / paint

So this gets us some of the way there. I need to investigate why it's
not doing more.

R=piman@chromium.org, sky
BUG=466426
Committed: https://crrev.com/7f686cdcff81d6779b962e98e529a7360be2809c
Cr-Commit-Position: refs/heads/master@{#326592}

Committed: https://crrev.com/93f5730af51af283713ad19dd5e08c3b3ea34eaa
Cr-Commit-Position: refs/heads/master@{#326976}

[danakj, 2015-04-22 23:31:54]

Depends on: https://codereview.chromium.org/1080633009/

[danakj, 2015-04-22 23:40:28]

Patchset #2 (id:20001) has been deleted

[sky, 2015-04-23 16:29:00]

I think this should be behind a flag until it's ready.

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:754: gfx::IntersectRects(damaged_region_.bounds(),
bounds());
Isn't bounds() in the parent coordinate space and damaged_region_ in this layers
coordinate space?

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_cac...
File ui/compositor/paint_cache.h (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_cac...
ui/compositor/paint_cache.h:16: class Rect;
nit: you don't use rect here (or need the rect include in the .cc).

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_cac...
ui/compositor/paint_cache.h:30: bool UseCache(const PaintContext& context);
Document what this does and what the return value means.

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_con...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_con...
ui/compositor/paint_context.h:66: bool EarlyOutOfPaintingWhenNotInvalidated()
const { return !!canvas_; }
Oy! The double negatives make this hard to parse (I'm referring to not invalid,
not !!). EarlyOutOfPaintingWhenValid() is easier to grok.

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.h:34: PaintRecorder(const PaintContext& context,
PaintCache* cache);
Document ownership/lifetime of PaintCache.

https://codereview.chromium.org/1101783002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:751: bool is_invalidated = true;
is_invalid?

https://codereview.chromium.org/1101783002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:774: is_invalidated =
context.IsRectInvalidated(GetLocalBounds());
nit: IsRectInvalidated() sounds like it is modifying context, when it's just
querying. IsRectInvalid() seems like a better name.

[danakj, 2015-04-23 16:34:09]

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:754: gfx::IntersectRects(damaged_region_.bounds(),
bounds());
On 2015/04/23 16:29:00, sky wrote:
> Isn't bounds() in the parent coordinate space and damaged_region_ in this
layers
> coordinate space?

Ah, yes, thanks. I wish they said so in their names.

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_con...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_con...
ui/compositor/paint_context.h:66: bool EarlyOutOfPaintingWhenNotInvalidated()
const { return !!canvas_; }
On 2015/04/23 16:29:00, sky wrote:
> Oy! The double negatives make this hard to parse (I'm referring to not
invalid,
> not !!). EarlyOutOfPaintingWhenValid() is easier to grok.

Good call :)

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.h (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.h:34: PaintRecorder(const PaintContext& context,
PaintCache* cache);
On 2015/04/23 16:29:00, sky wrote:
> Document ownership/lifetime of PaintCache.

OK. To me raw pointer implies weak pointer that you don't own, but I'll add a
comment saying so.

https://codereview.chromium.org/1101783002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1101783002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:774: is_invalidated =
context.IsRectInvalidated(GetLocalBounds());
On 2015/04/23 16:29:00, sky wrote:
> nit: IsRectInvalidated() sounds like it is modifying context, when it's just
> querying. IsRectInvalid() seems like a better name.

Hm, I disagree. "Invalidated" means that SchedulePaint was called, and "IsFoo"
is a query about Foo. But it sounds like you prefer "Invalid" over "Invalidated"
throughout for this concept of "did SchedulePaint happen". We call this
"invalidated" everywhere in cc/ so I'm just used to that. I can use invalid here
(though slightly prefer consistency).

[danakj, 2015-04-23 16:38:53]

On Thu, Apr 23, 2015 at 9:29 AM, <sky@chromium.org> wrote:

> I think this should be behind a flag until it's ready.


It's behind --ui-enable-slimming-paint (and --ui-enable-impl-side-painting)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-04-23 16:40:28]

PTAL

[sky, 2015-04-23 16:43:31]

As long as all of this is behind a flag, LGTM

https://codereview.chromium.org/1101783002/diff/60001/ui/compositor/paint_cac...
File ui/compositor/paint_cache.h (right):

https://codereview.chromium.org/1101783002/diff/60001/ui/compositor/paint_cac...
ui/compositor/paint_cache.h:26: bool UseCache(const PaintContext& context);
Please document, especially the return value.

https://codereview.chromium.org/1101783002/diff/60001/ui/compositor/paint_con...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/1101783002/diff/60001/ui/compositor/paint_con...
ui/compositor/paint_context.h:66: bool EarlyOutOfPaintingWhenValid() const {
return !!canvas_; }
Nit: ShouldEarlyOut...?

[danakj, 2015-04-23 16:46:14]

https://codereview.chromium.org/1101783002/diff/60001/ui/compositor/paint_cac...
File ui/compositor/paint_cache.h (right):

https://codereview.chromium.org/1101783002/diff/60001/ui/compositor/paint_cac...
ui/compositor/paint_cache.h:26: bool UseCache(const PaintContext& context);
On 2015/04/23 16:43:31, sky wrote:
> Please document, especially the return value.

Oops ya missed that comment.

https://codereview.chromium.org/1101783002/diff/60001/ui/compositor/paint_con...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/1101783002/diff/60001/ui/compositor/paint_con...
ui/compositor/paint_context.h:66: bool EarlyOutOfPaintingWhenValid() const {
return !!canvas_; }
On 2015/04/23 16:43:31, sky wrote:
> Nit: ShouldEarlyOut...?

Ya!

[danakj, 2015-04-23 16:49:13]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-04-23 16:49:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1101783002/#ps80001
(title: "cache: .")

[commit-bot: I haz the power, 2015-04-23 16:50:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1101783002/80001

[commit-bot: I haz the power, 2015-04-23 18:41:23]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-04-23 18:42:25]

Patchset 4 (id:??) landed as
https://crrev.com/7f686cdcff81d6779b962e98e529a7360be2809c
Cr-Commit-Position: refs/heads/master@{#326592}

[Justin Donnelly, 2015-04-23 20:50:55]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/1108463002/ by jdonnelly@chromium.org.

The reason for reverting is: ash_unittests are timing out on Win8 Aura and Linux
ChromiumOS Tests (dbg)(1) bots.

Reverting this CL in order to revert https://codereview.chromium.org/1080633009/
which we suspect to be the cause of the problem..

[danakj, 2015-04-25 15:53:20]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-04-25 15:53:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1101783002/80001

[commit-bot: I haz the power, 2015-04-25 17:53:27]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-04-25 17:54:09]

Patchset 4 (id:??) landed as
https://crrev.com/93f5730af51af283713ad19dd5e08c3b3ea34eaa
Cr-Commit-Position: refs/heads/master@{#326976}