Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(17)

Issue 2891053003: Add support for converted closures with explicit contexts to VM (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months ago by Dmitry Stefantsov
Modified:
3 months ago
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Add support for converted closures with explicit contexts to VM The closure-conversion transformation is not enabled yet. This commit only adds the support for it to FlowGraphBuilder and StreamingFlowGraphBuilder. More work should be done before enabling the transformation; most mportantly, the 'platform.dill' file that is used in the Kernel isolate and is loaded by VM for linking with executed programs should be separated. The former should receive a file not touched by the transformation, and the latter should receive a transformed one. BUG= R=jensj@google.com, karlklose@google.com, kustermann@google.com Committed: https://github.com/dart-lang/sdk/commit/54752407253caec67a2c5d52de6331316a9da6f4

Patch Set 1 #

Total comments: 8

Patch Set 2 : Fix issues found by Jens and enable streaming for the new nodes #

Patch Set 3 : Only stream VectorSet if the assigned value can be streamed #

Patch Set 4 : Change assertion about top-level functions sync-ness + fix some issues #

Patch Set 5 : Adjust CL to recent changes in "master" #

Patch Set 6 : Adjust CL to recent changes in "master" #

Patch Set 7 : Adjust CL to recent changes in "master" (1214e6fba6) #

Patch Set 8 : Adjust CL to recent changes in "master" (1f7d2d5439) #

Patch Set 9 : Disable closure conversion in frontend (was enabled for testing) #

Total comments: 4

Patch Set 10 : Fix "is" checks, return run step to test suit #

Patch Set 11 : Add streaming-style comment in a branch of VisitExpression #

Total comments: 2

Patch Set 12 : Return statement-to-block conversion for procedure bodies #

Total comments: 46

Patch Set 13 : Update the code according to Martin's comments #

Total comments: 1

Patch Set 14 : Apply the rest of the Martin's comments #

Patch Set 15 : Adjust CL to recent changes in "master" (34290dca77) #

Patch Set 16 : Define order for visiting constructor children in info gathering pass #

Patch Set 17 : Add clarifying comment about the order of visiting AST nodes #

Patch Set 18 : Adjust CL to recent changes in "master" (609d26a227) #

Patch Set 19 : Adjust CL to recent changes in "master" (9e8e1d8ac4) #

Patch Set 20 : Update expectations according to recent changes at frontend #

Patch Set 21 : Update testcase expectatins according to recent changes for initializers #

Patch Set 22 : Fix null context in closures in fields + update expectations #

Patch Set 23 : Temporarily disable Run step in closures test suite #

Unified diffs Side-by-side diffs Delta from patch set Stats (+670 lines, -92 lines) Patch
M pkg/kernel/lib/transformations/closure/converter.dart View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +10 lines, -1 line 0 comments Download
M pkg/kernel/lib/transformations/closure/info.dart View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +12 lines, -10 lines 0 comments Download
M pkg/kernel/lib/transformations/closure/rewriter.dart View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
M pkg/kernel/test/closures/closures.status View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +2 lines, -0 lines 0 comments Download
M pkg/kernel/test/closures/suite.dart View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 5 chunks +32 lines, -4 lines 0 comments Download
M pkg/kernel/testcases/closures/capture_this.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +3 lines, -3 lines 0 comments Download
M pkg/kernel/testcases/closures/closure_in_constructor.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +4 lines, -4 lines 0 comments Download
M pkg/kernel/testcases/closures/closure_in_initializer.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +3 lines, -3 lines 0 comments Download
M pkg/kernel/testcases/closures/closure_in_initializer_closure.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +3 lines, -3 lines 0 comments Download
M pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
M pkg/kernel/testcases/closures/contexts_in_super_initializers.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -2 lines 0 comments Download
M pkg/kernel/testcases/closures/field.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +9 lines, -9 lines 0 comments Download
M pkg/kernel/testcases/closures/for_in_closure.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +2 lines, -2 lines 0 comments Download
M pkg/kernel/testcases/closures/instance_tear_off.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 4 chunks +15 lines, -15 lines 0 comments Download
M pkg/kernel/testcases/closures/static_tear_off.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -12 lines 0 comments Download
M pkg/kernel/testcases/closures/type_variables.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +4 lines, -4 lines 0 comments Download
M pkg/kernel/testcases/closures/uncaptured_for_in_loop.dart.expect View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +2 lines, -2 lines 0 comments Download
M runtime/vm/kernel_binary_flowgraph.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +13 lines, -0 lines 0 comments Download
M runtime/vm/kernel_binary_flowgraph.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 13 chunks +242 lines, -3 lines 0 comments Download
M runtime/vm/kernel_to_il.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/kernel_to_il.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/object.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 7 chunks +34 lines, -2 lines 0 comments Download
M runtime/vm/object.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 15 chunks +266 lines, -8 lines 0 comments Download
M runtime/vm/raw_object.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +4 lines, -1 line 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 30 (9 generated)
Dmitry Stefantsov
This is the CL with the current progress on closure conversion. The changes are added ...
5 months ago (2017-05-18 11:07:22 UTC) #2
jensj
Comments in regards to the streaming. Also, e.g. "vector_copy->can_stream_ = false;" should probably become "vector_copy->can_stream_ ...
5 months ago (2017-05-18 11:19:06 UTC) #3
Dmitry Stefantsov
Thank you for the review Jens! In addition to using the 'STREAM_EXPRESSION_IF_POSSIBLE' macro, I changed ...
5 months ago (2017-05-18 12:18:53 UTC) #4
jensj
(Setting can_stream to true doesn't really do anything as that's the default). Are we sure ...
5 months ago (2017-05-18 12:23:48 UTC) #5
Dmitry Stefantsov
On 2017/05/18 12:23:48, jensj wrote: > (Setting can_stream to true doesn't really do anything as ...
5 months ago (2017-05-18 12:33:17 UTC) #6
Dmitry Stefantsov
On 2017/05/18 12:33:17, Dmitry Stefantsov wrote: > On 2017/05/18 12:23:48, jensj wrote: > > (Setting ...
5 months ago (2017-05-18 12:34:31 UTC) #7
Dmitry Stefantsov
PTAL. I only changed 'can_stream_' field in 'VectorSet', so that it can be streamed if ...
5 months ago (2017-05-18 14:34:15 UTC) #8
jensj
I have a few comments, but otherwise I think the streaming stuff looks fine. I ...
4 months ago (2017-06-19 10:41:55 UTC) #14
Dmitry Stefantsov
On 2017/06/19 10:41:55, jensj wrote: > I have a few comments, but otherwise I think ...
4 months ago (2017-06-19 11:13:31 UTC) #15
Dmitry Stefantsov
https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_binary_flowgraph.cc File runtime/vm/kernel_binary_flowgraph.cc (right): https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_binary_flowgraph.cc#newcode707 runtime/vm/kernel_binary_flowgraph.cc:707: VisitExpression(); On 2017/06/19 10:41:54, jensj wrote: > A comment ...
4 months ago (2017-06-19 11:23:32 UTC) #17
karlklose
LGTM https://codereview.chromium.org/2891053003/diff/240001/pkg/kernel/lib/transformations/closure/converter.dart File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2891053003/diff/240001/pkg/kernel/lib/transformations/closure/converter.dart#newcode428 pkg/kernel/lib/transformations/closure/converter.dart:428: bool hadSingleStatementBody = function.body is! Block; Please check ...
4 months ago (2017-06-19 11:58:15 UTC) #18
Dmitry Stefantsov
Thanks for the review, Karl! You were right, we the code that restores one-statement bodies ...
4 months ago (2017-06-19 12:12:44 UTC) #19
kustermann
A few more questions: Question 1) What is the plan regarding capturing type parameters? It ...
4 months ago (2017-06-20 12:13:14 UTC) #21
kustermann
Apart from these comments, the CL looks good! Maybe kevin should give the final ok ...
4 months ago (2017-06-20 12:42:15 UTC) #22
Dmitry Stefantsov
Thank you for the elaborate review, Martin! I'm sorry it's a bit too large CL. ...
3 months, 4 weeks ago (2017-06-22 14:12:53 UTC) #23
Dmitry Stefantsov
Thank you Martin for excellent questions with great context in them! Here is my answers. ...
3 months, 4 weeks ago (2017-06-22 14:30:55 UTC) #24
kustermann
Since it's not enabled yet and the remaining things are being worked on: lgtm Though ...
3 months, 3 weeks ago (2017-06-26 10:32:37 UTC) #25
Dmitry Stefantsov
On 2017/06/26 10:32:37, kustermann wrote: > Since it's not enabled yet and the remaining things ...
3 months, 3 weeks ago (2017-06-26 13:49:54 UTC) #26
Dmitry Stefantsov
Kevin, can you please take a look at this CL? I've followed Martins suggestions and ...
3 months, 3 weeks ago (2017-06-29 13:35:59 UTC) #27
jensj
Integration of recent streaming changes LGTM.
3 months, 3 weeks ago (2017-06-29 13:43:47 UTC) #28
Dmitry Stefantsov
3 months ago (2017-07-21 09:45:25 UTC) #30
Message was sent while issue was closed.
Committed patchset #23 (id:480001) manually as
54752407253caec67a2c5d52de6331316a9da6f4 (presubmit successful).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa