Don't invalidate rect if it's covered by fully invalidated parent's visual rect

Don't invalidate rect if it's covered by fully invalidated parent's visual rect

This saves the cost from
ObjectPaintInvalidator::invalidatePaintUsingContainer()
all the way down to
cc::Layer::SetNeedsDisplayRect().

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2707063003
Cr-Commit-Position: refs/heads/master@{#453070}
Committed: https://chromium.googlesource.com/chromium/src/+/f19c8dd31cb7b49958db2c8f0b633c29c7520c43

[Xianzhu, 2017-02-21 18:58:43]

Description was changed from

==========
Don't invalidate rect if it's covered by fully invalidated parent's visual rect

This saves the cost from
ObjectPaintInvalidator::invalidatePaintUsingContainer()
all the way down to
cc::Layer::SetNeedsDisplayRect().

BUG=685179
==========

to

==========
Don't invalidate rect if it's covered by fully invalidated parent's visual rect

This saves the cost from
ObjectPaintInvalidator::invalidatePaintUsingContainer()
all the way down to
cc::Layer::SetNeedsDisplayRect().

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-02-21 18:58:45]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 19:00:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 21:05:00]

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

[commit-bot: I haz the power, 2017-02-21 21:05:01]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Xianzhu, 2017-02-22 00:27:46]

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

[Xianzhu, 2017-02-22 17:05:48]

Perf tryjobs show that this patch can improve performance
blink_perf.paint:paint-offset-changes.html of by about 10%.

Will rebase the layout tests in the next patch set after your review of the
current patch.

[pdr., 2017-02-22 18:02:22]

LGTM

[Xianzhu, 2017-02-22 18:36:27]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 18:37:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 20:49:30]

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

[commit-bot: I haz the power, 2017-02-22 20:49:31]

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

[wkorman, 2017-02-22 21:52:22]

lgtm

https://codereview.chromium.org/2707063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp (right):

