Improve performance when calling PaintLayer::setNeedsRepaint()

Improve performance when calling PaintLayer::setNeedsRepaint()

Previously we call enclosingLayer()->setNeedsRepaint(), but
enclosingLayer() needs to walk the tree up to find the layer.

Now save enclosing layer in PaintInvalidationState, so that we
can get it directly during paint invalidation.

If not during paint invaidation:
- for invalidatePaintIncludingNonCompositingDescendants() and
  invalidateDisplayItemClientsIncludingNonCompositingDescendants(),
  just call setNeedsRepaint() for all layers encountered during
  tree traversal;
- others still use enclosingLayer()->setNeedsRepaint().

Committed: https://crrev.com/d66c377a43c54efa09cc2b4e90dab452d3f9dfdc
Cr-Commit-Position: refs/heads/master@{#370313}

[Xianzhu, 2016-01-14 18:22:54]

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

[chrishtr, 2016-01-14 18:51:13]

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3426: {
This looks like a copy of the old code. I expected to see deleted lines where
setNeedsRepaint() was removed.

[Xianzhu, 2016-01-14 19:19:08]

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3426: {
On 2016/01/14 18:51:13, chrishtr wrote:
> This looks like a copy of the old code. I expected to see deleted lines where
> setNeedsRepaint() was removed.

Now invalidateDisplayItemClients() doesn't set layers needs repaint, but the
callers do instead. This function becomes 'NotSettingLayerNeedsRepaint' because
of the change of invalidateDisplayItemClients().

[chrishtr, 2016-01-15 00:02:22]

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1253: if (PaintLayer*
enclosingLayer = this->enclosingLayer())
In particular, this is still calling setNeedsRepaint in the new version.

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3426: {
On 2016/01/14 at 19:19:08, Xianzhu wrote:
> On 2016/01/14 18:51:13, chrishtr wrote:
> > This looks like a copy of the old code. I expected to see deleted lines
where
> > setNeedsRepaint() was removed.
> 
> Now invalidateDisplayItemClients() doesn't set layers needs repaint, but the
callers do instead. This function becomes 'NotSettingLayerNeedsRepaint' because
of the change of invalidateDisplayItemClients().

I understand the goal, but can't find where you are changing it to not
setNeedsRepaint where it was doing so before. You changed line 3434 to
invalidatePaintOfPreviousPaintInvalidationRectNotSettingLayerNeedsRepaint(), but

as far as I can tell, the definition of
invalidatePaintOfPreviousPaintInvalidationRectNotSettingLayerNeedsRepaint() is
the same
as it was before.. ?

[Xianzhu, 2016-01-15 00:09:37]

On 2016/01/15 00:02:22, chrishtr wrote:
>
https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:1253: if (PaintLayer*
> enclosingLayer = this->enclosingLayer())
> In particular, this is still calling setNeedsRepaint in the new version.
> 
>
https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:3426: {
> On 2016/01/14 at 19:19:08, Xianzhu wrote:
> > On 2016/01/14 18:51:13, chrishtr wrote:
> > > This looks like a copy of the old code. I expected to see deleted lines
> where
> > > setNeedsRepaint() was removed.
> > 
> > Now invalidateDisplayItemClients() doesn't set layers needs repaint, but the
> callers do instead. This function becomes 'NotSettingLayerNeedsRepaint'
because
> of the change of invalidateDisplayItemClients().
> 
> I understand the goal, but can't find where you are changing it to not
> setNeedsRepaint where it was doing so before. You changed line 3434 to
> invalidatePaintOfPreviousPaintInvalidationRectNotSettingLayerNeedsRepaint(),
but
> 
> as far as I can tell, the definition of
> invalidatePaintOfPreviousPaintInvalidationRectNotSettingLayerNeedsRepaint() is
> the same
> as it was before.. ?

It did set layer needsRepaint because it calls invalidateDispalyItemClient()
which did set set layer needsRepaint. Now invalidateDisplayItemClients() just
invalidates display item clients without setting layer needs repaint.

[chrishtr, 2016-01-15 00:12:15]

On 2016/01/15 at 00:09:37, wangxianzhu wrote:
> On 2016/01/15 00:02:22, chrishtr wrote:
> >
https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> > 
> >
https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1253: if (PaintLayer*
> > enclosingLayer = this->enclosingLayer())
> > In particular, this is still calling setNeedsRepaint in the new version.
> > 
> >
https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutObject.cpp:3426: {
> > On 2016/01/14 at 19:19:08, Xianzhu wrote:
> > > On 2016/01/14 18:51:13, chrishtr wrote:
> > > > This looks like a copy of the old code. I expected to see deleted lines
> > where
> > > > setNeedsRepaint() was removed.
> > > 
> > > Now invalidateDisplayItemClients() doesn't set layers needs repaint, but
the
> > callers do instead. This function becomes 'NotSettingLayerNeedsRepaint'
because
> > of the change of invalidateDisplayItemClients().
> > 
> > I understand the goal, but can't find where you are changing it to not
> > setNeedsRepaint where it was doing so before. You changed line 3434 to
> > invalidatePaintOfPreviousPaintInvalidationRectNotSettingLayerNeedsRepaint(),
but
> > 
> > as far as I can tell, the definition of
> > invalidatePaintOfPreviousPaintInvalidationRectNotSettingLayerNeedsRepaint()
is
> > the same
> > as it was before.. ?
> 
> It did set layer needsRepaint because it calls invalidateDispalyItemClient()
which did set set layer needsRepaint. Now invalidateDisplayItemClients() just
invalidates display item clients without setting layer needs repaint.

See my comment on line 1253.

[Xianzhu, 2016-01-15 00:32:05]

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1585823002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1253: if (PaintLayer*
enclosingLayer = this->enclosingLayer())
On 2016/01/15 00:02:21, chrishtr wrote:
> In particular, this is still calling setNeedsRepaint in the new version.

Oops. Sorry I thought I had removed these two lines.

Done.

[chrishtr, 2016-01-15 00:38:43]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-01-15 00:38:43]

lgtm

[commit-bot: I haz the power, 2016-01-15 00:39:14]

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

[commit-bot: I haz the power, 2016-01-15 00:39:15]

This CL has an open dependency (Issue 1584903002 Patch 100001). Please resolve
the dependency and try again.

[Xianzhu, 2016-01-15 01:00:09]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-01-15 01:00:09]

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

[commit-bot: I haz the power, 2016-01-15 01:03:51]

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

[commit-bot: I haz the power, 2016-01-15 01:39:13]

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

[commit-bot: I haz the power, 2016-01-15 01:39:13]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2016-01-19 22:10:20]

Patchset #7 (id:120001) has been deleted

[Xianzhu, 2016-01-19 22:10:22]

Patchset #7 (id:140001) has been deleted

[Xianzhu, 2016-01-19 22:10:34]

Patchset #6 (id:100001) has been deleted

[Xianzhu, 2016-01-19 22:13:49]

Hi Chris, Sorry that previous patches didn't actually work because the extra
setNeedsReapint call in LayoutObject::invalidateDisplayItemClients() hid the
failures. Patch Set 6 works. Ptal.

[chrishtr, 2016-01-19 23:17:08]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-01-19 23:17:08]

lgtm

[commit-bot: I haz the power, 2016-01-19 23:19:01]

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

[commit-bot: I haz the power, 2016-01-19 23:59:59]

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

[commit-bot: I haz the power, 2016-01-20 00:00:00]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2016-01-20 00:03:51]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-01-20 00:07:35]

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

