Add GraphicsLayerDebugInfo::includesNewPaintInvalidation to distinguish paint from new RenderObjects

Add GraphicsLayerDebugInfo::includesNewPaintInvalidation to distinguish paint from new RenderObjects

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

This CL adds a flag |hadPaintInvalidation| in
RenderObject to track if the RenderObject had ever been
painted. The flag is then used to notify |WebLayer| if it
includes the first paint invalidation for 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

[kouhei (in TOK), 2014-08-18 07:02:45]

Would you take a look?

[kouhei (in TOK), 2014-08-18 10:40:26]

looking into crashes. please hold on review.

[kouhei (in TOK), 2014-08-19 00:32:16]

Crash bug fixed. PTAL.

[chrishtr, 2014-08-20 04:28:49]

I'll respond on the other CL as well, but I think we should extend
GraphicsLayerDebugInfo
to contain this data. You can still compute it and store it on the GraphicsLayer
in the
m_debugInfo object.

[kouhei (in TOK), 2014-08-20 06:34:02]

On 2014/08/20 04:28:49, chrishtr wrote:
> I'll respond on the other CL as well, but I think we should extend
> GraphicsLayerDebugInfo
> to contain this data. You can still compute it and store it on the
GraphicsLayer
> in the
> m_debugInfo object.

Got it. Updated CL.

[chrishtr, 2014-08-20 17:10:31]

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1677: setEverHadPaintInvalidation(true);
I think this should go in RenderObject::invalidatePaintUsingContainer()

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

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1357: void setEverHadPaintInvalidation(bool
b) { m_bitfields.setEverHadPaintInvalidation(b); }
It's only ever set to true. Rename to setHadPaintInvalidation()

[kouhei (in TOK), 2014-08-21 01:49:41]

Status update. I'm a bit struggling moving the flag into
WebGraphicsLayerDebugInfo.
The problem is that
- The flag is set on RenderObject::invalidateTree stage.
- The flag should be after "rasterize" stage which is invisible from Blink.
- On patch set 5: The flag is currently unset on "record" stage, which is before
"rasterize" stage, this is wrong.

Currently I'm trying to mitigate the problem by clearing out the flag just
before "RenderObject::invalidateTree" calls.

[kouhei (in TOK), 2014-08-21 04:42:53]

Thanks for your comments.

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1677: setEverHadPaintInvalidation(true);
On 2014/08/20 17:10:31, chrishtr wrote:
> I think this should go in RenderObject::invalidatePaintUsingContainer()

Hmm. RenderObject::invalidatePaintUsingContainer() is const, and is being called
from const methods, so it will end up changing constness of many methods. I'm
not sure if we want to do that.

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

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1357: void setEverHadPaintInvalidation(bool
b) { m_bitfields.setEverHadPaintInvalidation(b); }
On 2014/08/20 17:10:31, chrishtr wrote:
> It's only ever set to true. Rename to setHadPaintInvalidation()

Done.

[kouhei (in TOK), 2014-08-21 11:20:40]

https://codereview.chromium.org/474793002/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/474793002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:977: if (!isTracing)
FIXME: I need to limit this to only when TRACE_EVENT_CATEGORY is enabled || HUD
from devtools is enabled.

[chrishtr, 2014-08-21 21:28:34]

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1677: setEverHadPaintInvalidation(true);
On 2014/08/21 at 04:42:53, kouhei wrote:
> On 2014/08/20 17:10:31, chrishtr wrote:
> > I think this should go in RenderObject::invalidatePaintUsingContainer()
> 
> Hmm. RenderObject::invalidatePaintUsingContainer() is const, and is being
called from const methods, so it will end up changing constness of many methods.
I'm not sure if we want to do that.

Well we need to make sure it's hit in all cases. So long as that holds then ok.

[kouhei (in TOK), 2014-08-25 01:57:58]

Would you take another look?

cc side: https://codereview.chromium.org/474783002/

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/474793002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1677: setEverHadPaintInvalidation(true);
On 2014/08/21 21:28:34, chrishtr wrote:
> On 2014/08/21 at 04:42:53, kouhei wrote:
> > On 2014/08/20 17:10:31, chrishtr wrote:
> > > I think this should go in RenderObject::invalidatePaintUsingContainer()
> > 
> > Hmm. RenderObject::invalidatePaintUsingContainer() is const, and is being
> called from const methods, so it will end up changing constness of many
methods.
> I'm not sure if we want to do that.
> 
> Well we need to make sure it's hit in all cases. So long as that holds then
ok.

Done.

[chrishtr, 2014-08-25 16:55:06]

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

https://codereview.chromium.org/474793002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderLayerRepainter.cpp:126: GraphicsLayer* backingLayer
= m_renderer.layer()->graphicsLayerBacking();
Per comment in RenderObject, get rid of this method and inline into
setNeedsPaintInvaliationInRect. The backingLayer will always be present there as
well.

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

https://codereview.chromium.org/474793002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1521: if (view()->usesCompositing() &&
!hadPaintInvalidation())
Let's put this inside of
RenderLayerRepainter::setBackingNeedsPaintInvalidationInRect. The case of a view
issuing invalidations will end up in that method anyway (see
RenderView::invalidatePaintForRectangle).

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

https://codereview.chromium.org/474793002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1354: void setHadPaintInvalidation(bool b)
{ m_bitfields.setHadPaintInvalidation(b); }
Remove arg, only true is pased.

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

https://codereview.chromium.org/474793002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderRegion.cpp:122: void
RenderRegion::paintInvalidaitonOfFlowThreadContentRectangle(const LayoutRect&
paintInvalidationRect, const LayoutRect& flowThreadPortionRect, const
LayoutRect& flowThreadPortionOverflowRect, const LayoutPoint& regionLocation)
Fix the naming typo while you're at it:
paintInvalidationOfFlowThreadContentRectangle

