cc : Clean up cc clip tree

cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=642578, 642584, 642581
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[jaydasika, 2017-01-26 23:38:28]

Description was changed from

==========
cc : Cache the dynamically computed clip and visible rects


BUG=
==========

to

==========
cc : Cache the dynamically computed clip and visible rects


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jaydasika, 2017-02-09 23:17:40]

Patchset #16 (id:300001) has been deleted

[jaydasika, 2017-02-24 21:26:55]

Patchset #23 (id:460001) has been deleted

[jaydasika, 2017-03-08 22:56:12]

Description was changed from

==========
cc : Cache the dynamically computed clip and visible rects


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc : Cache the dynamically computed clip and visible rects


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[jaydasika, 2017-03-13 19:01:32]

Description was changed from

==========
cc : Cache the dynamically computed clip and visible rects


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space, target_effect_id,
layer_clipping_uses_only_local_clip, layers_are_clipped,
layers_are_clipped_when_surfaces_disabled, resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[jaydasika, 2017-03-13 19:02:29]

Description was changed from

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space, target_effect_id,
layer_clipping_uses_only_local_clip, layers_are_clipped,
layers_are_clipped_when_surfaces_disabled, resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[jaydasika, 2017-03-13 19:03:54]

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

[commit-bot: I haz the power, 2017-03-13 19:04:28]

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

[jaydasika, 2017-03-13 19:10:30]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, weiliangc@chromium.org

[jaydasika, 2017-03-13 19:10:33]

Patch 42 has a 6.8% regression in CalcDrawProps computation time for top-10k
pages on Android. After that patch, I only re-ordered computation of draw
properties and I think that shouldn't change the time taken. I will update the
results on Patch 43 (after reordering computation of draw properties) after I
get them.

[commit-bot: I haz the power, 2017-03-13 19:17:38]

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

[commit-bot: I haz the power, 2017-03-13 19:17:39]

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_...)

[jaydasika, 2017-03-13 19:21:14]

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

[commit-bot: I haz the power, 2017-03-13 19:21: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-03-13 19:31:27]

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

[commit-bot: I haz the power, 2017-03-13 19:31:28]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[jaydasika, 2017-03-13 20:23:08]

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

[commit-bot: I haz the power, 2017-03-13 20:23: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-03-13 21:21:38]

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

[commit-bot: I haz the power, 2017-03-13 21:21:39]

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

[jaydasika, 2017-03-13 21:55:00]

I have locally reproduced all layout test failures showing up on
linux_trusty_blink_rel and I don't see any visual difference between actual and
expected for all the failures. So, maybe we just need a rebaseline ? This patch
changes visible rect computation (for 3d transforms), so that maybe the reason
for why there is a difference in the first place

[jaydasika, 2017-03-14 20:48:02]

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

[commit-bot: I haz the power, 2017-03-14 20:49:12]

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-03-14 21:56:05]

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

[commit-bot: I haz the power, 2017-03-14 21:56:06]

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

[weiliangc, 2017-03-14 22:04:30]

So happy to look at this CL taking place. \o/

https://codereview.chromium.org/2655233006/diff/930001/cc/layers/layer_impl_u...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/layers/layer_impl_u...
cc/layers/layer_impl_unittest.cc:142: root->SetMasksToBounds(true);
Why is set maskstobounds needed here?

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/clip_node.cc
File cc/trees/clip_node.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/clip_node.cc#...
cc/trees/clip_node.cc:20: cached_clip_rects = std::vector<ClipRectData>(1);
Avoid raw number.

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/clip_node.h
File cc/trees/clip_node.h (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/clip_node.h#n...
cc/trees/clip_node.h:62: gfx::RectF accumulated_rect_in_screen_space;
Is this also a cached value? Could this be expressed as cached_clip_rects where
the target is screen?

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:106: if (target_transform_id ==
TransformTree::kRootNodeId) {
Is it possible to make this logic part of GetToTarget(...)?

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:576: clip_node->accumulated_rect_in_screen_space
= accumulated_clip;
Have you tried not calculating acculated_rect_in_screen_space all the time, but
update it only when needed like cache? What is the performance like?

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:659: ComputeClips(property_trees);
Also DCHECK in ComputeClips that effect tree doesn't need update.

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:688: ComputeClips(property_trees);
DCHECK like above.

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/property_tree...
cc/trees/property_tree.cc:2039: ClipRectData&
PropertyTrees::FetchClipRectFromCache(int clip_id,
Could we return pointer here to make cache miss more obvious? Does that affect
performance?

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:347: effect_node->surface_is_clipped =
Is there plan to remove surface_is_clipped? Or is it more part of clean up
effect tree work?

[jaydasika, 2017-03-15 00:55:43]

Patchset #48 (id:970001) has been deleted

[jaydasika, 2017-03-15 01:05:07]

Patchset #48 (id:990001) has been deleted

[jaydasika, 2017-03-15 01:15:10]

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

[commit-bot: I haz the power, 2017-03-15 01:15:50]

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-03-15 02:28:16]

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

[commit-bot: I haz the power, 2017-03-15 02:28:17]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[jaydasika, 2017-03-15 23:22:52]

Description was changed from

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=594675, 642584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[jaydasika, 2017-03-15 23:24:36]

Description was changed from

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=594675, 642584
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=594675, 642584, 642581
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[jaydasika, 2017-03-15 23:25:52]

Description was changed from

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=594675, 642584, 642581
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=642578, 642584, 642581
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[jaydasika, 2017-03-15 23:37:08]

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

[commit-bot: I haz the power, 2017-03-15 23:37:46]

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-03-15 23:40:39]

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

[commit-bot: I haz the power, 2017-03-15 23:40:40]

Dry run: Try jobs failed on following builders:
  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...)

