Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(137)

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
Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -0 lines) Patch
M src/ast/ast-numbering.cc View 1 1 chunk +3 lines, -0 lines 2 comments Download
M src/bailout-reason.h View 1 chunk +1 line, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 45 (18 generated)
Benedikt Meurer
4 years ago (2016-11-24 08:30:33 UTC) #1
Benedikt Meurer
Here's the proposed fix to avoid the "Unsupported phi use of let or const" performance ...
4 years ago (2016-11-24 08:31:52 UTC) #4
Michael Starzinger
LGTM (rubber-stamped).
4 years ago (2016-11-24 09:23:40 UTC) #7
rmcilroy
LGTM, thanks.
4 years ago (2016-11-24 14:37:30 UTC) #8
Michael Hablich
On 2016/11/24 14:37:30, rmcilroy wrote: > LGTM, thanks. Rubber-stamp lgtm regarding release if the admin ...
4 years ago (2016-11-24 17:58:37 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2525243002/1
4 years ago (2016-11-24 18:08:24 UTC) #11
commit-bot: I haz the power
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, ...
4 years ago (2016-11-24 18:28:35 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2525243002/1
4 years ago (2016-11-26 18:36:45 UTC) #15
commit-bot: I haz the power
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/builds/8671) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, ...
4 years ago (2016-11-26 18:52:35 UTC) #17
Benedikt Meurer
Seems like this is still failing for some strange reason in asm-validation. Will need to ...
4 years ago (2016-11-26 18:53:37 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2525243002/20001
4 years ago (2016-12-02 07:01:59 UTC) #25
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years ago (2016-12-02 07:53:34 UTC) #27
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/5529430dec0d8997319d46e02c473a7a4faf1933 Cr-Commit-Position: refs/heads/master@{#41445}
4 years ago (2016-12-02 07:54:09 UTC) #29
Michael Achenbach
asm_wasm starts flaking here: https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%20-%20sim/builds/4551
4 years ago (2016-12-02 11:50:44 UTC) #31
Benedikt Meurer
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%20-%20sim/builds/4551 Please give ...
4 years ago (2016-12-02 11:53:37 UTC) #32
Michael Achenbach
On 2016/12/02 11:53:37, Benedikt Meurer wrote: > On 2016/12/02 11:50:44, Michael Achenbach wrote: > > ...
4 years ago (2016-12-02 11:54:40 UTC) #33
titzer
On 2016/12/02 11:54:40, Michael Achenbach wrote: > On 2016/12/02 11:53:37, Benedikt Meurer wrote: > > ...
4 years ago (2016-12-02 13:24:14 UTC) #34
Michael Achenbach
This also causes a new flaky timeout on msan: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20sim%20-%20MSAN/builds/12264
4 years ago (2016-12-02 13:29:17 UTC) #35
Michael Achenbach
On 2016/12/02 13:29:17, Michael Achenbach wrote: > This also causes a new flaky timeout on ...
4 years ago (2016-12-02 13:36:48 UTC) #36
Benedikt Meurer
On 2016/12/02 13:36:48, Michael Achenbach wrote: > On 2016/12/02 13:29:17, Michael Achenbach wrote: > > ...
4 years ago (2016-12-02 14:07:52 UTC) #37
Michael Achenbach
On 2016/12/02 14:07:52, Benedikt Meurer wrote: > On 2016/12/02 13:36:48, Michael Achenbach wrote: > > ...
4 years ago (2016-12-02 14:20:13 UTC) #38
gsathya
On 2016/12/02 14:20:13, Michael Achenbach wrote: > On 2016/12/02 14:07:52, Benedikt Meurer wrote: > > ...
4 years ago (2016-12-03 01:28:49 UTC) #39
Benedikt Meurer
On 2016/12/03 01:28:49, gsathya wrote: > On 2016/12/02 14:20:13, Michael Achenbach wrote: > > On ...
4 years ago (2016-12-04 13:58:16 UTC) #40
Benedikt Meurer
On 2016/12/04 13:58:16, Benedikt Meurer wrote: > On 2016/12/03 01:28:49, gsathya wrote: > > On ...
4 years ago (2016-12-04 14:00:47 UTC) #41
adamk
As noted on https://codereview.chromium.org/2553523002/, inner functions declared at the top level shouldn't need to be ...
4 years ago (2016-12-05 20:36:19 UTC) #43
adamk
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.cc#newcode161 src/ast/ast-numbering.cc:161: if (IsLexicalVariableMode(node->var()->mode())) { As noted on the other CL ...
4 years ago (2016-12-05 20:47:31 UTC) #44
Benedikt Meurer
4 years ago (2016-12-06 05:10:48 UTC) #45
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.

Powered by Google App Engine
This is Rietveld 408576698