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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by Dmitry Stefantsov
Modified:
4 hours, 6 minutes 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=

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: 44

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+693 lines, -93 lines) Patch
M pkg/kernel/lib/transformations/closure/converter.dart View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +17 lines, -15 lines 0 comments Download
M pkg/kernel/lib/transformations/closure/rewriter.dart View 1 2 3 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 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 6 chunks +40 lines, -9 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 runtime/vm/kernel_binary.cc View 1 2 3 4 5 6 7 5 chunks +6 lines, -4 lines 0 comments Download
M runtime/vm/kernel_binary_flowgraph.h View 1 2 3 4 5 6 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 21 chunks +274 lines, -24 lines 0 comments Download
M runtime/vm/kernel_reader.cc View 1 2 3 4 5 6 1 chunk +3 lines, -1 line 0 comments Download
M runtime/vm/kernel_to_il.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M runtime/vm/kernel_to_il.cc View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M runtime/vm/object.h View 1 2 3 4 5 6 7 8 9 10 11 12 9 chunks +38 lines, -6 lines 0 comments Download
M runtime/vm/object.cc View 1 2 3 4 5 6 7 8 9 10 11 12 20 chunks +284 lines, -20 lines 0 comments Download
M runtime/vm/raw_object.h View 1 2 3 4 5 6 2 chunks +4 lines, -1 line 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 24 (8 generated)
Dmitry Stefantsov
This is the CL with the current progress on closure conversion. The changes are added ...
1 month 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_ ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
3 days, 7 hours 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 ...
3 days, 7 hours 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 ...
3 days, 7 hours 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 ...
3 days, 6 hours 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 ...
3 days, 6 hours 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 ...
2 days, 6 hours 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 ...
2 days, 5 hours 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. ...
4 hours, 24 minutes ago (2017-06-22 14:12:53 UTC) #23
Dmitry Stefantsov
4 hours, 6 minutes ago (2017-06-22 14:30:55 UTC) #24
Thank you Martin for excellent questions with great context in them!  Here is my
answers.

On 2017/06/20 12:13:14, kustermann wrote:
> 
> Question 1) What is the plan regarding capturing type parameters?
> 

Currently type parameters are ignored by closure conversion.  I'm working on a
design document about how to enable the support for them.  I think it would be
implemented in a few follow-up CLs.


> 
> The issue here is that, by following the parent context chain, you can get
back
> to the receiver (i.e. "this") and from there to the type argument vector, but
> there is no way of expressing this in kernel IR (to the best of my knowledge).
> 

Yes, you are right, we need to pass type parameters explicitly.


> So two solutions would be making the static closure function generic and pass
a
> type argument vector to MakeClosure().

This is a way I thing we should go here.  I'm describing something like that in
a doc.


> Another option is to put the captured
> [TypeParameter]s into the vector, but that requires new Kernel IR instructions
I
> think.
> 

I agree.  And it's probably better be done when we have reified generic types in
Kernel.  So, I think the previous idea (passing type parameters to MakeClosure)
is a better approach for now.


> 
> Question 2) It seems that the vectors are too big and might cause us to hang
on
> to dead memory. Is this on the list to fix?
> 
> 

I think, yes, but not in this CL.  I think we may also want to discuss and
evaluate how it affects performance in this case.


> 
> 
> Question 3) Why is the transformation not idempotent yet? There is no
> explanation afaik.
> 

It is already idempotent, and the comments were incorrect.  Thank you for
pointing me to that!  I updated the comments.  The reason why it wasn't
idempotent is that auxiliary methods for tear-off support were inserted
unconditionally, even if they already existed.  Now we are moving creation of
these methods to back-ends, and it effectively makes closure conversion in
Kernel idempotent.


> 
> Question/Suggestion 4) I think it might make sense to write a small design
> document explaining how it works. I've figured it out now by reading the code,
> but it would be nice for other people. Also regarding future plans, e.g.
> await/async support.
> 

The documentation on the transformations is on its way, but I can't tell
particular time when it will be published.  The doc describing how type
parameters may be added to MakeClosure is going to be the first one in the
queue, I think.
Sign in to reply to this message.

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