Don't access DisplayItemClient::VisualRect() for cached display items.

Don't access DisplayItemClient::VisualRect() for cached display items.

- Access DisplayItemClient::VisualRect() when a new DisplayItem is
  created, and save it in the new DisplayItem.
- When a cached display item is copied, don't access DisplayItemClient::
  VisualRect() but fetch from the cached DisplayItem.
- Remove DisplayItemList::visual_rects_.
- Save original visual rects (LayoutRect) and apply GraphicsLayer offset
  with EnclosingIntRect() when the display items are added into
  WebDisplayItemList.

Please see the bug for the reason.

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

Review-Url: https://codereview.chromium.org/2894093002
Cr-Commit-Position: refs/heads/master@{#473371}
Committed: https://chromium.googlesource.com/chromium/src/+/3c7f1c4ecd1421d074daa5a103ea3f895c1c8724

[Xianzhu, 2017-05-19 00:07:21]

Description was changed from

==========
Don't access DisplayItemClient::VisualRect() for cached display items.

See bug for details.

BUG=724263
==========

to

==========
Don't access DisplayItemClient::VisualRect() for cached display items.

See bug for details.

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

[Xianzhu, 2017-05-19 00:09:11]

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

[commit-bot: I haz the power, 2017-05-19 00:10:38]

Dry run: CQ is trying da patch.

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

[Xianzhu, 2017-05-19 00:16:32]

Description was changed from

==========
Don't access DisplayItemClient::VisualRect() for cached display items.

See bug for details.

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

to

==========
Don't access DisplayItemClient::VisualRect() for cached display items.

- Access DisplayItemClient::VisualRect() when a new DisplayItem is
  created, and save it in the new DisplayItem.
- When a cached display item is copied, don't access DisplayItemClient::
  VisualRect() but fetch from the cached DisplayItem.
- Remove DisplayItemList::visual_rects_.
- Save original visual rects (LayoutRect) and apply GraphicsLayer offset
  with EnclosingIntRect() when the display items are added into
  WebDisplayItemList.

Please see the bug for the reason.

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

[Xianzhu, 2017-05-19 00:27:41]

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

[Xianzhu, 2017-05-19 00:27:42]

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:202:
visual_rect_(client.VisualRect()),
For now save visual rect for all display items because
- Besides DrawingDisplayItem, we also need visual rect for FilterDisplayItem and
ForeignLayerDisplayItem for SPv2 raster invalidation;
- We also need to save the old visual rect in the "invalid display item" in the
original cached display item for SPv2 raster invalidation;
- This doesn't increase memory usage because this replaces
DisplayItemList::visual_rects_.

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
(right):

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:42:
DisplayItem& AppendByMoving(DisplayItem& item) {
This is inlined because this function is a hot spot during painting when most of
the display items are cached.

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:57:
item.visual_rect_ = result.visual_rect_;
The above line is new, to save the old visual rect.

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemListTest.cpp
(left):

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemListTest.cpp:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
This is replaced by DrawingDisplayItemTest, because DisplayItemList no longer
manages visual rects.

[wkorman, 2017-05-19 00:55:08]

lgtm

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp
(right):

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp:93:
graphics_layer_->OffsetFromLayoutObjectWithSubpixelAccumulation(),
Does this add some small overhead vs. ToT, as (I think) we used to only do this
once per new item and save thereafter, whereas now we do it each time we append
regardless of old or new?

Seems ok but figured worth checkpoint as part of the review.

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
(right):

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:42:
DisplayItem& AppendByMoving(DisplayItem& item) {
On 2017/05/19 00:27:42, Xianzhu wrote:
> This is inlined because this function is a hot spot during painting when most
of
> the display items are cached.

When do we use ALWAYS_INLINE? I see PaintPropertyTreebuilder uses that macro.

[commit-bot: I haz the power, 2017-05-19 01:08:55]

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

[commit-bot: I haz the power, 2017-05-19 01:08:56]

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-05-19 01:25:11]

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp
(right):

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp:93:
graphics_layer_->OffsetFromLayoutObjectWithSubpixelAccumulation(),
On 2017/05/19 00:55:08, wkorman wrote:
> Does this add some small overhead vs. ToT, as (I think) we used to only do
this
> once per new item and save thereafter, whereas now we do it each time we
append
> regardless of old or new?
> 
> Seems ok but figured worth checkpoint as part of the review.

Good catch.

The overhead happens when the GraphicsLayer has nothing invalidated, and we
reuse the whole cached DisplayItemList. I think we can optimize the case by also
reusing the original WebDisplayItemList instead of reconstructing it from the
cached DisplayItemList.

If anything is invalidated, regardless of this patch, we re-apply the offset for
each cached or new display item. Previously we called
DisplayItemClient::VisualRect() and now we call DisplayItem::VisualRect()
instead. The latter might be slightly faster because the call is inlined instead
of virtual.

Let's see how this affects performance tests.

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h
(right):

https://codereview.chromium.org/2894093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.h:42:
DisplayItem& AppendByMoving(DisplayItem& item) {
On 2017/05/19 00:55:08, wkorman wrote:
> On 2017/05/19 00:27:42, Xianzhu wrote:
> > This is inlined because this function is a hot spot during painting when
most
> of
> > the display items are cached.
> 
> When do we use ALWAYS_INLINE? I see PaintPropertyTreebuilder uses that macro.

We added ALWAYS_INLINE for some big private functions whose implementations are
in .cpp that are called from few places. I'm not sure why the compiler didn't
inline them even with 'inline'. It seems related to the size of the functions
and/or separation of declaration and implementation. Or perhaps just because
profiling is enabled, in the case ALWAYS_INLINE can make the profiling build
more similar to the real release build.

[Xianzhu, 2017-05-19 16:29:51]

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

[commit-bot: I haz the power, 2017-05-19 16:30:42]

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-05-19 16:35:30]

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

[commit-bot: I haz the power, 2017-05-19 16:35:30]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2017-05-19 17:01:25]

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

[Xianzhu, 2017-05-19 17:01:26]

+pdr for platform/

[pdr., 2017-05-19 17:11:35]

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

[pdr., 2017-05-19 17:11:36]

Overall I think this is a good approach for safety. I think it's simpler as
well.

I think this patch may conflict with Danakj's patch here:
https://codereview.chromium.org/2889653002
(This has landed and been reverted a few times but I think it will eventually
stick.)

Is there a way to use the record_bounds variable danakj is adding instead of
storing an additional size variable?

https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:23: class
LayoutSize;
Curious: why a LayoutSize instead of an IntSize or a FloatSize?

https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:238: const
LayoutRect& VisualRect() const { return visual_rect_; }
Can you add a comment here about what space this rect is in?

[Xianzhu, 2017-05-19 18:01:05]

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

[commit-bot: I haz the power, 2017-05-19 18:01:54]

Dry run: CQ is trying da patch.

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

[Xianzhu, 2017-05-19 18:02:41]

Description was changed from

==========
Don't access DisplayItemClient::VisualRect() for cached display items.

- Access DisplayItemClient::VisualRect() when a new DisplayItem is
  created, and save it in the new DisplayItem.
- When a cached display item is copied, don't access DisplayItemClient::
  VisualRect() but fetch from the cached DisplayItem.
- Remove DisplayItemList::visual_rects_.
- Save original visual rects (LayoutRect) and apply GraphicsLayer offset
  with EnclosingIntRect() when the display items are added into
  WebDisplayItemList.

Please see the bug for the reason.

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

to

==========
Don't access DisplayItemClient::VisualRect() for cached display items.

- Access DisplayItemClient::VisualRect() when a new DisplayItem is
  created, and save it in the new DisplayItem.
- When a cached display item is copied, don't access DisplayItemClient::
  VisualRect() but fetch from the cached DisplayItem.
- Remove DisplayItemList::visual_rects_.
- Save original visual rects (LayoutRect) and apply GraphicsLayer offset
  with EnclosingIntRect() when the display items are added into
  WebDisplayItemList.

Please see the bug for the reason.

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

[Xianzhu, 2017-05-19 18:02:41]

wangxianzhu@chromium.org changed reviewers:
- pdr@google.com

[Xianzhu, 2017-05-19 18:06:33]

https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:23: class
LayoutSize;
On 2017/05/19 17:11:36, pdr. wrote:
> Curious: why a LayoutSize instead of an IntSize or a FloatSize?

This is for the "const LayoutSize& visual_rect_offset" parameter of
AppendToWebDisplayItemList(). Previously the parameter was "const IntRect&
visual_rect".

The offset is from
GraphicsLayer::OffsetFromLayoutObjectWithSubpixelAccumulation() which is a
LayoutSize.

Added comments about the parameter.

https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:238: const
LayoutRect& VisualRect() const { return visual_rect_; }
On 2017/05/19 17:11:36, pdr. wrote:
> Can you add a comment here about what space this rect is in?

Done.

[pdr., 2017-05-19 18:10:49]

On 2017/05/19 at 18:06:33, wangxianzhu wrote:
>
https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):
> 
>
https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:23: class
LayoutSize;
> On 2017/05/19 17:11:36, pdr. wrote:
> > Curious: why a LayoutSize instead of an IntSize or a FloatSize?
> 
> This is for the "const LayoutSize& visual_rect_offset" parameter of
AppendToWebDisplayItemList(). Previously the parameter was "const IntRect&
visual_rect".
> 
> The offset is from
GraphicsLayer::OffsetFromLayoutObjectWithSubpixelAccumulation() which is a
LayoutSize.
> 
> Added comments about the parameter.
> 
>
https://codereview.chromium.org/2894093002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:238: const
LayoutRect& VisualRect() const { return visual_rect_; }
> On 2017/05/19 17:11:36, pdr. wrote:
> > Can you add a comment here about what space this rect is in?
> 
> Done.

