Add |GraphicsLayerDebugInfo::getAnnotatedInvalidationRects|

Add |GraphicsLayerDebugInfo::getAnnotatedInvalidationRects|

This is a Blink side patch for visualizing paint
invalidation for newly created RenderObject.
Demo @ https://www.youtube.com/watch?v=L-zo91nL2nE&list=UUGC5ommqy7KyPzZ-eGlL_Dg

After this CL, GraphicsLayerDebugInfo holds a vector of
invalidation rects along with their source / reason.
The rects can be queried from the embedder using the method
|GraphicsLayerDebugInfo::getAnnotatedInvalidationRects|.

This CL also adds a flag |RenderObject::hadPaintInvalidation|
to track if the RenderObject had ever been painted.
The flag is then used to annotate invalidation rects
for first paint of the RenderObject.

After this CL, WebLayer implementator can query this info to
visualize the layer invalidation including the first paint,
which is useful for telling if the repaint storm is due to
the page itself, or is a performance bug in Blink rendering
stack.

BUG=402033
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181774

[kouhei (in TOK), 2014-08-27 10:37:21]

kouhei@chromium.org changed reviewers:
+ chrishtr@chromium.org, jchaffraix@chromium.org, leviw@chromium.org,
wangxianzhu@chromium.org

[kouhei (in TOK), 2014-08-27 10:37:21]

Would you take a look?

This is my second attempt after https://codereview.chromium.org/474793002/ .
This time, I'm tracking the invalidation rects along with the reason, so that
they can be viewed in FrameViewer.

[chrishtr, 2014-08-27 18:48:40]

Do you have a corresponding cc change? I'd like to get it run by enne@ at a high
level first to make sure it's what is agreed upon.

[kouhei (in TOK), 2014-08-28 07:05:44]

enne: Would you do a high level review if this approach is acceptable to you?

On 2014/08/27 18:48:40, chrishtr wrote:
> Do you have a corresponding cc change? I'd like to get it run by enne@ at a
high
> level first to make sure it's what is agreed upon.
I uploaded my WIP cc changes in
https://codereview.chromium.org/474783002/#ps160001 , but its not yet ready for
review.

[kouhei (in TOK), 2014-08-30 14:25:39]

kouhei@chromium.org changed reviewers:
+ enne@chromium.org

[kouhei (in TOK), 2014-08-30 14:25:40]

enne: Would you take a high level look?
Sorry I forgot to add you in the reviewer list by mistake.

[abarth-chromium, 2014-09-02 19:34:57]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2014-09-02 19:34:58]

https://codereview.chromium.org/498193003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/498193003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1297:
ADD_BOOLEAN_BITFIELD(hadPaintInvalidation, HadPaintInvalidation);
I don't think we should add bits to RenderObject to support debugging features. 
Bits in RenderObject are too valuable to use for that purpose.

[enne (OOO), 2014-09-02 19:41:34]

enne@chromium.org changed reviewers:
- abarth@chromium.org

[enne (OOO), 2014-09-02 19:41:35]

https://codereview.chromium.org/498193003/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/498193003/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:905:
m_debugInfo.appendAnnotatedInvalidateRect(rect, annotations);
Is there a way to only do this when tracing or showing paint rects?

[kouhei (in TOK), 2014-09-03 22:53:48]

https://codereview.chromium.org/498193003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/498193003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1297:
ADD_BOOLEAN_BITFIELD(hadPaintInvalidation, HadPaintInvalidation);
On 2014/09/02 19:34:58, abarth wrote:
> I don't think we should add bits to RenderObject to support debugging
features. 
> Bits in RenderObject are too valuable to use for that purpose.

Acknowledged. I'm thinking of keeping a HashSet for RenderObject that never got
painted iff the tracing is enabled.

https://codereview.chromium.org/498193003/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/498193003/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:905:
m_debugInfo.appendAnnotatedInvalidateRect(rect, annotations);
On 2014/09/02 19:41:34, enne wrote:
> Is there a way to only do this when tracing or showing paint rects?

I agree we should have a way to only enable these when tracing is enabled, but
I'm not sure what would be a best way. I'm thinking of adding an entry in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
, but was wondering if it would be too heavyweight to query it everytime in this
hot call path.