[enne (OOO), 2014-08-25 17:59:07]

How does this feature work with just a boolean? It seems like invalidation on a
layer is marked as a "new paint" invalidation if any of them are.

[kouhei (in TOK), 2014-08-26 14:18:18]

On 2014/08/25 17:59:07, enne wrote:
> How does this feature work with just a boolean? It seems like invalidation on
a
> layer is marked as a "new paint" invalidation if any of them are.

Yes. That is what this patch does. HUD only shows a giant invalidation rect per
layer, so a flag was enough. We'll need more detailed information for the
FrameViewer.

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

https://codereview.chromium.org/474793002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderLayerRepainter.cpp:126: GraphicsLayer* backingLayer
= m_renderer.layer()->graphicsLayerBacking();
On 2014/08/25 16:55:05, chrishtr wrote:
> Per comment in RenderObject, get rid of this method and inline into
> setNeedsPaintInvaliationInRect. The backingLayer will always be present there
as
> well.

Done.

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

https://codereview.chromium.org/474793002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1521: if (view()->usesCompositing() &&
!hadPaintInvalidation())
On 2014/08/25 16:55:05, chrishtr wrote:
> Let's put this inside of
> RenderLayerRepainter::setBackingNeedsPaintInvalidationInRect. The case of a
view
> issuing invalidations will end up in that method anyway (see
> RenderView::invalidatePaintForRectangle).

Done.

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

https://codereview.chromium.org/474793002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1354: void setHadPaintInvalidation(bool b)
{ m_bitfields.setHadPaintInvalidation(b); }
On 2014/08/25 16:55:05, chrishtr wrote:
> Remove arg, only true is pased.

Done.

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

https://codereview.chromium.org/474793002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderRegion.cpp:122: void
RenderRegion::paintInvalidaitonOfFlowThreadContentRectangle(const LayoutRect&
paintInvalidationRect, const LayoutRect& flowThreadPortionRect, const
LayoutRect& flowThreadPortionOverflowRect, const LayoutPoint& regionLocation)
On 2014/08/25 16:55:06, chrishtr wrote:
> Fix the naming typo while you're at it:
> paintInvalidationOfFlowThreadContentRectangle

Done.

[chrishtr, 2014-08-26 16:49:50]

lgtm

[enne (OOO), 2014-08-26 19:04:41]

On 2014/08/26 at 14:18:18, kouhei wrote:
> On 2014/08/25 17:59:07, enne wrote:
> > How does this feature work with just a boolean? It seems like invalidation
on a
> > layer is marked as a "new paint" invalidation if any of them are.
> 
> Yes. That is what this patch does. HUD only shows a giant invalidation rect
per layer, so a flag was enough. We'll need more detailed information for the
FrameViewer.

I guess it doesn't make much sense to me to do this halfway with a boolean if
you're going to eventually need more information.  The HUD could show precise
invalidation.  It just happens to show a unioned damage rect right now since
historically that's all we had and the code is old.

I worry that making that entire unioned damage rect marked as "new paint" even
though the damage could come from many different sources will be confusing to
developers.

[kouhei (in TOK), 2014-08-27 12:09:48]

On 2014/08/26 19:04:41, enne wrote:
> On 2014/08/26 at 14:18:18, kouhei wrote:
> > On 2014/08/25 17:59:07, enne wrote:
> > > How does this feature work with just a boolean? It seems like invalidation
> on a
> > > layer is marked as a "new paint" invalidation if any of them are.
> > 
> > Yes. That is what this patch does. HUD only shows a giant invalidation rect
> per layer, so a flag was enough. We'll need more detailed information for the
> FrameViewer.
> 
> I guess it doesn't make much sense to me to do this halfway with a boolean if
> you're going to eventually need more information.  The HUD could show precise
> invalidation.  It just happens to show a unioned damage rect right now since
> historically that's all we had and the code is old.
> 
> I worry that making that entire unioned damage rect marked as "new paint" even
> though the damage could come from many different sources will be confusing to
> developers.

Thanks for your comments. My second attempt to track more detailed info is here:
https://codereview.chromium.org/498193003/
This CL only used a flag, but the new CL instruments each invalidation rects and
their reason.

[abarth-chromium, 2014-09-02 19:30:15]

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

[abarth-chromium, 2014-09-02 19:30:16]

https://codereview.chromium.org/474793002/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/474793002/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1299:
ADD_BOOLEAN_BITFIELD(hadPaintInvalidation, HadPaintInvalidation);
Maybe I'm misunderstanding this CL.  Are we spending a bit in RenderObject
purely for debug information?  Generally speaking, that's a poor tradeoff.  Bits
in RenderObject are very valuable.

[kouhei (in TOK), 2014-09-02 21:02:27]

On 2014/09/02 19:30:16, abarth wrote:
>
https://codereview.chromium.org/474793002/diff/160001/Source/core/rendering/R...
> File Source/core/rendering/RenderObject.h (right):
> 
>
https://codereview.chromium.org/474793002/diff/160001/Source/core/rendering/R...
> Source/core/rendering/RenderObject.h:1299:
> ADD_BOOLEAN_BITFIELD(hadPaintInvalidation, HadPaintInvalidation);
> Maybe I'm misunderstanding this CL.  Are we spending a bit in RenderObject
> purely for debug information?  Generally speaking, that's a poor tradeoff. 
Bits
> in RenderObject are very valuable.

Thanks for your comment. I think it also applies to the new CL:
https://codereview.chromium.org/498193003/ .

Let me close this CL as we are not landing this. Sorry for the confusion.