Issue trace events to gather repaint information.

Issue trace events to gather repaint information.

This CL adds a few trace events in to gather information about the cause
and size of repaints.

BUG=320139, 351498
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170486

[dsinclair, 2014-03-21 14:19:17]

WIP 

I've started adding some trace events in for repainting. Wanted to get any
feedback you folks had. Let me know if there are other events you'd like to see
added in here.

I need to verify that the trace event arguments are only evaluated if the event
is enabled. If that isn't the case I'll need to do a bit of work where I do
String::format to make it only happen when the category is enabled.

[Julien - ping for review, 2014-03-21 18:35:34]

https://codereview.chromium.org/208143003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/208143003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1477:
TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("blink.repaint"),
"RenderObject::repaintRectangle()",
Is there a lot of values in tracing callers of repaintUsingContainer considering
the InvalidationReason?

https://codereview.chromium.org/208143003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1534: RepaintReason repaintReason =
RepaintNone;
I hate RepaintNone as a default reason. I think this should just be
IncrementalInvalidation (btw, let's ditch repaint from our vocabulary).

https://codereview.chromium.org/208143003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1564: repaintReason =
RepaintBoundsChange;
Sir, this is a change in behavior! We don't want to do incremental invalidations
if newBounds and oldBounds don't intersect. This is because once you don't
intersect, incremental invalidations can be more expensive than just a full
invalidation. This made some of our test progress and I think this is correct in
general.

[dsinclair, 2014-03-24 15:04:30]

https://codereview.chromium.org/208143003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/208143003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1477:
TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("blink.repaint"),
"RenderObject::repaintRectangle()",
On 2014/03/21 18:35:34, Julien Chaffraix - PST wrote:
> Is there a lot of values in tracing callers of repaintUsingContainer
considering
> the InvalidationReason?


The invalidationReasons are only coming from repaintAfterlayoutIfNeeded. Having
these events lets us know where repaintUsingContainer is being called from if it
doesn't go through repaintAfterLayoutIfNeeded.

https://codereview.chromium.org/208143003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1534: RepaintReason repaintReason =
RepaintNone;
On 2014/03/21 18:35:34, Julien Chaffraix - PST wrote:
> I hate RepaintNone as a default reason. I think this should just be
> IncrementalInvalidation (btw, let's ditch repaint from our vocabulary).

Done.

https://codereview.chromium.org/208143003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1564: repaintReason =
RepaintBoundsChange;
On 2014/03/21 18:35:34, Julien Chaffraix - PST wrote:
> Sir, this is a change in behavior! We don't want to do incremental
invalidations
> if newBounds and oldBounds don't intersect. This is because once you don't
> intersect, incremental invalidations can be more expensive than just a full
> invalidation. This made some of our test progress and I think this is correct
in
> general.


I'm not sure I understand, there should be no behaviour change from this CL.
It's possible I named the enum badly, but I'm doing the same work as it was
previously. If the new bounds does not intersect the old bounds we will still
trigger the full repainting on line 1569 as repaintReason != RepaintNone.

[dsinclair, 2014-03-25 14:16:49]

+nduca to make sure I'm not doing anything bad with my trace events.

[nduca, 2014-03-26 20:54:45]

seems pretty awesome. probes look find but every trace event of this form is a
static initializer which has some nonzero costs. Wondering about perf impact as
a result. If we're not too worried, then seems solid. If we need to mitigate,
seems like we could read this once in RuntimeEnabledFeatures somehow and then
have everyone check RuntimeEnabledFeatures or something else of that form,
which'd be a global check instead of a static local var init, then pointer check

[dsinclair, 2014-03-31 14:04:59]

jchaffraix, ping.

[Julien - ping for review, 2014-03-31 18:26:44]

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

https://codereview.chromium.org/208143003/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1504: return "unknown";
ASSERT_NOT_REACHED()?

https://codereview.chromium.org/208143003/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1524: invalidationReason =
InvalidationSelfLayout;
We should just use the ternary operator ? here as it would make the code more
compact.

https://codereview.chromium.org/208143003/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1553: "reason",
invalidationReasonToString(invalidationReason));
Why don't we just pass the invalidation reason to repaintUsingContainer? That
would avoid these lines and remove some code duplication / duplicated traces.

[dsinclair, 2014-03-31 19:24:34]

I opt'd to update all callers of repaintUsingContainer as it seemed like the
nicer solution.

A couple of the invalidations are named with Repaint in the name. I did this to
make it clear they came from the ::repaint methods. We can rename them if/when
we switch the repaint method names.

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

https://codereview.chromium.org/208143003/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1504: return "unknown";
On 2014/03/31 18:26:45, Julien Chaffraix - PST wrote:
> ASSERT_NOT_REACHED()?

Done.

https://codereview.chromium.org/208143003/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1524: invalidationReason =
InvalidationSelfLayout;
On 2014/03/31 18:26:45, Julien Chaffraix - PST wrote:
> We should just use the ternary operator ? here as it would make the code more
> compact.

Done.

https://codereview.chromium.org/208143003/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1553: "reason",
invalidationReasonToString(invalidationReason));
On 2014/03/31 18:26:45, Julien Chaffraix - PST wrote:
> Why don't we just pass the invalidation reason to repaintUsingContainer? That
> would avoid these lines and remove some code duplication / duplicated traces.

