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

Issue 2557593004: [ignition] desugar GetIterator() via bytecode rather than via AST (Closed)

Created:
4 years ago by caitp
Modified:
4 years ago
CC:
v8-reviews_googlegroups.com, rmcilroy
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[ignition] desugar GetIterator() via bytecode rather than via AST Introduces: - a new AST node representing the GetIterator() algorithm in the specification, to be used by ForOfStatement, YieldExpression (in the case of delegating yield*), and the future `for-await-of` loop proposed in http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwrap-functions. - a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not Object` checks which are common throughout the specification. This node is easily eliminated by TurboFan. The AST node is desugared specially in bytecode, rather than manually when building the AST. The benefit of this is that desugaring in the BytecodeGenerator is much simpler and easier to understand than desugaring the AST. This also reduces parse time very slightly, and allows us to use LoadIC rather than KeyedLoadIC, which seems to have better baseline performance. This results in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I believe owes to the use of the slightly faster LoadIC as opposed to the KeyedLoadIC in the baseline case. Both produce identical optimized code via TurboFan when the type check can be eliminated, and the load can be replaced with a constant value. BUG=v8:4280 R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, neis@chromium.org, jarin@chromium.org TBR=rossberg@chromium.org Committed: https://crrev.com/b5f146a02aeb28b89df6a2494ddf700a98d372ec Cr-Commit-Position: refs/heads/master@{#41555}

Patch Set 1 #

Total comments: 2

Patch Set 2 : get tests passing #

Total comments: 19

Patch Set 3 : rebase #

Total comments: 7

Patch Set 4 : first round comments #

Patch Set 5 : georg's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+275 lines, -126 lines) Patch
M src/asmjs/asm-wasm-builder.cc View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M src/ast/ast.h View 1 2 3 4 3 chunks +41 lines, -0 lines 0 comments Download
M src/ast/ast-expression-rewriter.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M src/ast/ast-literal-reindexer.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M src/ast/ast-numbering.cc View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M src/ast/ast-traversal-visitor.h View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M src/ast/prettyprinter.cc View 2 chunks +9 lines, -0 lines 0 comments Download
M src/bailout-reason.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/ast-graph-builder.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M src/compiler/ast-loop-assignment-analyzer.cc View 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/bytecode-graph-builder.h View 2 chunks +4 lines, -0 lines 0 comments Download
M src/compiler/bytecode-graph-builder.cc View 1 2 3 4 2 chunks +12 lines, -0 lines 0 comments Download
M src/crankshaft/hydrogen.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M src/crankshaft/typing.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/full-codegen/full-codegen.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/interpreter/bytecode-array-builder.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M src/interpreter/bytecode-array-builder.cc View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M src/interpreter/bytecode-array-writer.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/interpreter/bytecode-generator.h View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 2 3 4 2 chunks +28 lines, -0 lines 0 comments Download
M src/interpreter/bytecodes.h View 1 2 3 4 4 chunks +4 lines, -0 lines 0 comments Download
M src/interpreter/interpreter.cc View 1 2 3 4 1 chunk +43 lines, -0 lines 0 comments Download
M src/parsing/parser.h View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 chunks +7 lines, -41 lines 0 comments Download
M src/parsing/pattern-rewriter.cc View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M src/runtime/runtime.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M src/runtime/runtime-internal.cc View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M test/cctest/compiler/test-loop-assignment-analysis.cc View 1 2 3 1 chunk +2 lines, -21 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ForOf.golden View 1 2 12 chunks +37 lines, -32 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/Generators.golden View 1 2 8 chunks +16 lines, -15 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/Modules.golden View 1 2 3 4 11 chunks +11 lines, -11 lines 0 comments Download
M test/unittests/interpreter/bytecode-array-builder-unittest.cc View 1 2 3 4 3 chunks +8 lines, -3 lines 0 comments Download

Messages

Total messages: 50 (24 generated)
caitp
Hi, I was experimenting with an approach like this mainly because writing bytecode seems to ...
4 years ago (2016-12-06 03:36:41 UTC) #2
Benedikt Meurer
Hey Caitlin, This goes into the direction we've been discussing briefly in the past (mostly ...
4 years ago (2016-12-06 05:02:00 UTC) #8
Benedikt Meurer
We talked about ideas how to deopt into "the middle" of a bytecode earlier. In ...
4 years ago (2016-12-06 05:04:51 UTC) #10
Benedikt Meurer
https://codereview.chromium.org/2557593004/diff/1/src/interpreter/bytecode-generator.cc File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2557593004/diff/1/src/interpreter/bytecode-generator.cc#newcode2863 src/interpreter/bytecode-generator.cc:2863: .Call(method, args, feedback_index(call_slot), Call::NAMED_PROPERTY_CALL); On 2016/12/06 05:04:51, Benedikt Meurer ...
4 years ago (2016-12-06 05:06:32 UTC) #11
caitp
https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter/bytecode_expectations/ForOf.golden File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right): https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter/bytecode_expectations/ForOf.golden#newcode27 test/cctest/interpreter/bytecode_expectations/ForOf.golden:27: B(CallRuntime), U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0), well, the other downside is ...
4 years ago (2016-12-06 05:49:11 UTC) #12
rmcilroy
Approach LGTM from the point of view of the interpreter. A couple of nits and ...
4 years ago (2016-12-06 15:02:28 UTC) #17
caitp
https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecode-generator.cc File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecode-generator.cc#newcode2858 src/interpreter/bytecode-generator.cc:2858: // Let method be ? GetMethod(obj, @@iterator). On 2016/12/06 ...
4 years ago (2016-12-06 15:14:59 UTC) #18
caitp
https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter/bytecode_expectations/ForOf.golden File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right): https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter/bytecode_expectations/ForOf.golden#newcode27 test/cctest/interpreter/bytecode_expectations/ForOf.golden:27: B(CallRuntime), U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0), On 2016/12/06 15:14:58, caitp wrote: ...
4 years ago (2016-12-06 16:07:07 UTC) #19
adamk
Regarding the question about the receiver check, my vote is for strict compliance. Can you ...
4 years ago (2016-12-06 18:49:05 UTC) #20
caitp
On 2016/12/06 18:49:05, adamk wrote: > Regarding the question about the receiver check, my vote ...
4 years ago (2016-12-06 19:00:44 UTC) #21
Benedikt Meurer
I also would like us to fully understand where the performance difference comes from first. ...
4 years ago (2016-12-06 19:17:07 UTC) #22
caitp
On 2016/12/06 19:17:07, Benedikt Meurer wrote: > I also would like us to fully understand ...
4 years ago (2016-12-06 19:56:34 UTC) #23
caitp
On 2016/12/06 19:56:34, caitp wrote: > On 2016/12/06 19:17:07, Benedikt Meurer wrote: > > I ...
4 years ago (2016-12-06 19:59:41 UTC) #24
caitp
On 2016/12/06 19:59:41, caitp wrote: > On 2016/12/06 19:56:34, caitp wrote: > > On 2016/12/06 ...
4 years ago (2016-12-06 20:13:24 UTC) #25
caitp
On 2016/12/06 20:13:24, caitp wrote: > On 2016/12/06 19:59:41, caitp wrote: > > On 2016/12/06 ...
4 years ago (2016-12-06 20:14:52 UTC) #26
caitp
On 2016/12/06 20:14:52, caitp wrote: > On 2016/12/06 20:13:24, caitp wrote: > > On 2016/12/06 ...
4 years ago (2016-12-06 21:22:10 UTC) #28
adamk
Thanks for the investigation. Given that we'd need to do something funny to the AST-based ...
4 years ago (2016-12-06 23:27:36 UTC) #33
Benedikt Meurer
On 2016/12/06 23:27:36, adamk wrote: > Thanks for the investigation. Given that we'd need to ...
4 years ago (2016-12-07 04:53:03 UTC) #34
neis
nit comments that i forgot to send yesterday (some may have been addressed already): https://codereview.chromium.org/2557593004/diff/20001/src/ast/ast.h ...
4 years ago (2016-12-07 10:17:27 UTC) #35
rmcilroy
https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecode-generator.cc File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecode-generator.cc#newcode2858 src/interpreter/bytecode-generator.cc:2858: // Let method be ? GetMethod(obj, @@iterator). On 2016/12/06 ...
4 years ago (2016-12-07 10:50:42 UTC) #36
caitp
I think that's everything... lets flip the switch and see if the 20% improvement shows ...
4 years ago (2016-12-07 14:49:41 UTC) #37
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/2557593004/80001
4 years ago (2016-12-07 14:49:52 UTC) #40
commit-bot: I haz the power
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30160)
4 years ago (2016-12-07 14:53:44 UTC) #42
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/2557593004/80001
4 years ago (2016-12-07 14:55:03 UTC) #45
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years ago (2016-12-07 15:19:59 UTC) #48
commit-bot: I haz the power
4 years ago (2016-12-07 15:20:39 UTC) #50
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/b5f146a02aeb28b89df6a2494ddf700a98d372ec
Cr-Commit-Position: refs/heads/master@{#41555}

Powered by Google App Engine
This is Rietveld 408576698