Invalidate the bigger rect if one rect contains the other in LayoutObject::fullyInvalidateRect()

Invalidate the bigger rect if one rect contains the other in LayoutObject::fullyInvalidateRect()

This reduces the cost along the path from
LayoutObject::invalidatePaintUsingContainer() to
RecordingSource::SetNeedsDisplayRect(), and the cost of
InvalidationRegion::Union().

BUG=602068
Committed: https://crrev.com/8c0c32361a933e23ed11c206413c381588fe44d8
Cr-Commit-Position: refs/heads/master@{#386782}

[Xianzhu, 2016-04-11 23:34:37]

wangxianzhu@chromium.org changed reviewers:
+ pdr@chromium.org

[Xianzhu, 2016-04-11 23:34:37]

I will rebase this CL onto https://codereview.chromium.org/1880763002/ after
it's landed and layout test results are rebaselined, so that we can easily see
which invalidation rects of which objects are avoided.

[pdr., 2016-04-12 03:01:22]

LGTM

I'm curious what this looks like on cluster telemetry. Do you mind kicking off a
run just to ensure we don't regress?

My comment below about cleaning up the logic is optional.

https://codereview.chromium.org/1882473002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1882473002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1582: void
LayoutObject::fullyInvalidatePaint(const LayoutBoxModelObject&
paintInvalidationContainer, PaintInvalidationReason invalidationReason, const
LayoutRect& oldBounds, const LayoutRect& newBounds)
This logic took me a while to reason through. It's correct, but I wonder if we
could make it easier to understand without affecting the compiled result? Maybe
something like the following where we don't re-use invalidationRect:

// The following logic avoids invalidating twice if one set of bounds contains
the other.
if (!oldBounds.contains(newBounds)) {
    LayoutRect invalidation = newBounds;
    adjustInvalidationRectForCompositedScrolling(invalidation,
paintInvalidationContainer);
    invalidatePaintUsingContainer(paintInvalidationContainer, invalidation,
invalidationReason);

    // Early out when the old bounds have already been invalidated.
    if (newBounds.contains(oldBounds))
       return;
}
LayoutRect invalidation = oldBounds;
adjustInvalidationRectForCompositedScrolling(invalidation,
paintInvalidationContainer);
invalidatePaintUsingContainer(paintInvalidationContainer, invalidation,
invalidationReason);

[Xianzhu, 2016-04-12 16:46:31]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-04-12 16:46:31]

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

[commit-bot: I haz the power, 2016-04-12 16:46:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1882473002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1882473002/20001

[Xianzhu, 2016-04-12 16:47:50]

For now we don't have a good benchmark to run on cluster telemetry, but we have
UMA metric for paint invalidation.

https://codereview.chromium.org/1882473002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1882473002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1582: void
LayoutObject::fullyInvalidatePaint(const LayoutBoxModelObject&
paintInvalidationContainer, PaintInvalidationReason invalidationReason, const
LayoutRect& oldBounds, const LayoutRect& newBounds)
On 2016/04/12 03:01:22, pdr wrote:
> This logic took me a while to reason through. It's correct, but I wonder if we
> could make it easier to understand without affecting the compiled result?
Maybe
> something like the following where we don't re-use invalidationRect:
> 
> // The following logic avoids invalidating twice if one set of bounds contains
> the other.
> if (!oldBounds.contains(newBounds)) {
>     LayoutRect invalidation = newBounds;
>     adjustInvalidationRectForCompositedScrolling(invalidation,
> paintInvalidationContainer);
>     invalidatePaintUsingContainer(paintInvalidationContainer, invalidation,
> invalidationReason);
> 
>     // Early out when the old bounds have already been invalidated.
>     if (newBounds.contains(oldBounds))
>        return;
> }
> LayoutRect invalidation = oldBounds;
> adjustInvalidationRectForCompositedScrolling(invalidation,
> paintInvalidationContainer);
> invalidatePaintUsingContainer(paintInvalidationContainer, invalidation,
> invalidationReason);

Done (except that oldBounds and newBounds inversed to keep the sequence the same
as before if both need invalidation).

[commit-bot: I haz the power, 2016-04-12 18:13:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-12 18:13:38]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2016-04-12 18:17:17]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-04-12 18:17:17]

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

[commit-bot: I haz the power, 2016-04-12 18:17:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1882473002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1882473002/40001

[commit-bot: I haz the power, 2016-04-12 19:53:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-12 19:55:37]

Description was changed from

==========
Invalidate the bigger rect if one rect contains the other in
LayoutObject::fullyInvalidateRect()

This reduces the cost along the path from
LayoutObject::invalidatePaintUsingContainer() to
RecordingSource::SetNeedsDisplayRect(), and the cost of
InvalidationRegion::Union().

BUG=602068
==========

to

==========
Invalidate the bigger rect if one rect contains the other in
LayoutObject::fullyInvalidateRect()

This reduces the cost along the path from
LayoutObject::invalidatePaintUsingContainer() to
RecordingSource::SetNeedsDisplayRect(), and the cost of
InvalidationRegion::Union().

BUG=602068

Committed: https://crrev.com/8c0c32361a933e23ed11c206413c381588fe44d8
Cr-Commit-Position: refs/heads/master@{#386782}
==========

[commit-bot: I haz the power, 2016-04-12 19:55:38]

Patchset 3 (id:??) landed as
https://crrev.com/8c0c32361a933e23ed11c206413c381588fe44d8
Cr-Commit-Position: refs/heads/master@{#386782}