[jaydasika, 2017-03-16 00:07:50]

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

[commit-bot: I haz the power, 2017-03-16 00:08:33]

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-03-16 01:38:10]

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

[commit-bot: I haz the power, 2017-03-16 01:38:11]

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

[jaydasika, 2017-03-16 01:57:44]

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

[commit-bot: I haz the power, 2017-03-16 01:58:06]

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-03-16 03:38:53]

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

[commit-bot: I haz the power, 2017-03-16 03:38:55]

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

[jaydasika, 2017-03-16 20:23:10]

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

[commit-bot: I haz the power, 2017-03-16 20:23:47]

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

[jaydasika, 2017-03-16 20:33:31]

The changes other than addressing comments from the last reviewed patch :

1) The reviewed patch's regression in CDP time was 9.2%. So, now draw properties
computation is grouped by property. So, we compute transforms for all layers,
clips for all layers and so on instead of computing all properties for a single
layer. The theory that I and ajuma@ had for this was that there was probably
some locality in transforms computation, which made it faster to compute them
all together.

2) The new path did not handle the fully_visible(both layer and render surface
are unclipped) case for visible rects that led to some layout tests failures.
Added that case and a cc unittest.

3) One SPv2 layout tests fails because surface is_clipped value is computed
inside property tree building. Added a temporary work around and will fix it in
a follow up

4) All the other layout tests just need rebaselining (slight pixel differences
are due to differences in visible rects that changes snapping?)

https://codereview.chromium.org/2655233006/diff/930001/cc/layers/layer_impl_u...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/layers/layer_impl_u...
cc/layers/layer_impl_unittest.cc:142: root->SetMasksToBounds(true);
On 2017/03/14 22:04:29, weiliangc wrote:
> Why is set maskstobounds needed here?

To make root get a clip node, otherwise when we change this later in this test,
we have to rebuild property trees instead of just updating.

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/clip_node.cc
File cc/trees/clip_node.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/clip_node.cc#...
cc/trees/clip_node.cc:20: cached_clip_rects = std::vector<ClipRectData>(1);
On 2017/03/14 22:04:29, weiliangc wrote:
> Avoid raw number.