Done.

[Julien - ping for review, 2014-03-31 20:11:03]

lgtm.

It is a useful trace for anyone looking at invalidations so worth landing.
Hopefully this doesn't regress performance as Nat was pointing out. If it does,
we will have to find another way.

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

https://codereview.chromium.org/208143003/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1483: const char*
RenderObject::invalidationReasonToString(const InvalidationReason reason) const
No need for |const| InvalidationReason as it's passed by value anyway.

https://codereview.chromium.org/208143003/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1509: ASSERT_NOT_REACHED();
FYI, I would remove the 'default' argument as it prevents the compiler from
catching non-handled-cases.

You can put the ASSERT_NOT_REACHED outside the switch. Also shouldn't we return
something (anything) in this unhandled case?

https://codereview.chromium.org/208143003/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1530: if (invalidationReason ==
InvalidationIncremental&& style()->hasBorderRadius()) {
Missing space before &&.

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

https://codereview.chromium.org/208143003/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1086: const char*
invalidationReasonToString(const InvalidationReason) const;
Same comment about |const| InvalidationReason.

[dsinclair, 2014-03-31 20:22:09]

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

https://codereview.chromium.org/208143003/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1483: const char*
RenderObject::invalidationReasonToString(const InvalidationReason reason) const
On 2014/03/31 20:11:04, Julien Chaffraix - PST wrote:
> No need for |const| InvalidationReason as it's passed by value anyway.

Done.

https://codereview.chromium.org/208143003/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1509: ASSERT_NOT_REACHED();
On 2014/03/31 20:11:04, Julien Chaffraix - PST wrote:
> FYI, I would remove the 'default' argument as it prevents the compiler from
> catching non-handled-cases.
> 
> You can put the ASSERT_NOT_REACHED outside the switch. Also shouldn't we
return
> something (anything) in this unhandled case?

Done.

https://codereview.chromium.org/208143003/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1530: if (invalidationReason ==
InvalidationIncremental&& style()->hasBorderRadius()) {
On 2014/03/31 20:11:04, Julien Chaffraix - PST wrote:
> Missing space before &&.

Done.

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

https://codereview.chromium.org/208143003/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1086: const char*
invalidationReasonToString(const InvalidationReason) const;
On 2014/03/31 20:11:04, Julien Chaffraix - PST wrote:
> Same comment about |const| InvalidationReason.

Done.

[dsinclair, 2014-03-31 20:22:19]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:22:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/208143003/150001

[commit-bot: I haz the power, 2014-03-31 23:52:09]

Change committed as 170486