Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2525243002:
[compiler] Consistently use Ignition+TurboFan for lexical variables. (Closed)
|
Created:
4 years ago by Benedikt Meurer Modified:
4 years ago CC:
v8-reviews_googlegroups.com, Clemens Hammacher Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[compiler] Consistently use Ignition+TurboFan for lexical variables.
Crankshaft has this highly unpredictable performance cliff around the
"Unsupported phi use of const or let variable", which is due to the
fact that Crankshaft refuses to do hole checks (for a couple of
reasons). So ideally we should not even try to send any lexically bound
variables to fullcodegen+Crankshaft, but instead give them to Ignition
and TurboFan.
This CL only adds the appropriate check to the AstNumbering, but doesn't
remove the functionality from fullcodegen/Crankshaft yet. This would be
step two in case this CL sticks. If you see any major performance
regressions with this CL in the range, just revert it and ping me.
See Node.js issue https://github.com/nodejs/node/issues/9729 for
additional information.
BUG=v8:5666
R=hablich@chromium.org, mstarzinger@chromium.org, rmcilroy@chromium.org
Committed: https://crrev.com/5529430dec0d8997319d46e02c473a7a4faf1933
Cr-Commit-Position: refs/heads/master@{#41445}
Patch Set 1 #Patch Set 2 : REBASE #
Total comments: 2
Depends on Patchset: Messages
Total messages: 45 (18 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...
Here's the proposed fix to avoid the "Unsupported phi use of let or const" performance cliff in the wild. Seems to clash with some asm validation, any ideas?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
LGTM (rubber-stamped).
LGTM, thanks.
On 2016/11/24 14:37:30, rmcilroy wrote: > LGTM, thanks. Rubber-stamp lgtm regarding release if the admin validator problems are resolved.
The CQ bit was checked by bmeurer@chromium.org
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
Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16896) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by bmeurer@chromium.org
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
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
Seems like this is still failing for some strange reason in asm-validation. Will need to investigate with Ross' fixes.
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: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by bmeurer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org, rmcilroy@chromium.org, hablich@chromium.org Link to the patchset: https://codereview.chromium.org/2525243002/#ps20001 (title: "REBASE")
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": 20001, "attempt_start_ts": 1480662114813950,
"parent_rev": "781cf16252a0004cd477ef937ee7a28a892bf674", "commit_rev":
"2af6b3ea08216249309988c45691a25bf6de4655"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [compiler] Consistently use Ignition+TurboFan for lexical variables. Crankshaft has this highly unpredictable performance cliff around the "Unsupported phi use of const or let variable", which is due to the fact that Crankshaft refuses to do hole checks (for a couple of reasons). So ideally we should not even try to send any lexically bound variables to fullcodegen+Crankshaft, but instead give them to Ignition and TurboFan. This CL only adds the appropriate check to the AstNumbering, but doesn't remove the functionality from fullcodegen/Crankshaft yet. This would be step two in case this CL sticks. If you see any major performance regressions with this CL in the range, just revert it and ping me. See Node.js issue https://github.com/nodejs/node/issues/9729 for additional information. BUG=v8:5666 R=hablich@chromium.org, mstarzinger@chromium.org, rmcilroy@chromium.org ========== to ========== [compiler] Consistently use Ignition+TurboFan for lexical variables. Crankshaft has this highly unpredictable performance cliff around the "Unsupported phi use of const or let variable", which is due to the fact that Crankshaft refuses to do hole checks (for a couple of reasons). So ideally we should not even try to send any lexically bound variables to fullcodegen+Crankshaft, but instead give them to Ignition and TurboFan. This CL only adds the appropriate check to the AstNumbering, but doesn't remove the functionality from fullcodegen/Crankshaft yet. This would be step two in case this CL sticks. If you see any major performance regressions with this CL in the range, just revert it and ping me. See Node.js issue https://github.com/nodejs/node/issues/9729 for additional information. BUG=v8:5666 R=hablich@chromium.org, mstarzinger@chromium.org, rmcilroy@chromium.org Committed: https://crrev.com/5529430dec0d8997319d46e02c473a7a4faf1933 Cr-Commit-Position: refs/heads/master@{#41445} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5529430dec0d8997319d46e02c473a7a4faf1933 Cr-Commit-Position: refs/heads/master@{#41445}
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
asm_wasm starts flaking here: https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%...
Message was sent while issue was closed.
On 2016/12/02 11:50:44, Michael Achenbach wrote: > asm_wasm starts flaking here: > https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%... Please give to wasm folks.
Message was sent while issue was closed.
On 2016/12/02 11:53:37, Benedikt Meurer wrote: > On 2016/12/02 11:50:44, Michael Achenbach wrote: > > asm_wasm starts flaking here: > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%... > > Please give to wasm folks. They're CC'ed. Wasm folks PTAL.
Message was sent while issue was closed.
On 2016/12/02 11:54:40, Michael Achenbach wrote: > On 2016/12/02 11:53:37, Benedikt Meurer wrote: > > On 2016/12/02 11:50:44, Michael Achenbach wrote: > > > asm_wasm starts flaking here: > > > > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%... > > > > Please give to wasm folks. > > They're CC'ed. Wasm folks PTAL. I spent some time and reduced the testcase quite a bit. I think it is a deoptimization bug, since it actually happens in JS code, right after a deoptimization happens. Filed: https://bugs.chromium.org/p/v8/issues/detail?id=5710
Message was sent while issue was closed.
This also causes a new flaky timeout on msan: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20...
Message was sent while issue was closed.
On 2016/12/02 13:29:17, Michael Achenbach wrote: > This also causes a new flaky timeout on msan: > https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20... Please skip the test that times out on msan, as also this blocks rolling.
Message was sent while issue was closed.
On 2016/12/02 13:36:48, Michael Achenbach wrote: > On 2016/12/02 13:29:17, Michael Achenbach wrote: > > This also causes a new flaky timeout on msan: > > > https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20... > > Please skip the test that times out on msan, as also this blocks rolling. Done with https://codereview.chromium.org/2542843008/.
Message was sent while issue was closed.
On 2016/12/02 14:07:52, Benedikt Meurer wrote: > On 2016/12/02 13:36:48, Michael Achenbach wrote: > > On 2016/12/02 13:29:17, Michael Achenbach wrote: > > > This also causes a new flaky timeout on msan: > > > > > > https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20... > > > > Please skip the test that times out on msan, as also this blocks rolling. > > Done with https://codereview.chromium.org/2542843008/. Thanks!
Message was sent while issue was closed.
On 2016/12/02 14:20:13, Michael Achenbach wrote: > On 2016/12/02 14:07:52, Benedikt Meurer wrote: > > On 2016/12/02 13:36:48, Michael Achenbach wrote: > > > On 2016/12/02 13:29:17, Michael Achenbach wrote: > > > > This also causes a new flaky timeout on msan: > > > > > > > > > > https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20... > > > > > > Please skip the test that times out on msan, as also this blocks rolling. > > > > Done with https://codereview.chromium.org/2542843008/. > > Thanks! Benedikt, this CL regresses the promises benchmark by over 10%. See the first spike here - https://chromeperf.appspot.com/report?sid=a6397f62515bcf6553280211ef6af517b8c...
Message was sent while issue was closed.
On 2016/12/03 01:28:49, gsathya wrote: > On 2016/12/02 14:20:13, Michael Achenbach wrote: > > On 2016/12/02 14:07:52, Benedikt Meurer wrote: > > > On 2016/12/02 13:36:48, Michael Achenbach wrote: > > > > On 2016/12/02 13:29:17, Michael Achenbach wrote: > > > > > This also causes a new flaky timeout on msan: > > > > > > > > > > > > > > > https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20... > > > > > > > > Please skip the test that times out on msan, as also this blocks rolling. > > > > > > Done with https://codereview.chromium.org/2542843008/. > > > > Thanks! > > Benedikt, this CL regresses the promises benchmark by over 10%. See the first > spike here - > https://chromeperf.appspot.com/report?sid=a6397f62515bcf6553280211ef6af517b8c... Not surprising, as some of the other promise functions now go to I+TF due to the const's in src/js/promise.js
Message was sent while issue was closed.
On 2016/12/04 13:58:16, Benedikt Meurer wrote: > On 2016/12/03 01:28:49, gsathya wrote: > > On 2016/12/02 14:20:13, Michael Achenbach wrote: > > > On 2016/12/02 14:07:52, Benedikt Meurer wrote: > > > > On 2016/12/02 13:36:48, Michael Achenbach wrote: > > > > > On 2016/12/02 13:29:17, Michael Achenbach wrote: > > > > > > This also causes a new flaky timeout on msan: > > > > > > > > > > > > > > > > > > > > > https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20... > > > > > > > > > > Please skip the test that times out on msan, as also this blocks > rolling. > > > > > > > > Done with https://codereview.chromium.org/2542843008/. > > > > > > Thanks! > > > > Benedikt, this CL regresses the promises benchmark by over 10%. See the first > > spike here - > > > https://chromeperf.appspot.com/report?sid=a6397f62515bcf6553280211ef6af517b8c... > > Not surprising, as some of the other promise functions now go to I+TF due to the > const's in src/js/promise.js About time to get rid of the .js implementation *hint* *hint* :-)
Message was sent while issue was closed.
adamk@chromium.org changed reviewers: + adamk@chromium.org
Message was sent while issue was closed.
As noted on https://codereview.chromium.org/2553523002/, inner functions declared at the top level shouldn't need to be treated as lexical variables. I'm going to dig in a bit and try to understand why they're being treated that way. Also I'd caution that making all code that uses inner functions go down the I + TF path is going to have a major effect in the wild, not just on our natives.
Message was sent while issue was closed.
https://codereview.chromium.org/2525243002/diff/20001/src/ast/ast-numbering.cc File src/ast/ast-numbering.cc (right): https://codereview.chromium.org/2525243002/diff/20001/src/ast/ast-numbering.c... src/ast/ast-numbering.cc:161: if (IsLexicalVariableMode(node->var()->mode())) { As noted on the other CL (fixing a perf regression in array.js), it seems like this check could be narrowed to IsLexicalVariableMode() && node->var()->binding_needs_init() (or maybe you don't even need IsLexicalVariableMode()). That would avoid disabling crankshaft for the case of named function expressions (which never need a hole check for the function name var). Thoughts?
Message was sent while issue was closed.
https://codereview.chromium.org/2525243002/diff/20001/src/ast/ast-numbering.cc File src/ast/ast-numbering.cc (right): https://codereview.chromium.org/2525243002/diff/20001/src/ast/ast-numbering.c... src/ast/ast-numbering.cc:161: if (IsLexicalVariableMode(node->var()->mode())) { Ah perfect, thanks a lot Adam for the investigation and the explanation. I was a bit puzzled that this affected named function expressions. I'll change it to node->var()->binding_needs_init() only. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
