Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

Now we can allocate PrePaintTreeWalkContext in stack because it becomes
much smaller (264 bytes -> 96 bytes which is also much smaller than
PaintInvalidationState (168 bytes) used in non-slimmingPaintInvalidation).

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

Review-Url: https://codereview.chromium.org/2741833005
Cr-Commit-Position: refs/heads/master@{#456265}
Committed: https://chromium.googlesource.com/chromium/src/+/82c89156820e06f1586edd0cc3850e7a5450921c

[Xianzhu, 2017-03-10 20:18:02]

Description was changed from

==========
Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

As treeBuilderContext is unique_ptr, we can allocate
PrePaintTreeWalkContext in stack because it's much smaller.
==========

to

==========
Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

As treeBuilderContext is unique_ptr, we can allocate
PrePaintTreeWalkContext in stack because it's much smaller.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-03-10 20:18:10]

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

[commit-bot: I haz the power, 2017-03-10 20:18:25]

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

[Xianzhu, 2017-03-10 20:45:32]

Description was changed from

==========
Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

As treeBuilderContext is unique_ptr, we can allocate
PrePaintTreeWalkContext in stack because it's much smaller.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

Now we can allocate PrePaintTreeWalkContext in stack because it's much
smaller (264 bytes -> 96 bytes which is also smaller than
PaintInvalidationState (168 bytes) used in non-slimmingPaintInvalidation).

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

[Xianzhu, 2017-03-10 20:46:44]

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

[Xianzhu, 2017-03-10 20:47:24]

Description was changed from

==========
Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

Now we can allocate PrePaintTreeWalkContext in stack because it's much
smaller (264 bytes -> 96 bytes which is also smaller than
PaintInvalidationState (168 bytes) used in non-slimmingPaintInvalidation).

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

to

==========
Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

Now we can allocate PrePaintTreeWalkContext in stack because it becomes
much smaller (264 bytes -> 96 bytes which is also much smaller than
PaintInvalidationState (168 bytes) used in non-slimmingPaintInvalidation).

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

[pdr., 2017-03-10 20:52:39]

Thank you for breaking this into a smaller patch. Much easier to review!

https://codereview.chromium.org/2741833005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2741833005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:33: //
PaintPropertyTreeBuilderContext is large and can lead to stack overflows
I think we may still have the issues in https://crbug.com/698653 with deep
recursion + PaintInvalidatorContext. Can you make both treeBuilderContext and
paintInvalidatorContext heap-allocated in this patch?

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

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

[commit-bot: I haz the power, 2017-03-10 21:40:14]

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-03-10 21:54:48]

https://codereview.chromium.org/2741833005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2741833005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:33: //
PaintPropertyTreeBuilderContext is large and can lead to stack overflows
On 2017/03/10 20:52:39, pdr. wrote:
> I think we may still have the issues in https://crbug.com/698653 with deep
> recursion + PaintInvalidatorContext. Can you make both treeBuilderContext and
> paintInvalidatorContext heap-allocated in this patch?

I think 96 bytes is already small enough. It's smaller than
PaintInvalidationState (168 bytes) which has been used for years. According to
the data in the web-platform-team discussion about stack overflow, most stack
overflows on m56 and m57 happened during layout and paint. None of paint
invalidation functions is in the top 20 functions where stack overflow occurred.

[pdr., 2017-03-10 22:23:22]

On 2017/03/10 at 21:54:48, wangxianzhu wrote:
>
https://codereview.chromium.org/2741833005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):
> 
>
https://codereview.chromium.org/2741833005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:33: //
PaintPropertyTreeBuilderContext is large and can lead to stack overflows
> On 2017/03/10 20:52:39, pdr. wrote:
> > I think we may still have the issues in https://crbug.com/698653 with deep
> > recursion + PaintInvalidatorContext. Can you make both treeBuilderContext
and
> > paintInvalidatorContext heap-allocated in this patch?
> 
> I think 96 bytes is already small enough. It's smaller than
PaintInvalidationState (168 bytes) which has been used for years. According to
the data in the web-platform-team discussion about stack overflow, most stack
overflows on m56 and m57 happened during layout and paint. None of paint
invalidation functions is in the top 20 functions where stack overflow occurred.

But if we were running out of stack with (recursion depth * 264), we now have
(recursion depth * 96) which should still crash with only ~3x deeper stacks.
Anyway, I'm convinced this wasn't an issue before and probably is okay now, but
lets keep an eye out for stack overflows.

LGTM

[Xianzhu, 2017-03-11 00:06:01]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-03-11 00:06:24]

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-11 02:57:19]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1489190761823140, "parent_rev":
"43624541fe9d3f03badfab95ab582d4530318154", "commit_rev":
"82c89156820e06f1586edd0cc3850e7a5450921c"}

[commit-bot: I haz the power, 2017-03-11 02:57:55]

Description was changed from

==========
Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

Now we can allocate PrePaintTreeWalkContext in stack because it becomes
much smaller (264 bytes -> 96 bytes which is also much smaller than
PaintInvalidationState (168 bytes) used in non-slimmingPaintInvalidation).

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

to

==========
Make PrePaintTreeWalkContext::treeBuilderContext a unique_ptr

This prepares for https://codereview.chromium.org/2732573003/ which
will make treeBuilderContext optional during tree walk.

Now we can allocate PrePaintTreeWalkContext in stack because it becomes
much smaller (264 bytes -> 96 bytes which is also much smaller than
PaintInvalidationState (168 bytes) used in non-slimmingPaintInvalidation).

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

Review-Url: https://codereview.chromium.org/2741833005
Cr-Commit-Position: refs/heads/master@{#456265}
Committed:
https://chromium.googlesource.com/chromium/src/+/82c89156820e06f1586edd0cc385...
==========

[commit-bot: I haz the power, 2017-03-11 02:57:56]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/82c89156820e06f1586edd0cc385...