Done.

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/clip_node.h
File cc/trees/clip_node.h (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/clip_node.h#n...
cc/trees/clip_node.h:62: gfx::RectF accumulated_rect_in_screen_space;
On 2017/03/14 22:04:29, weiliangc wrote:
> Is this also a cached value? Could this be expressed as cached_clip_rects
where
> the target is screen?

Yes, it is a cached value and it can be expressed in cached_clip_rects with
is_clipped = true and target_id = root_id, but I found that its faster to fetch
this when its stored separately (because we won't have to iterate through the
vector).

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:106: if (target_transform_id ==
TransformTree::kRootNodeId) {
On 2017/03/14 22:04:30, weiliangc wrote:
> Is it possible to make this logic part of GetToTarget(...)?

Ah, that makes sense, I added this to speed up transform computation. Adding
this it GetTo(From)Target should speed up other places where we calculate target
transforms to screen.

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:576: clip_node->accumulated_rect_in_screen_space
= accumulated_clip;
On 2017/03/14 22:04:30, weiliangc wrote:
> Have you tried not calculating acculated_rect_in_screen_space all the time,
but
> update it only when needed like cache? What is the performance like?

I tried that. The performance was almost same. I decided to go with this as the
code is much simpler here than in that approach (the other approach has a lot of
state maintenance for accumulating clips, very similar to how we accumulate
clips for clip rects).

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:659: ComputeClips(property_trees);
On 2017/03/14 22:04:29, weiliangc wrote:
> Also DCHECK in ComputeClips that effect tree doesn't need update.

Actually, after creating a fast-path of using ToScreen in GetToTarget, this
comment is no longer true, we only need transform tree updated to have an
updated ToScreen

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:688: ComputeClips(property_trees);
On 2017/03/14 22:04:30, weiliangc wrote:
> DCHECK like above.

Done.

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/property_tree...
cc/trees/property_tree.cc:2039: ClipRectData&
PropertyTrees::FetchClipRectFromCache(int clip_id,
On 2017/03/14 22:04:30, weiliangc wrote:
> Could we return pointer here to make cache miss more obvious? Does that affect
> performance?

I haven't tried it, so I don't know if it effects performance. I will change it
to a pointer. The reason I did not choose a pointer in the first place is I was
trying to make this function similar to FetchDrawTransformsFromCache

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2655233006/diff/930001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:347: effect_node->surface_is_clipped =
On 2017/03/14 22:04:30, weiliangc wrote:
> Is there plan to remove surface_is_clipped? Or is it more part of clean up
> effect tree work?

Yes, I will do that in a follow-up

[jaydasika, 2017-03-16 21:29:39]

Use Patch 52 for review. I just added back perf files for running on cluster
telemetry after all the changes.

[jaydasika, 2017-03-17 17:41:52]

After all the changes, the regression is down to ~2.5% (on patch 52 on top-10k
pages on andriod). My guess is, checking if the target is screen and returning
ToScreen in GetDrawTransforms is the reason for the improvement.

[jaydasika, 2017-03-20 18:26:52]

ping

[weiliangc, 2017-03-20 19:32:37]

In previous comment you mentioned that cache locality might be the cause for the
perf regression, I am wondering whether there are things we can experiment on.
Feel free to push to another CL. 

For example, the accumulated_rect_in_screen_space, if instead of putting it on a
node, use a vector on clip tree. Would that make a difference?

Thank you for doing all the work. Overall this looks great.

https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h
File cc/trees/clip_node.h (right):

https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h#...
cc/trees/clip_node.h:64: gfx::RectF accumulated_rect_in_screen_space;
Could you add cached_ to the name here as well just so it's clear?

[jaydasika, 2017-03-20 20:41:50]

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

[commit-bot: I haz the power, 2017-03-20 20:42:28]

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-03-20 20:46:26]

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

[commit-bot: I haz the power, 2017-03-20 20:46:28]

Dry run: Try jobs failed on following builders:
  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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jaydasika, 2017-03-20 20:48:49]

On 2017/03/20 19:32:37, weiliangc wrote:
> In previous comment you mentioned that cache locality might be the cause for
the
> perf regression, I am wondering whether there are things we can experiment on.
> Feel free to push to another CL. 
> 
> For example, the accumulated_rect_in_screen_space, if instead of putting it on
a
> node, use a vector on clip tree. Would that make a difference?
> 
> Thank you for doing all the work. Overall this looks great.
> 
> https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h
> File cc/trees/clip_node.h (right):
> 
>
https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h#...
> cc/trees/clip_node.h:64: gfx::RectF accumulated_rect_in_screen_space;
> Could you add cached_ to the name here as well just so it's clear?

My initial cache was a vector of vectors for clip rects and a vector for visible
rects, both stored on clip tree. The performance improved when I moved them to
clip nodes, but I haven't run experiments on the case where we have vector on
clip tree for visible rects and vector on each clip node for clip rects. Is that
the case you are talking about ? If yes, I will try it after this one lands.

[jaydasika, 2017-03-20 20:49:16]

https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h
File cc/trees/clip_node.h (right):

https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h#...
cc/trees/clip_node.h:64: gfx::RectF accumulated_rect_in_screen_space;
On 2017/03/20 19:32:37, weiliangc wrote:
> Could you add cached_ to the name here as well just so it's clear?

Done.

[weiliangc, 2017-03-20 21:02:39]

On 2017/03/20 at 20:48:49, jaydasika wrote:
> On 2017/03/20 19:32:37, weiliangc wrote:
> > In previous comment you mentioned that cache locality might be the cause for
the
> > perf regression, I am wondering whether there are things we can experiment
on.
> > Feel free to push to another CL. 
> > 
> > For example, the accumulated_rect_in_screen_space, if instead of putting it
on a
> > node, use a vector on clip tree. Would that make a difference?
> > 
> > Thank you for doing all the work. Overall this looks great.
> > 
> > https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h
> > File cc/trees/clip_node.h (right):
> > 
> >
https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h#...
> > cc/trees/clip_node.h:64: gfx::RectF accumulated_rect_in_screen_space;
> > Could you add cached_ to the name here as well just so it's clear?
> 
> My initial cache was a vector of vectors for clip rects and a vector for
visible rects, both stored on clip tree. The performance improved when I moved
them to clip nodes, but I haven't run experiments on the case where we have
vector on clip tree for visible rects and vector on each clip node for clip
rects. Is that the case you are talking about ? If yes, I will try it after this
one lands.

Yeah. I thought for the vector, the calculation and setting might be faster and
the accessing might be slower, so I'm curious to how that turns out?

[jaydasika, 2017-03-20 21:14:02]

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

[jaydasika, 2017-03-20 21:14:30]

On 2017/03/20 21:02:39, weiliangc wrote:
> On 2017/03/20 at 20:48:49, jaydasika wrote:
> > On 2017/03/20 19:32:37, weiliangc wrote:
> > > In previous comment you mentioned that cache locality might be the cause
for
> the
> > > perf regression, I am wondering whether there are things we can experiment
> on.
> > > Feel free to push to another CL. 
> > > 
> > > For example, the accumulated_rect_in_screen_space, if instead of putting
it
> on a
> > > node, use a vector on clip tree. Would that make a difference?
> > > 
> > > Thank you for doing all the work. Overall this looks great.
> > > 
> > >
https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h
> > > File cc/trees/clip_node.h (right):
> > > 
> > >
>
https://codereview.chromium.org/2655233006/diff/1090001/cc/trees/clip_node.h#...
> > > cc/trees/clip_node.h:64: gfx::RectF accumulated_rect_in_screen_space;
> > > Could you add cached_ to the name here as well just so it's clear?
> > 
> > My initial cache was a vector of vectors for clip rects and a vector for
> visible rects, both stored on clip tree. The performance improved when I moved
> them to clip nodes, but I haven't run experiments on the case where we have
> vector on clip tree for visible rects and vector on each clip node for clip
> rects. Is that the case you are talking about ? If yes, I will try it after
this
> one lands.
> 
> Yeah. I thought for the vector, the calculation and setting might be faster
and
> the accessing might be slower, so I'm curious to how that turns out?

Ok, I will try that in a follow-up.

[commit-bot: I haz the power, 2017-03-20 21:14:37]

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

[jaydasika, 2017-03-20 21:31:41]

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

[commit-bot: I haz the power, 2017-03-20 21: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-03-20 23:00:29]

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

[commit-bot: I haz the power, 2017-03-20 23:00:31]

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

[weiliangc, 2017-03-21 17:13:52]

LGTM

[jaydasika, 2017-03-21 17:53:10]

jaydasika@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[jaydasika, 2017-03-21 17:53:12]

+ pdr / chrishtr for blink changes.

[chrishtr, 2017-03-21 18:06:31]

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
Bug(642581) fast/forms/textarea/textarea-scrolled-mask.html [ NeedsRebaseline ]
How does it change?

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/So...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
(left):

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:421:
EXPECT_TRUE(clipNode->layers_are_clipped);
Why did these change? Is this concept irrelevanat now?

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/So...
File
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
(left):

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:106:
clipNode.resets_clip = true;
Are these fields deprecated?

[jaydasika, 2017-03-21 18:18:33]

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
Bug(642581) fast/forms/textarea/textarea-scrolled-mask.html [ NeedsRebaseline ]
On 2017/03/21 18:06:30, chrishtr wrote:
> How does it change?

This patch changes visible rects which is changing snapping and there are slight
pixel differences.

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/So...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
(left):

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:421:
EXPECT_TRUE(clipNode->layers_are_clipped);
On 2017/03/21 18:06:30, chrishtr wrote:
> Why did these change? Is this concept irrelevanat now?

This field decides whether a layer is clipped. It is now computed dynamically
while computing layer's draw properties(along with computing the layer's clip
rect).  So, its not needed in clip nodes anymore.

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/So...
File
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
(left):

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:106:
clipNode.resets_clip = true;
On 2017/03/21 18:06:30, chrishtr wrote:
> Are these fields deprecated?

