Heap allocate PrePaintTreeWalk context object

Heap allocate PrePaintTreeWalk context object

This patch makes the PrePaintTreeWalk context object heap allocated
to avoid overflowing the stack.

Using an iterative algorithm was explored in [1] but the design
is complicated because of early-out and clearPaintFlags logic.
This patch uses the existing recursive walk which is easier to
follow, but moves the large allocation out of the heap.

[1] https://codereview.chromium.org/2735823005

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

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

[pdr., 2017-03-08 01:14:07]

Description was changed from

==========
Heap allocate PrePaintTreeWalk context object

This patch makes the PrePaintTreeWalk context object heap allocated
to avoid overflowing the stack.

Using an iterative algorithm was explored in [1] but the design has
quite a bit of complexity to efficiently implement the early-out
and clearPaintFlags logic. This patch uses the existing recursive
walk which is easier to follow, but moves the large allocation out
of the heap.

[1] https://codereview.chromium.org/2735823005

BUG=698653
==========

to

==========
Heap allocate PrePaintTreeWalk context object

This patch makes the PrePaintTreeWalk context object heap allocated
to avoid overflowing the stack.

Using an iterative algorithm was explored in [1] but the design has
quite a bit of complexity to efficiently implement the early-out
and clearPaintFlags logic. This patch uses the existing recursive
walk which is easier to follow, but moves the large allocation out
of the heap.

[1] https://codereview.chromium.org/2735823005

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

[pdr., 2017-03-08 01:14:27]

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

[commit-bot: I haz the power, 2017-03-08 01:15: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-08 03:05:28]

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

[commit-bot: I haz the power, 2017-03-08 03:05:29]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-03-08 15:26:27]

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

[pdr., 2017-03-08 16:56:27]

Description was changed from

==========
Heap allocate PrePaintTreeWalk context object

This patch makes the PrePaintTreeWalk context object heap allocated
to avoid overflowing the stack.

Using an iterative algorithm was explored in [1] but the design has
quite a bit of complexity to efficiently implement the early-out
and clearPaintFlags logic. This patch uses the existing recursive
walk which is easier to follow, but moves the large allocation out
of the heap.

[1] https://codereview.chromium.org/2735823005

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

to

==========
Heap allocate PrePaintTreeWalk context object

This patch makes the PrePaintTreeWalk context object heap allocated
to avoid overflowing the stack.

Using an iterative algorithm was explored in [1] but the design
is complicated because of early-out and clearPaintFlags logic.
This patch uses the existing recursive walk which is easier to
follow, but moves the large allocation out of the heap.

[1] https://codereview.chromium.org/2735823005

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

[chrishtr, 2017-03-08 17:25:02]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-03-08 17:25:03]

lgtm

[commit-bot: I haz the power, 2017-03-08 17:25:27]

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

[Xianzhu, 2017-03-08 17:37:45]

lgtm, though I'm a bit afraid of the performance impact. Let's land and look at
the perf results.

[commit-bot: I haz the power, 2017-03-08 18:06:04]

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

[commit-bot: I haz the power, 2017-03-08 18:06:05]

Failed to apply patch for
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:258
  error: third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp: patch does
not apply

Patch:       third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
Index: third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
diff --git a/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
b/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
index
7adecea88839413b0e1330aa2849047a6175db28..48b1749b2bfb14cf5d7eb1618e2cf9d3fbafe952
100644
--- a/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
+++ b/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
@@ -234,23 +234,27 @@ bool PrePaintTreeWalk::shouldEndWalkBefore(
 
 void PrePaintTreeWalk::walk(const LayoutObject& object,
                             const PrePaintTreeWalkContext& parentContext) {
-  PrePaintTreeWalkContext context(parentContext);
-
   if (shouldEndWalkBefore(object, parentContext))
     return;
 
+  // PrePaintTreeWalkContext is large and can lead to stack overflows when
+  // recursion is deep so this context object is allocated on the heap.
+  // See: https://crbug.com/698653.
+  std::unique_ptr<PrePaintTreeWalkContext> context =
+      WTF::wrapUnique(new PrePaintTreeWalkContext(parentContext));
+
   // This must happen before updatePropertiesForSelf, because the latter reads
   // some of the state computed here.
-  updateAuxiliaryObjectProperties(object, context);
+  updateAuxiliaryObjectProperties(object, *context);
 
   m_propertyTreeBuilder.updatePropertiesForSelf(object,
-                                                context.treeBuilderContext);
+                                                context->treeBuilderContext);
   m_paintInvalidator.invalidatePaintIfNeeded(object,
-                                             context.paintInvalidatorContext);
-  m_propertyTreeBuilder.updatePropertiesForChildren(object,
-                                                   
context.treeBuilderContext);
+                                             context->paintInvalidatorContext);
+  m_propertyTreeBuilder.updatePropertiesForChildren(
+      object, context->treeBuilderContext);
 