[commit-bot: I haz the power, 2016-01-20 01:26:36]

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

[commit-bot: I haz the power, 2016-01-20 01:26:36]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-01-20 01:46:30]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-01-20 01:49:05]

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

[commit-bot: I haz the power, 2016-01-20 05:14:51]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-01-20 05:16:29]

Description was changed from

==========
Improve performance when calling PaintLayer::setNeedsRepaint()

Previously we call enclosingLayer()->setNeedsRepaint(), but
enclosingLayer() needs to walk the tree up to find the layer.

Now save enclosing layer in PaintInvalidationState, so that we
can get it directly during paint invalidation.

If not during paint invaidation:
- for invalidatePaintIncludingNonCompositingDescendants() and
  invalidateDisplayItemClientsIncludingNonCompositingDescendants(),
  just call setNeedsRepaint() for all layers encountered during
  tree traversal;
- others still use enclosingLayer()->setNeedsRepaint().
==========

to

==========
Improve performance when calling PaintLayer::setNeedsRepaint()

Previously we call enclosingLayer()->setNeedsRepaint(), but
enclosingLayer() needs to walk the tree up to find the layer.

Now save enclosing layer in PaintInvalidationState, so that we
can get it directly during paint invalidation.

If not during paint invaidation:
- for invalidatePaintIncludingNonCompositingDescendants() and
  invalidateDisplayItemClientsIncludingNonCompositingDescendants(),
  just call setNeedsRepaint() for all layers encountered during
  tree traversal;
- others still use enclosingLayer()->setNeedsRepaint().

Committed: https://crrev.com/d66c377a43c54efa09cc2b4e90dab452d3f9dfdc
Cr-Commit-Position: refs/heads/master@{#370313}
==========

[commit-bot: I haz the power, 2016-01-20 05:16:30]

Patchset 6 (id:??) landed as
https://crrev.com/d66c377a43c54efa09cc2b4e90dab452d3f9dfdc
Cr-Commit-Position: refs/heads/master@{#370313}