Yes. (All the fields deleted were used to calculate clip_rect_in_target_space
which is now deprecated)

[chrishtr, 2017-03-21 18:20:30]

lgtm

[pdr., 2017-03-21 18:23:44]

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
Bug(642581) fast/forms/textarea/textarea-scrolled-mask.html [ NeedsRebaseline ]
On 2017/03/21 at 18:18:33, jaydasika wrote:
> On 2017/03/21 18:06:30, chrishtr wrote:
> > How does it change?
> 
> This patch changes visible rects which is changing snapping and there are
slight pixel differences.

NeedsRebaseline doesn't really work for flag expectations. Lets mark this as
failing instead.

[pdr., 2017-03-21 18:25:22]

On 2017/03/21 at 18:23:44, pdr. wrote:
>
https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
> File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):
> 
>
https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
> third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
Bug(642581) fast/forms/textarea/textarea-scrolled-mask.html [ NeedsRebaseline ]
> On 2017/03/21 at 18:18:33, jaydasika wrote:
> > On 2017/03/21 18:06:30, chrishtr wrote:
> > > How does it change?
> > 
> > This patch changes visible rects which is changing snapping and there are
slight pixel differences.
> 
> NeedsRebaseline doesn't really work for flag expectations. Lets mark this as
failing instead.