-  invalidatePaintLayerOptimizationsIfNeeded(object, context);
+  invalidatePaintLayerOptimizationsIfNeeded(object, *context);
 
   for (const LayoutObject* child = object.slowFirstChild(); child;
        child = child->nextSibling()) {
@@ -258,19 +262,19 @@ void PrePaintTreeWalk::walk(const LayoutObject& object,
       child->getMutableForPainting().clearPaintFlags();
       continue;
     }
-    walk(*child, context);
+    walk(*child, *context);
   }
 
   if (object.isLayoutPart()) {
     const LayoutPart& layoutPart = toLayoutPart(object);
     FrameViewBase* frameViewBase = layoutPart.widget();
     if (frameViewBase && frameViewBase->isFrameView()) {
-      context.treeBuilderContext.current.paintOffset +=
+      context->treeBuilderContext.current.paintOffset +=
           layoutPart.replacedContentRect().location() -
           frameViewBase->frameRect().location();
-      context.treeBuilderContext.current.paintOffset =
-          roundedIntPoint(context.treeBuilderContext.current.paintOffset);
-      walk(*toFrameView(frameViewBase), context);
+      context->treeBuilderContext.current.paintOffset =
+          roundedIntPoint(context->treeBuilderContext.current.paintOffset);
+      walk(*toFrameView(frameViewBase), *context);
     }
     // TODO(pdr): Investigate RemoteFrameView (crbug.com/579281).
   }

[pdr., 2017-03-08 18:54:49]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-03-08 18:54:49]

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

[commit-bot: I haz the power, 2017-03-08 18:55:24]

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

[pdr., 2017-03-08 19:10:19]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2017-03-08 19:12:26]

On 2017/03/08 at 17:37:45, wangxianzhu wrote:
> lgtm, though I'm a bit afraid of the performance impact. Let's land and look
at the perf results.

I agree, am looking forward to the perf bots.

I am taking this out of the CQ so this lands after Chris's geometrymapper cache
patch (https://codereview.chromium.org/2729243002) so we can better tell the
perf impact. If you're aware of other major performance changes landing, can you
try to land them after this?

[Xianzhu, 2017-03-08 19:14:05]

On 2017/03/08 19:12:26, pdr. wrote:
> On 2017/03/08 at 17:37:45, wangxianzhu wrote:
> > lgtm, though I'm a bit afraid of the performance impact. Let's land and look
> at the perf results.
> 
> I agree, am looking forward to the perf bots.
> 
> I am taking this out of the CQ so this lands after Chris's geometrymapper
cache
> patch (https://codereview.chromium.org/2729243002) so we can better tell the
> perf impact. If you're aware of other major performance changes landing, can
you
> try to land them after this?

Sure.

[pdr., 2017-03-09 04:05:56]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-03-09 04:06:19]

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-09 04:36:36]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489032356953770,
"parent_rev": "b6ea87ae5519b2da334db4a6edfd4baa46f990a2", "commit_rev":
"ce1b788de6382f8c3725efe1fb14e9d4c639b5ab"}

[commit-bot: I haz the power, 2017-03-09 04:37:56]

Description was changed from

==========
Heap allocate PrePaintTreeWalk context object

This patch makes the PrePaintTreeWalk context object heap allocated
to avoid overflowing the stack.

Using an iterative algorithm was explored in [1] but the design
is complicated because of early-out and clearPaintFlags logic.
This patch uses the existing recursive walk which is easier to
follow, but moves the large allocation out of the heap.

[1] https://codereview.chromium.org/2735823005

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

to

==========
Heap allocate PrePaintTreeWalk context object

This patch makes the PrePaintTreeWalk context object heap allocated
to avoid overflowing the stack.

Using an iterative algorithm was explored in [1] but the design
is complicated because of early-out and clearPaintFlags logic.
This patch uses the existing recursive walk which is easier to
follow, but moves the large allocation out of the heap.

[1] https://codereview.chromium.org/2735823005

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

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

[commit-bot: I haz the power, 2017-03-09 04:37:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ce1b788de6382f8c3725efe1fb14...

[pdr., 2017-03-09 17:50:41]

On 2017/03/09 at 04:37:57, commit-bot wrote:
> Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ce1b788de6382f8c3725efe1fb14...

This appears to have had no effect on perf bots:
https://chromeperf.appspot.com/group_report?rev=455668