v8: fix stack overflow in recursive method

[bnoordhuis]

HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes. Make it non-recursive by storing
intermediate results in a heap-allocated list.

The first commit is the obvious way to implement it, the second commit optimizes a bit.

Switching to heap allocations might in theory have performance implications but, as a data point, it doesn't seem to affect the running time of the node.js test suite.

V8 CI (1st commit): https://ci.nodejs.org/job/node-test-commit-v8-linux/651/ (green)
V8 CI (2nd commit): https://ci.nodejs.org/job/node-test-commit-v8-linux/652/ (green)
V8 CI (3rd commit): https://ci.nodejs.org/job/node-test-commit-v8-linux/653/
Node CI (2nd commit): https://ci.nodejs.org/job/node-test-pull-request/7439/
Node CI (3rd commit): https://ci.nodejs.org/job/node-test-pull-request/7466/

See #11991.

[hashseed]

I don't expect any performance impact because this part of Crankshaft should run on the background thread anyways.

[hashseed]

You probably can use the list as a stack (instead of queue) and just pop the last element at the end of the loop? That would lessen the peak memory use.

[bnoordhuis]

Made the suggested change. I wasn't sure if walking them in a different order was sound but it computes the union so I suppose it must be.

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/653/
Node CI: https://ci.nodejs.org/job/node-test-pull-request/7466/

[hashseed]

LGTM.

FWIW 2nd commit didn't traverse the same order as the recursion did either (BFS vs DFS).

[jasnell]

LGTM but is this a backport of a change in V8? or should this land upstream first?

[hashseed]

V8 is not likely to ship a version with this patch anymore, and Crankshaft is being deprecated, so I'm not sure how much value upstreaming has. I would give lgtm if @bnoordhuis provides a CL for upstream though.

[bnoordhuis]

I'm fine with floating it as a patch. Upstreaming it would be make-work for the both of us.

[gibfahn]

Should this be backported to v6.x? Please apply dont-land if not.

[bnoordhuis]

@gibfahn Yes, v6.x and v4.x. The filename is deps/v8/src/hydrogen-gvn.cc in v4.x but the patch should otherwise apply cleanly.

[gibfahn]

Didn't apply cleanly in v4.x with the filename changed. Would you be willing to backport (assuming you think it's worth getting into a v4.x release).

It's possible I messed up the patch modification of course.

.git/rebase-apply/patch contents:

---
deps/v8/src/hydrogen-gvn.cc | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/deps/v8/src/hydrogen-gvn.cc b/deps/v8/src/hydrogen-gvn.cc
index e6ddd75..bf51bad 100644
--- a/deps/v8/src/hydrogen-gvn.cc+++ b/deps/v8/src/hydrogen-gvn.cc@@ -5,6 +5,8 @@
#include "src/crankshaft/hydrogen-gvn.h"
#include "src/crankshaft/hydrogen.h"
+#include "src/list.h"+#include "src/list-inl.h"
#include "src/v8.h"
namespace v8 {
@@ -650,19 +652,23 @@ SideEffects
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock(
HBasicBlock* dominator, HBasicBlock* dominated) {
SideEffects side_effects;
- for (int i = 0; i < dominated->predecessors()->length(); ++i) {- HBasicBlock* block = dominated->predecessors()->at(i);- if (dominator->block_id() < block->block_id() &&- block->block_id() < dominated->block_id() &&- !visited_on_paths_.Contains(block->block_id())) {- visited_on_paths_.Add(block->block_id());- side_effects.Add(block_side_effects_[block->block_id()]);- if (block->IsLoopHeader()) {- side_effects.Add(loop_side_effects_[block->block_id()]);+ List<HBasicBlock*> blocks;+ for (;;) {+ for (int i = 0; i < dominated->predecessors()->length(); ++i) {+ HBasicBlock* block = dominated->predecessors()->at(i);+ if (dominator->block_id() < block->block_id() &&+ block->block_id() < dominated->block_id() &&+ !visited_on_paths_.Contains(block->block_id())) {+ visited_on_paths_.Add(block->block_id());+ side_effects.Add(block_side_effects_[block->block_id()]);+ if (block->IsLoopHeader()) {+ side_effects.Add(loop_side_effects_[block->block_id()]);+ }+ blocks.Add(block);
}
- side_effects.Add(CollectSideEffectsOnPathsToDominatedBlock(- dominator, block));
}
+ if (blocks.is_empty()) break;+ dominated = blocks.RemoveLast();
}
return side_effects;
}

[bnoordhuis]

Benign conflict in the includes. v4.x PR: #13080