abarth: Do you have any other idea on adding a flag?

[dsinclair, 2014-09-04 15:38:37]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2014-09-04 15:38:38]

https://codereview.chromium.org/498193003/diff/40001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/498193003/diff/40001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:905:
m_debugInfo.appendAnnotatedInvalidateRect(rect, annotations);
On 2014/09/03 22:53:47, kouhei wrote:
> On 2014/09/02 19:41:34, enne wrote:
> > Is there a way to only do this when tracing or showing paint rects?
> 
> I agree we should have a way to only enable these when tracing is enabled, but
> I'm not sure what would be a best way. I'm thinking of adding an entry in
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> , but was wondering if it would be too heavyweight to query it everytime in
this
> hot call path.
> 
> abarth: Do you have any other idea on adding a flag?


For tracing, you can check if the given trace category is enabled. Take a look
at TRACE_EVENT_CATEGORY_GROUP_ENABLED. I'm not sure how to check if show paint
rects is enabled or not though.

If that is too heavy, there is a TRACE_EVENT_GET_CATEGORY_GROUP_ENABLED in
chromium tracing header which can be ported to Blink. This will give a unsigned
char* which can be stored in a static that you reference if the given category
is enabled or not. So, after the first time just becomes a branch instead of a
lookup and branch.

[enne (OOO), 2014-09-04 17:13:24]

Another alternative is to plumb the reason along with the invalidation all the
way through cc::Layer::SetNeedsDisplayRect, and then cc can ignore it or record
it based on both trace categories and current settings (show paint rects)?

[kouhei (in TOK), 2014-09-04 17:42:35]

Thanks for your feedback!

Updated CL to not use a HashSet instead of adding a bit in RenderObject, and
conditionally enable this using TRACE_EVENT_GROUP. PTAL.

[enne (OOO), 2014-09-04 18:39:33]

https://codereview.chromium.org/498193003/diff/60001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/498193003/diff/60001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:74:
TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("blink.invalidation"),
&isEnabled);
How do you plan on having this handle show paint rects? Or is this only going to
be a tracing concept?

[kouhei (in TOK), 2014-09-04 18:58:52]

https://codereview.chromium.org/498193003/diff/60001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/498193003/diff/60001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:74:
TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("blink.invalidation"),
&isEnabled);
On 2014/09/04 18:39:33, enne wrote:
> How do you plan on having this handle show paint rects? Or is this only going
to
> be a tracing concept?

Devtools has access to the trace event category flag, so I was thinking of
flipping the flag from there.

[enne (OOO), 2014-09-04 21:39:41]

https://codereview.chromium.org/498193003/diff/60001/Source/platform/graphics...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/498193003/diff/60001/Source/platform/graphics...
Source/platform/graphics/GraphicsLayer.cpp:74:
TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("blink.invalidation"),
&isEnabled);
On 2014/09/04 18:58:52, kouhei wrote:
> On 2014/09/04 18:39:33, enne wrote:
> > How do you plan on having this handle show paint rects? Or is this only
going
> to
> > be a tracing concept?
> 
> Devtools has access to the trace event category flag, so I was thinking of
> flipping the flag from there.

Flipping the flag = enabling that trace category? If so, I think that would be
weird to have clicking a devtools checkbox change which events were being
recorded.

Is there some way to do it in reverse and check the devtools show paint rects
setting here? That's one reason that I had suggested passing the invalidation
reason through to cc, since LayerTreeDebugState knows about paint rect options.

[kouhei (in TOK), 2014-09-04 22:25:34]

Thanks for your suggestion. The hardest part seems to be how we should
conditionally enable this.

The first paint invalidation tracking will be used from both 1) "show repaint
rects" feature which does not use trace events, and 2) FrameViewer which uses
trace events.
We want the flag to decide both a) if RenderObject should track first paint, and
b) cc should track annotated invalidation rects.

TRACE_EVENT_CATEGORY_GROUP_ENABLED is a global flag, so it works, but it is true
that the patch is abusing the flag to also enable annotations to be shown on
"show repaint rects" which does not use tracing events.