OK LGTM

[Xianzhu, 2017-05-19 18:38:02]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-19 18:38:07]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-05-19 18:38:07]

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

[commit-bot: I haz the power, 2017-05-19 18:39:04]

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-05-19 22:39:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495219087261500,
"parent_rev": "d00385ad4d2d12bf0aede04cbe08ae4122148ed7", "commit_rev":
"3c7f1c4ecd1421d074daa5a103ea3f895c1c8724"}

[commit-bot: I haz the power, 2017-05-19 22:40:06]

Description was changed from

==========
Don't access DisplayItemClient::VisualRect() for cached display items.

- Access DisplayItemClient::VisualRect() when a new DisplayItem is
  created, and save it in the new DisplayItem.
- When a cached display item is copied, don't access DisplayItemClient::
  VisualRect() but fetch from the cached DisplayItem.
- Remove DisplayItemList::visual_rects_.
- Save original visual rects (LayoutRect) and apply GraphicsLayer offset
  with EnclosingIntRect() when the display items are added into
  WebDisplayItemList.

Please see the bug for the reason.

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

to

==========
Don't access DisplayItemClient::VisualRect() for cached display items.

- Access DisplayItemClient::VisualRect() when a new DisplayItem is
  created, and save it in the new DisplayItem.
- When a cached display item is copied, don't access DisplayItemClient::
  VisualRect() but fetch from the cached DisplayItem.
- Remove DisplayItemList::visual_rects_.
- Save original visual rects (LayoutRect) and apply GraphicsLayer offset
  with EnclosingIntRect() when the display items are added into
  WebDisplayItemList.

Please see the bug for the reason.

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

Review-Url: https://codereview.chromium.org/2894093002
Cr-Commit-Position: refs/heads/master@{#473371}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c7f1c4ecd1421d074daa5a103ea...
==========

[commit-bot: I haz the power, 2017-05-19 22:40:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3c7f1c4ecd1421d074daa5a103ea...

[michaelpg, 2017-05-22 10:12:40]

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

[michaelpg, 2017-05-22 10:12:41]

This CL has been identified as the cause of crbug.com/724920, a new delay
observed when expanding a Polymer <iron-collapse> element. Can you take a look?

[Xianzhu, 2017-05-22 16:57:13]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2899793002/ by wangxianzhu@chromium.org.

The reason for reverting is: BUG=724920,724263,725041,724936.