Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2956233002:
[turbofan] Further optimize spread/apply with arguments/rest parameters. (Closed)
|
Created:
3 years, 5 months ago by Benedikt Meurer Modified:
3 years, 5 months ago Reviewers:
petermarshall CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Further optimize spread/apply with arguments/rest parameters.
Extend the use list check for the arguments object/rest parameters
during apply/spread optimization to allow for more cases, such that
even in code like
function foo() {
if (arguments.length === 1) return arguments[0];
return bar.apply(this, arguments);
}
we don't need to materialize the arguments object. This obviously comes
with a phase ordering problem, which we resolve by introducing a
waitlist in the JSCallReducer, which contains the nodes that we should
check again after all the other reductions are done, and which might
then be reducible. This is not 100% ideal, but get's us closer to where
we want to be, and it's crucial to speed up Node core, especially the
event emitter.
BUG=v8:4551, v8:5511, v8:5726
R=petermarshall@chromium.org
Review-Url: https://codereview.chromium.org/2956233002
Cr-Commit-Position: refs/heads/master@{#46337}
Committed: https://chromium.googlesource.com/v8/v8/+/1c555714dbd96709376f99a8f4487e9c3abcfa74
Patch Set 1 #Patch Set 2 : Small improvement. #Patch Set 3 : Drop debug print. #
Total comments: 9
Patch Set 4 : Fix nit. #
Messages
Total messages: 26 (17 generated)
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey Peter, Here's the fix we talked about during breakfast. Please take a look. Thanks, Benedikt
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking good but let's discuss the waitlist invariants https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... File src/compiler/js-call-reducer.cc (right): https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:43: void JSCallReducer::Finalize() { I'm trying to figure out how we guarantee there is no infinite loop here: - It seems like finalize can be called more than once. - Could the reduction below trigger more things to be added to the waitlist? How do we keep track of all that? https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:47: std::set<Node*> const waitlist = std::move(waitlist_); I don't think std::move does anything useful here, but you'd have to defer to someone with stronger c++ foo than I (clemensh perhaps). https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:709: // explicitly checking that {node} only used by {LoadField} or {LoadElement}. Nit: ...{node} *is* only... https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:758: if (access.offset == JSArray::kLengthOffset) { If this is checking for LoadField on an arguments object, and we want to detect and whitelist accesses to the length, then should we just check access.offset == JSArgumentsObject::kLengthOffset ? Why the equivalence with JSArray::kLengthOffset?
https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... File src/compiler/js-call-reducer.cc (right): https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:43: void JSCallReducer::Finalize() { Finalize can be called multiple times, but only if there's a regular reduction in between. So unless any of the items in the waitlist trigger a reduction (i.e. we make progress on at least one node), the Finalize logic will not be invoked again. https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:47: std::set<Node*> const waitlist = std::move(waitlist_); It is the important bit, as it clears the waitlist_ and transfers all the items to waitlist.
https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... File src/compiler/js-call-reducer.cc (right): https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:758: if (access.offset == JSArray::kLengthOffset) { Rest parameters are JSArray instances.
LGTM with nit! thanks https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... File src/compiler/js-call-reducer.cc (right): https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:758: if (access.offset == JSArray::kLengthOffset) { On 2017/06/30 at 04:39:37, Benedikt Meurer wrote: > Rest parameters are JSArray instances. OK
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... File src/compiler/js-call-reducer.cc (right): https://codereview.chromium.org/2956233002/diff/40001/src/compiler/js-call-re... src/compiler/js-call-reducer.cc:709: // explicitly checking that {node} only used by {LoadField} or {LoadElement}. On 2017/06/29 13:25:24, petermarshall wrote: > Nit: ...{node} *is* only... Done.
The CQ bit was unchecked by bmeurer@chromium.org
The CQ bit was checked by bmeurer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petermarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2956233002/#ps60001 (title: "Fix nit.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498807304549470,
"parent_rev": "f79b3d4e975361ff8b4526eecda8add67d669cdb", "commit_rev":
"1c555714dbd96709376f99a8f4487e9c3abcfa74"}
Message was sent while issue was closed.
Description was changed from
==========
[turbofan] Further optimize spread/apply with arguments/rest parameters.
Extend the use list check for the arguments object/rest parameters
during apply/spread optimization to allow for more cases, such that
even in code like
function foo() {
if (arguments.length === 1) return arguments[0];
return bar.apply(this, arguments);
}
we don't need to materialize the arguments object. This obviously comes
with a phase ordering problem, which we resolve by introducing a
waitlist in the JSCallReducer, which contains the nodes that we should
check again after all the other reductions are done, and which might
then be reducible. This is not 100% ideal, but get's us closer to where
we want to be, and it's crucial to speed up Node core, especially the
event emitter.
BUG=v8:4551,v8:5511, v8:5726
R=petermarshall@chromium.org
==========
to
==========
[turbofan] Further optimize spread/apply with arguments/rest parameters.
Extend the use list check for the arguments object/rest parameters
during apply/spread optimization to allow for more cases, such that
even in code like
function foo() {
if (arguments.length === 1) return arguments[0];
return bar.apply(this, arguments);
}
we don't need to materialize the arguments object. This obviously comes
with a phase ordering problem, which we resolve by introducing a
waitlist in the JSCallReducer, which contains the nodes that we should
check again after all the other reductions are done, and which might
then be reducible. This is not 100% ideal, but get's us closer to where
we want to be, and it's crucial to speed up Node core, especially the
event emitter.
BUG=v8:4551,v8:5511, v8:5726
R=petermarshall@chromium.org
Review-Url: https://codereview.chromium.org/2956233002
Cr-Commit-Position: refs/heads/master@{#46337}
Committed:
https://chromium.googlesource.com/v8/v8/+/1c555714dbd96709376f99a8f4487e9c3ab...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/1c555714dbd96709376f99a8f4487e9c3ab... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