enne: Thanks for the idea. Deferring the decision to cc works beautifully for
b), but if we avoid adding a bit to RenderObject and use slow HashSet, then we
would want to conditionally enable |RenderObject::hadPaintInvalidation| tracking
too.

I'm currently thinking of adding a globally accessible flag for 1) "show repaint
rects" case and taking an OR with TRACE_EVENT_CATEGORY_GROUP_ENABLED, but then
adding a flag that is both accessible from core/ and platform/ is difficult.

[kouhei (in TOK), 2014-09-05 00:18:10]

Updated CL.

> I'm currently thinking of adding a globally accessible flag for 1) "show
repaint
> rects" case and taking an OR with TRACE_EVENT_CATEGORY_GROUP_ENABLED, but then
> adding a flag that is both accessible from core/ and platform/ is difficult.

I added a flag in platform/graphics/FirstPaintInvalidationTracking.h which is
accessible from both core/ and platform/. Do you think this would make sense?

[kouhei (in TOK), 2014-09-08 20:39:31]

I think I addressed all comments. Would you take a look?

[chrishtr, 2014-09-09 18:53:31]

lgtm

[kouhei (in TOK), 2014-09-09 22:07:50]

kouhei@chromium.org changed reviewers:
+ tkent@chromium.org

[kouhei (in TOK), 2014-09-09 22:07:51]

tkent: Would you take a look at changes in Source/web and platform/?

[tkent, 2014-09-10 00:03:28]

https://codereview.chromium.org/498193003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/498193003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:106:
DEFINE_STATIC_LOCAL(RenderObjectWeakSet, set, ());
This set doesn't look traceable.  Needs Persistent?

https://codereview.chromium.org/498193003/diff/140001/Source/platform/graphic...
File Source/platform/graphics/FirstPaintInvalidationTracking.cpp (right):

https://codereview.chromium.org/498193003/diff/140001/Source/platform/graphic...
Source/platform/graphics/FirstPaintInvalidationTracking.cpp:13: bool
showPaintRectsEnabled = false;
static bool showPaintRectsEnabled = false;

is simpler than anonymous namespace.

https://codereview.chromium.org/498193003/diff/140001/Source/platform/graphic...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/498193003/diff/140001/Source/platform/graphic...
Source/platform/graphics/GraphicsLayer.cpp:1: /* * Copyright (C) 2009 Apple Inc.
All rights reserved.
accidental edit?

[kouhei (in TOK), 2014-09-10 23:46:29]

https://codereview.chromium.org/498193003/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/498193003/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:106:
DEFINE_STATIC_LOCAL(RenderObjectWeakSet, set, ());
On 2014/09/10 00:03:27, tkent (overloaded) wrote:
> This set doesn't look traceable.  Needs Persistent?

Done.

https://codereview.chromium.org/498193003/diff/140001/Source/platform/graphic...
File Source/platform/graphics/FirstPaintInvalidationTracking.cpp (right):

https://codereview.chromium.org/498193003/diff/140001/Source/platform/graphic...
Source/platform/graphics/FirstPaintInvalidationTracking.cpp:13: bool
showPaintRectsEnabled = false;
On 2014/09/10 00:03:27, tkent (overloaded) wrote:
> static bool showPaintRectsEnabled = false;
> 
> is simpler than anonymous namespace.
> 

Done.

https://codereview.chromium.org/498193003/diff/140001/Source/platform/graphic...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/498193003/diff/140001/Source/platform/graphic...
Source/platform/graphics/GraphicsLayer.cpp:1: /* * Copyright (C) 2009 Apple Inc.
All rights reserved.
On 2014/09/10 00:03:28, tkent (overloaded) wrote:
> accidental edit?

Done.

[tkent, 2014-09-10 23:49:53]

lgtm

[kouhei (in TOK), 2014-09-10 23:51:09]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2014-09-10 23:51:18]

Thanks for review!

[commit-bot: I haz the power, 2014-09-10 23:51:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/498193003/160001

[commit-bot: I haz the power, 2014-09-10 23:55:52]

Committed patchset #9 (id:160001) as 181774