https://codereview.chromium.org/2707063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:342: if
(object.isTextOrSVGChild() ||
Is there a relevant spec section or something we could link here in a comment so
that future eng will know how we decided on the below conditionals as being the
right ones? This is not rhetorical, I ask because I myself pondered while
reading. The out-of-flow and floating made immediate sense, I assume
column-span-all is a proxy for is-multicol and there's some multicol spec. In
any case a comment likely helpful for future others.

https://codereview.chromium.org/2707063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:371:
DCHECK(m_object.isRooted());
Just curious, why shift to DCHECK?

https://codereview.chromium.org/2707063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:373: if
(m_object != paintInvalidationContainer && m_object.parent() &&
Could be worth adding a comment to explain background re: this optimization in
prose. Reading all of the conditionals below alone makes it kind of obtuse.

[Xianzhu, 2017-02-22 23:21:00]

Ptal.

https://codereview.chromium.org/2707063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp (right):

https://codereview.chromium.org/2707063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:342: if
(object.isTextOrSVGChild() ||
On 2017/02/22 21:52:22, wkorman wrote:
> Is there a relevant spec section or something we could link here in a comment
so
> that future eng will know how we decided on the below conditionals as being
the
> right ones? This is not rhetorical, I ask because I myself pondered while
> reading. The out-of-flow and floating made immediate sense, I assume
> column-span-all is a proxy for is-multicol and there's some multicol spec. In
> any case a comment likely helpful for future others.

The conditions are the same conditions that LayoutObject::container() will
surely return parent().

However, your question made me think of better heuristics.

I uploaded a new patch that uses heuristics that are closer to what we want: the
parent and the child are on the same backing and the parent's visual rect covers
child's. The new patch is more efficient because the condition covers more cases
that are suitable for the optimization.

The new patch also avoids the optimization if the invalidation is not during
paint invalidation phase (e.g. invalidatePaintRectangle or
invalidatePaintOfPreviousVisualRect) and the optimization might be incorrect.

https://codereview.chromium.org/2707063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:371:
DCHECK(m_object.isRooted());
On 2017/02/22 21:52:22, wkorman wrote:
> Just curious, why shift to DCHECK?

isRooted() is slow because it traverse the layout/layer tree.

However, in the new patch I'll leave invalidatePaintUsingContainer() unchanged,
so I would like to leave this change to a follow-up.

[Xianzhu, 2017-02-22 23:47:10]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 23:48:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wkorman, 2017-02-23 01:26:25]

lgtm

https://codereview.chromium.org/2707063003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp (right):

https://codereview.chromium.org/2707063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:480:
m_object.parent() != m_context.paintInvalidationContainer &&
I wanted to better understand how checking whether parent !=
paint-invalidation-container relates to whether they're on the same backing?

I figure this is correct and I just need to hear you explain it a bit more to
me, but perhaps we should also make it clearer in the comment since I wondered.
It is hard for me to quickly understand "exclude the case that they are not",
when in fact if we pass this conditional test, what it means is that we will
proceed in considering application of the optimization.

[commit-bot: I haz the power, 2017-02-23 01:33:42]

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

[commit-bot: I haz the power, 2017-02-23 01:33:42]

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

[Xianzhu, 2017-02-23 04:38:49]

https://codereview.chromium.org/2707063003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp (right):

https://codereview.chromium.org/2707063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:480:
m_object.parent() != m_context.paintInvalidationContainer &&
On 2017/02/23 01:26:25, wkorman wrote:
> I wanted to better understand how checking whether parent !=
> paint-invalidation-container relates to whether they're on the same backing?
> 
> I figure this is correct and I just need to hear you explain it a bit more to
> me, but perhaps we should also make it clearer in the comment since I
wondered.
> It is hard for me to quickly understand "exclude the case that they are not",
> when in fact if we pass this conditional test, what it means is that we will
> proceed in considering application of the optimization.

The idea was that the paint invalidation container itself may paint into
multiple backings. The parent may have been fully invalidated on the scrolling
container layer, while the current object may be invalidating on the scrolling
contents layer.

I have extracted the conditions into
ObjectPaintInvalidator::parentFullyInvalidatedOnSameBacking() and added
comments.

[Xianzhu, 2017-02-23 04:40:29]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 04:40:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 06:23:50]

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

[commit-bot: I haz the power, 2017-02-23 06:23:52]

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

[Xianzhu, 2017-02-24 06:32:34]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 06:32:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 07:11:12]

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

[commit-bot: I haz the power, 2017-02-24 07:11:15]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2017-02-24 16:00:02]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-24 16:00:04]

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

[commit-bot: I haz the power, 2017-02-24 16:00:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 17:40:46]

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

[commit-bot: I haz the power, 2017-02-24 17:40:49]

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

[Xianzhu, 2017-02-25 02:48:17]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-25 02:48:19]

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

[commit-bot: I haz the power, 2017-02-25 02:48:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-25 05:01:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487990897528950,
"parent_rev": "b39e4cecb07231262bbce0ab825c31b1fe9bc76d", "commit_rev":
"f19c8dd31cb7b49958db2c8f0b633c29c7520c43"}

[commit-bot: I haz the power, 2017-02-25 05:01:46]

Description was changed from

==========
Don't invalidate rect if it's covered by fully invalidated parent's visual rect

This saves the cost from
ObjectPaintInvalidator::invalidatePaintUsingContainer()
all the way down to
cc::Layer::SetNeedsDisplayRect().

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Don't invalidate rect if it's covered by fully invalidated parent's visual rect

This saves the cost from
ObjectPaintInvalidator::invalidatePaintUsingContainer()
all the way down to
cc::Layer::SetNeedsDisplayRect().

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2707063003
Cr-Commit-Position: refs/heads/master@{#453070}
Committed:
https://chromium.googlesource.com/chromium/src/+/f19c8dd31cb7b49958db2c8f0b63...
==========

[commit-bot: I haz the power, 2017-02-25 05:01:51]

Committed patchset #7 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f19c8dd31cb7b49958db2c8f0b63...