These are examples of subpixel issues, lets do something like:
Bug(589265) fast/forms/textarea/textarea-scrolled-mask.html [ Failure ]

[pdr., 2017-03-21 18:25:33]

LGTM otherwise. Nice cleanup!

[jaydasika, 2017-03-21 18:54:31]

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2655233006/diff/1170001/third_party/WebKit/La...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
Bug(642581) fast/forms/textarea/textarea-scrolled-mask.html [ NeedsRebaseline ]
On 2017/03/21 18:23:44, pdr. wrote:
> On 2017/03/21 at 18:18:33, jaydasika wrote:
> > On 2017/03/21 18:06:30, chrishtr wrote:
> > > How does it change?
> > 
> > This patch changes visible rects which is changing snapping and there are
> slight pixel differences.
> 
> NeedsRebaseline doesn't really work for flag expectations. Lets mark this as
> failing instead.

Done.

[jaydasika, 2017-03-21 18:54:42]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2017-03-21 18:54:42]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
weiliangc@chromium.org, chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2655233006/#ps1190001
(title: "review comments")

[commit-bot: I haz the power, 2017-03-21 18:55:16]

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-03-21 20:20:51]

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

[commit-bot: I haz the power, 2017-03-21 20:20:54]

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

[jaydasika, 2017-03-21 20:26:02]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2017-03-21 20:26:42]

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-03-21 20:42:13]

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

[commit-bot: I haz the power, 2017-03-21 20:42:16]

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

[jaydasika, 2017-03-21 20:57:07]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2017-03-21 20:57:08]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
weiliangc@chromium.org, chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2655233006/#ps1210001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-21 20:58:02]

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-03-21 21:36:51]

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

[commit-bot: I haz the power, 2017-03-21 21:36:53]

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

[jaydasika, 2017-03-21 21:43:31]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2017-03-21 21:44:11]

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-03-21 22:12:44]

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

[commit-bot: I haz the power, 2017-03-21 22:12:46]

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

[jaydasika, 2017-03-21 22:30:10]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:30:42]

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-03-21 23:11:24]

CQ is committing da patch.

Bot data: {"patchset_id": 1210001, "attempt_start_ts": 1490135410381400,
"parent_rev": "d528436cf0ab46778ccd56a439a250e41113fc46", "commit_rev":
"3a6b144350b8c18f94c48143414031627572dd41"}

[commit-bot: I haz the power, 2017-03-21 23:12:06]

Description was changed from

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=642578, 642584, 642581
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc : Clean up cc clip tree

* Cache the dynamically computed clip and visible rects and use them to
calculate clip and visible rects.

* Stop creating clip nodes for render surfaces.

* Delete combined_clip_in_target_space, clip_in_target_space,
  target_effect_id, layer_clipping_uses_only_local_clip,
  layers_are_clipped, layers_are_clipped_when_surfaces_disabled,
  resets_clip from clip node

* Remove the verify_clip_rect_calculations path from tests.

BUG=642578, 642584, 642581
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-03-21 23:12:09]

Committed patchset #58 (id:1210001) as
https://chromium.googlesource.com/chromium/src/+/3a6b144350b8c18f94c481434140...