Add support for converted closures with explicit contexts to VM

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

[Dmitry Stefantsov, 2017-05-18 11:07:21]

dmitryas@google.com changed reviewers:
+ jensj@google.com, kmillikin@google.com

[Dmitry Stefantsov, 2017-05-18 11:07:22]

This is the CL with the current progress on closure conversion. The changes are
added to VM, but the transformation is not enabled in Fasta yet.

Currently, a transformed program can be run if closure conversion works
globally, for example, like this:

$ DART_CONFIGURATION=ReleaseX64 ./fasta run ~/dart/shorts/hello.dart

or like this:

$ DART_CONFIGURATION=ReleaseX64 ./fasta compile ~/dart/shorts/hello.dart
$ out/ReleaseX64/dart ~/dart/shorts/hello.dart.dill

There are some know issues that should be fixed. It can be done in subsequent
patch sets to this CL or in separate CLs, because the transformation is not
enabled yet. Support for partial compilation, when transformed 'platform.dill'
and transformed program are linked together, can be added in a separate CL or in
subsequent patch sets to this CL as well.

[jensj, 2017-05-18 11:19:06]

Comments in regards to the streaming.

Also, e.g. "vector_copy->can_stream_ = false;" should probably become
"vector_copy->can_stream_ = vector_copy->vector_expression_->can_stream()" (in
kernel_binary.cc) now.

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_binary_fl...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:3431: instructions +=
BuildExpressionStatement();  // read value expression.
Shouldn't this just be BuildExpression();?

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:3476: instructions += BuildExpression(); 
// read vector expression.
from the kernel_binary.cc file I would guess the comment should be "read context
vector"?

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:3491: return instructions;
Isn't there a SkipDartType or something missing?

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:5738: ASSERT(node->value() > 0);
STREAM_EXPRESSION_IF_POSSIBLE(node); ? (the same for the other ones)

[Dmitry Stefantsov, 2017-05-18 12:18:53]

Thank you for the review Jens!

In addition to using the 'STREAM_EXPRESSION_IF_POSSIBLE' macro, I changed
'can_stream_' in the new nodes to 'true'. It seems to be working. PTAL.

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_binary_fl...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:3431: instructions +=
BuildExpressionStatement();  // read value expression.
On 2017/05/18 11:19:05, jensj wrote:
> Shouldn't this just be BuildExpression();?

Yes, of course. Thanks! Fixed.

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:3476: instructions += BuildExpression(); 
// read vector expression.
On 2017/05/18 11:19:06, jensj wrote:
> from the kernel_binary.cc file I would guess the comment should be "read
context
> vector"?

Yep. Thanks!

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:3491: return instructions;
On 2017/05/18 11:19:06, jensj wrote:
> Isn't there a SkipDartType or something missing?

Yes. I should have put an invocation of SkipDartType here. Thanks! Fixed.

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2891053003/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:5738: ASSERT(node->value() > 0);
On 2017/05/18 11:19:06, jensj wrote:
> STREAM_EXPRESSION_IF_POSSIBLE(node); ? (the same for the other ones)

Yep, that explains why the code worked without the invocation of 'SkipDartType'
:)

So, I enabled streaming on the new nodes, and seems to work.

[jensj, 2017-05-18 12:23:48]

(Setting can_stream to true doesn't really do anything as that's the default).

Are we sure about setting it unconditionally to true though? If the expression
contained in it can be anything it can also be the expression that we cannot
currently stream. If you know that it's not I suppose that's fine.

[Dmitry Stefantsov, 2017-05-18 12:33:17]

On 2017/05/18 12:23:48, jensj wrote:
> (Setting can_stream to true doesn't really do anything as that's the default).
> 

Yes, but they were set to false previously for all of the new nodes, except for
'VectorCreation'. And now that 'STREAM_EXPRESSION_IF_POSSIBLE' is also there, it
effectively should mean that now the new nodes are streaming while previously
they weren't.

> Are we sure about setting it unconditionally to true though? If the expression
> contained in it can be anything it can also be the expression that we cannot
> currently stream. If you know that it's not I suppose that's fine.

The vector expression is currently either a variable get (#context) or a vector
get with index '0' of arbitrary depth in case of parent contexts (#context[0],
#context[0][0], etc). I guess they all can be streamed.

[Dmitry Stefantsov, 2017-05-18 12:34:31]

On 2017/05/18 12:33:17, Dmitry Stefantsov wrote:
> On 2017/05/18 12:23:48, jensj wrote:
> > (Setting can_stream to true doesn't really do anything as that's the
default).
> > 
> 
> Yes, but they were set to false previously for all of the new nodes, except
for
> 'VectorCreation'. And now that 'STREAM_EXPRESSION_IF_POSSIBLE' is also there,
it
> effectively should mean that now the new nodes are streaming while previously
> they weren't.
> 
> > Are we sure about setting it unconditionally to true though? If the
expression
> > contained in it can be anything it can also be the expression that we cannot
> > currently stream. If you know that it's not I suppose that's fine.
> 
> The vector expression is currently either a variable get (#context) or a
vector
> get with index '0' of arbitrary depth in case of parent contexts (#context[0],
> #context[0][0], etc). I guess they all can be streamed.

But I still have to check the value expression in 'VectorSet', I guess.

[Dmitry Stefantsov, 2017-05-18 14:34:15]

PTAL. I only changed 'can_stream_' field in 'VectorSet', so that it can be
streamed if the value that is put into a vector item can be streamed.

[Dmitry Stefantsov, 2017-05-22 11:35:44]

Description was changed from

==========
Add support for closure-converted closures with explicit contexts in 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.
==========

to

==========
Add support for closure-converted closures with explicit contexts in 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=
==========

[Dmitry Stefantsov, 2017-05-24 07:42:21]

Description was changed from

==========
Add support for closure-converted closures with explicit contexts in 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=
==========

to

==========
Add support for closure-converted closures with explicit contexts in 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=
==========

[Dmitry Stefantsov, 2017-05-24 07:42:43]

Description was changed from

==========
Add support for closure-converted closures with explicit contexts in 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=
==========

to

==========
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=
==========

[Dmitry Stefantsov, 2017-05-24 09:19:11]

Patchset #4 (id:60001) has been deleted

[Dmitry Stefantsov, 2017-05-24 12:01:52]

Patchset #5 (id:100001) has been deleted

[jensj, 2017-06-19 10:41:55]

I have a few comments, but otherwise I think the streaming stuff looks fine.
I don't think I know enough about the rest to review that.

https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_bina...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:707: VisitExpression();
A comment about what is read here would be nice.

https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_to_i...
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_to_i...
runtime/vm/kernel_to_il.cc:2616: result_ = Object::vector_type().raw();
I think you've forgotten to add something similar in
StreamingDartTypeTranslator::BuildTypeInternal ?

[Dmitry Stefantsov, 2017-06-19 11:13:31]

On 2017/06/19 10:41:55, jensj wrote:
> I have a few comments, but otherwise I think the streaming stuff looks fine.
> I don't think I know enough about the rest to review that.
> 
>
https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_bina...
> File runtime/vm/kernel_binary_flowgraph.cc (right):
> 
>
https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_bina...
> runtime/vm/kernel_binary_flowgraph.cc:707: VisitExpression();
> A comment about what is read here would be nice.
> 
>
https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_to_i...
> File runtime/vm/kernel_to_il.cc (right):
> 
>
https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_to_i...
> runtime/vm/kernel_to_il.cc:2616: result_ = Object::vector_type().raw();
> I think you've forgotten to add something similar in
> StreamingDartTypeTranslator::BuildTypeInternal ?

+Karl

Thanks you for the review, Jens!  I agree with the comments and will fix them in
the background and upload the Patch Set soon.

Karl, can you please take a look at the Dart code of this CL?

[Dmitry Stefantsov, 2017-06-19 11:13:45]

dmitryas@google.com changed reviewers:
+ karlklose@google.com

[Dmitry Stefantsov, 2017-06-19 11:23:32]

https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_bina...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:707: VisitExpression();
On 2017/06/19 10:41:54, jensj wrote:
> A comment about what is read here would be nice.

Agreed. I must have missed the spot. Thanks!

https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_to_i...
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2891053003/diff/200001/runtime/vm/kernel_to_i...
runtime/vm/kernel_to_il.cc:2616: result_ = Object::vector_type().raw();
On 2017/06/19 10:41:54, jensj wrote:
> I think you've forgotten to add something similar in
> StreamingDartTypeTranslator::BuildTypeInternal ?

Agreed. It should be there, and it looks like it's already there :)

[karlklose, 2017-06-19 11:58:15]

LGTM

https://codereview.chromium.org/2891053003/diff/240001/pkg/kernel/lib/transfo...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/2891053003/diff/240001/pkg/kernel/lib/transfo...
pkg/kernel/lib/transformations/closure/converter.dart:428: bool
hadSingleStatementBody = function.body is! Block;
Please check if this is still required.

[Dmitry Stefantsov, 2017-06-19 12:12:44]

Thanks for the review, Karl!  You were right, we the code that restores
one-statement bodies is not required any more.

https://codereview.chromium.org/2891053003/diff/240001/pkg/kernel/lib/transfo...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/2891053003/diff/240001/pkg/kernel/lib/transfo...
pkg/kernel/lib/transformations/closure/converter.dart:428: bool
hadSingleStatementBody = function.body is! Block;
On 2017/06/19 11:58:14, karlklose wrote:
> Please check if this is still required.

Thanks for pointing to this!  Apparently, DillLibraryBuilder code doesn't have
this requirement any more.  So I removed this line and the body-restoration code
in lines 437-452 below.

[Dmitry Stefantsov, 2017-06-19 12:19:29]

dmitryas@google.com changed reviewers:
+ kustermann@google.com

[kustermann, 2017-06-20 12:13:14]

A few more questions:


Question 1) What is the plan regarding capturing type parameters?

It seems type parameters don't get captured correctly atm, e.g.: 


  class Foo<T> {
    final int base;

    Foo(this.base);

    ClosureFun bar() {
      int shared = 1;
      ff(int j) {
        <T>[]; // capture type parameter
        return base + (10 * shared++) + j;
      }
      return ff;
    }
  }

Gets translated to:


  class Foo<T extends core::Object> extends core::Object {
    final field core::int base;
     constructor •(core::int base) → void
       : clo::Foo::base = base, super core::Object::•();

     method bar() → (core::int) → core::int {
       final Vector #context = MakeVector(3);
       #context[1] = this;
       #context[2] = 1;
       final (core::int) → dynamic ff = MakeClosure<(core::int) →
dynamic>(clo::closure#Foo#bar#ff, #context);
       return ff;
     }
   }
   ...
   static method closure#Foo#bar#ff(Vector #contextParameter, core::int j) →
dynamic {
     <dynamic>[];
     return (#contextParameter[1]).base.+(10.*(let final dynamic #t1 =
#contextParameter[2] in let final dynamic #t2 = #contextParameter[2] = #t1.+(1)
in #t1)).+(j);
   }


Here `<T>[]` got translated to `<dynamic>[]`.

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).

So two solutions would be making the static closure function generic and pass a
type argument vector to MakeClosure(). Another option is to put the captured
[TypeParameter]s into the vector, but that requires new Kernel IR instructions I
think.


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?

This example 
    main() {
      int i = 1;
      try {
        foo() => print(i++);
        {
          int j = 1;
          bar() => print(i++ + j++);
          callit(foo);
          callit(bar);
        }
      } finally {
        foo() => print(i++);
        {
          int j = 1;
          bar() => print(i++ + j++);
          callit(foo);
          callit(bar);
        }
      }
    }
    callit(x) => x();

gets tranformed to:

    static method main() → dynamic {
      final Vector #context = MakeVector(4);
      #context[1] = 1;
      try {
        final () → dynamic foo = MakeClosure<() →
dynamic>(tes::closure#main#foo, #context);
        {
          #context[2] = 1;
          final () → dynamic bar = MakeClosure<() →
dynamic>(tes::closure#main#bar, #context);
          tes::callit(foo);
          tes::callit(bar);
        }
      }
      finally {
        final () → dynamic foo = MakeClosure<() →
dynamic>(tes::closure#main#foo#1, #context);
        {
          #context[3] = 1;
          final () → dynamic bar = MakeClosure<() →
dynamic>(tes::closure#main#bar#1, #context);
          tes::callit(foo);
          tes::callit(bar);
        }
      }
    }

So we make a [Vector] of length 4 and all objects the [Vector] points to are
kept alive until the [Vector] dies. This is in some way a memory regression in
comparison to normal VM. So I think we need to create more [Vector]s in
sub-scopes and chain them via the parent chain.


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


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.

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/lib/transfo...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/lib/transfo...
pkg/kernel/lib/transformations/closure/converter.dart:631: // TODO(dmitryas):
Add support for tear-offs. When added, uncomment this.
Consider filing an issue about kernel-based closure support and reference it
from all commits related to it.

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
File pkg/kernel/test/closures/suite.dart (right):

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
pkg/kernel/test/closures/suite.dart:81: // The platform in these tests are
reloaded for each testscase, because the
s/platform/platform.dill/
s/are reloaded/is reloaded/
s/testscase/test case/

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
pkg/kernel/test/closures/suite.dart:83: // not an idempotent transformation yet.
Maybe add a pointer (github issues / ...), so a reader can find out why it's not
idempotent yet.

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
pkg/kernel/test/closures/suite.dart:89: }
Why the extra trip to the event loop via `new Future<Program> ...`, you can just
do

```dart

Future<Program> loadProgram() async {
  final Uri sdk = await computePatchedSdk();
  return loadProgramFronBinary(...);
}
```

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
pkg/kernel/test/closures/suite.dart:177: return process.toResult();
I think you could simplify this (slightly):

```dart

  final File generated = new File.fromUri(uri);
  try {
    final StdioProcess process = await StioProcess.run(...);
    print(process.output);
    return process.toResult();
  } finally {
    generated.parent.deleteSync(recursive: true);
  }
```

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:3170: // This method follows the logic
similar to that of
s/follows the logic .... of/follows the logic of/

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:3175: const Function& function) {
Given the significant duplication of code, maybe consider just parametrizing the
old code?

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:3198: intptr_t positional_argument_count =
ReadListLength();
Usually we try to use "const" where possible. (also in other places in the CL)

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:6040: LocalVariable* result =
MakeTemporary();
I suspect this is unsafe:

The MakeTemporary() makes the top-of-stack value addressable relative to the
frame pointer. You can use [result] (e.g. via LoadLocal(result)) as long as it's
on the stack.

But the StoreInstanceFieldInstr below will consume 2 elements from the stack
(value/context: both are Pop()ed) and will push the result. So the [result] got
Pop()ed from the stack and you can't use it anymore.

This means that [result] will point to one item past top-of-stack.

Take a look at kernel_to_il.cc:StoreLocal() to see how you can fix it.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:6059: TokenPosition::kNoSource, Pop(),
Thread::Current()->GetNextDeoptId());
Maybe introduce local variables, so one knows what gets Pop()ed.

Value* context_to_copy = Pop();

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:6080: LocalVariable* closure =
MakeTemporary();
Here for example (as opposed to above), the [closure] will stay on the stack
until the very end, so it's safe to make it addressable via [MakeTemporary] and
do several LoadLocal(closure)s.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.h:510: Fragment AllocateContext(int size);
s/int/intptr_t/

Although it makes little difference here, our "guidelines" says to prefer
intptr_t over int.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:5687: RawFunction* Function::converted_closure_function()
const {
We use python_style_naming iff it's a simple getter/setter, otherwise we use
CamelCaseNaming.

(though not all of the code is consistent :-/ )

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7093: // vector of captured variables.  When being invoked,
converted clousre
s/closre/closure

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7095: // the invocation are passed as the remaining
arguments to that function.
Maybe some sample code to make it clear how the transformed code looks like:

Just run the transformer on something simple, such as:

Function foo(int start) {
  int i = 0;
  return () => start + i++;
}

use pkg/kernel/bin/dump.dart before/after conversion and paste it here.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7111: //   * they retrieve the captured variables from
Vectors explicitly, so they
s/Vectors/[Vector]/

Maybe use [Vector] always, so it becomes clear it's actually a new heap object.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7119: // take the rest of the arguments from the
invocation.
Please add a TODO here for captured type parameters (see other comment)

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7176: Array::Handle(zone, Array::New(num_params,
Heap::kOld)));
You call very often Handle::New(), consider passing in the zone.

Zone* zone = Thread::Current()->zone();

Handle::New(zone, ...);

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7181: // Add implicit closure object as the first
parameter.
Consider adding a few new-lines here, to make the code more readable.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:16139: //
closure_function.set_context_scope(Object::empty_context_scope());
commented code

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.h#ne...
runtime/vm/object.h:2415: // Return the converted closure function created for
this function.
s/Return/Returns/

also above in the old code.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.h#ne...
runtime/vm/object.h:2418: void DropUncompiledConvertedClosureFunction() const;
A reader of this could would be very curious what the difference between a
"implicit closure function" and a "converted closure function" is.

Consider adding a length comment somewhere explaining the difference (or add a
pointer to look where to find the documentation)

[kustermann, 2017-06-20 12:42:15]

Apart from these comments, the CL looks good!

Maybe kevin should give the final ok to land it.

[Dmitry Stefantsov, 2017-06-22 14:12:53]

Thank you for the elaborate review, Martin!

I'm sorry it's a bit too large CL.  I definitely should have split it into
multiple CLs.  So, I would like to make essential fixes here, and do other
changes in follow-up CLs.

I implemented small fixes in a new Patch Set in this CL (uploaded recently) and
am working on your other suggestions to add them in follow-up Patch Sets to this
CL a bit later.  In the meantime I would really appreciate if you look at my
questions to some of your suggestions, so that I have a better idea of how to
implement them.

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/lib/transfo...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/lib/transfo...
pkg/kernel/lib/transformations/closure/converter.dart:631: // TODO(dmitryas):
Add support for tear-offs. When added, uncomment this.
On 2017/06/20 12:13:13, kustermann wrote:
> Consider filing an issue about kernel-based closure support and reference it
> from all commits related to it.

Good idea!  I was thinking about doing it, but then realized that tear-offs are
probably won't be implemented in Kernel, but rather at back-ends.  The reason
for this is that it's easier to implement it having the global knowledge and
doing it lazily at link time or even more lazily at invocation and use time
(which is exactly how it happens now in VM).

So, given that tear-offs probably won't be implemented in a Kernel
transformation, this and similar comments should be removed along with the
accompanying code in a follow-up CL.

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
File pkg/kernel/test/closures/suite.dart (right):

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
pkg/kernel/test/closures/suite.dart:81: // The platform in these tests are
reloaded for each testscase, because the
On 2017/06/20 12:13:13, kustermann wrote:
> s/platform/platform.dill/
> s/are reloaded/is reloaded/
> s/testscase/test case/

Thanks for the suggestions!  Actually, after disabling representation of
tear-offs via closures in Kernel, the statement about closure conversion being
not idempotent holds no longer.  So, this comment is deleted, and a single
instance of the loaded 'platform.dill' is lazily cached in this method and used
for every test.

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
pkg/kernel/test/closures/suite.dart:83: // not an idempotent transformation yet.
On 2017/06/20 12:13:13, kustermann wrote:
> Maybe add a pointer (github issues / ...), so a reader can find out why it's
not
> idempotent yet.

As stated above, after dealing with tear-offs was left for back-ends, closure
conversion became idempotent.  But thanks for pointing to this comment!  It
didn't make sense now, and it should have been deleted.

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
pkg/kernel/test/closures/suite.dart:89: }
On 2017/06/20 12:13:13, kustermann wrote:
> Why the extra trip to the event loop via `new Future<Program> ...`, you can
just
> do
> 
> ```dart
> 
> Future<Program> loadProgram() async {
>   final Uri sdk = await computePatchedSdk();
>   return loadProgramFronBinary(...);
> }
> ```

Good finding!

I think I was trying to lazily cache an instance of `Future<Program>` at some
point in a code like this: `program ??= new Future<Program>(() async ...`  And
then later I wanted to reload 'platform.dill' for each test and `program ??= `
part was removed, leaving this ugly method body.  Sorry for that!

I rewrote the method now, as stated above, using lazy caching and the `program
??= ` part, but now it caches an instance of `Program`, not `Future<Program>`,
and does it without an additional async wrapper.

https://codereview.chromium.org/2891053003/diff/260001/pkg/kernel/test/closur...
pkg/kernel/test/closures/suite.dart:177: return process.toResult();
On 2017/06/20 12:13:13, kustermann wrote:
> I think you could simplify this (slightly):
> 
> ```dart
> 
>   final File generated = new File.fromUri(uri);
>   try {
>     final StdioProcess process = await StioProcess.run(...);
>     print(process.output);
>     return process.toResult();
>   } finally {
>     generated.parent.deleteSync(recursive: true);
>   }
> ```

Agreed.  I like this changed version better!  I changed the code accordingly.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:3170: // This method follows the logic
similar to that of
On 2017/06/20 12:13:14, kustermann wrote:
> s/follows the logic .... of/follows the logic of/

Agreed.  It reads better.  Thanks!

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:3175: const Function& function) {
On 2017/06/20 12:13:13, kustermann wrote:
> Given the significant duplication of code, maybe consider just parametrizing
the
> old code?

I agree, we may benefit from the parametrization.  But I would rather do it in a
separate CL.  What are your thoughts, Martin?

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:3198: intptr_t positional_argument_count =
ReadListLength();
On 2017/06/20 12:13:14, kustermann wrote:
> Usually we try to use "const" where possible. (also in other places in the CL)

Do you mean something like `const intptr_t positional_argument_count ...`?  I
can make this change, but this variable declaration is the same like the similar
one from `BuildGraphOfImplicitClosureConversion` that I took as the basis. 
Should I then also make the changes there?

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:6040: LocalVariable* result =
MakeTemporary();
On 2017/06/20 12:13:14, kustermann wrote:
> I suspect this is unsafe:
> 
> The MakeTemporary() makes the top-of-stack value addressable relative to the
> frame pointer. You can use [result] (e.g. via LoadLocal(result)) as long as
it's
> on the stack.
> 
> But the StoreInstanceFieldInstr below will consume 2 elements from the stack
> (value/context: both are Pop()ed) and will push the result. So the [result]
got
> Pop()ed from the stack and you can't use it anymore.
> 
> This means that [result] will point to one item past top-of-stack.
> 
> Take a look at kernel_to_il.cc:StoreLocal() to see how you can fix it.

Thank you so much for pointing me to that!  Indeed this code should be
rewritten.  I'll upload a Patch Set in this CL fixing this.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:6059: TokenPosition::kNoSource, Pop(),
Thread::Current()->GetNextDeoptId());
On 2017/06/20 12:13:13, kustermann wrote:
> Maybe introduce local variables, so one knows what gets Pop()ed.
> 
> Value* context_to_copy = Pop();

Thanks!  That would read better indeed.  Done.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:6080: LocalVariable* closure =
MakeTemporary();
On 2017/06/20 12:13:14, kustermann wrote:
> Here for example (as opposed to above), the [closure] will stay on the stack
> until the very end, so it's safe to make it addressable via [MakeTemporary]
and
> do several LoadLocal(closure)s.

Yep.  Now I see it.  Thanks!

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.h:510: Fragment AllocateContext(int size);
On 2017/06/20 12:13:14, kustermann wrote:
> s/int/intptr_t/
> 
> Although it makes little difference here, our "guidelines" says to prefer
> intptr_t over int.

I agree that using `intptr_t` here is more aligned with our conventions, but
this method redirects to `AllocateContext(int)` method of `FlowGraphBuilder` in
runtime/vm/kernel_to_il.h.  In turn, it should be in check with
`PushContext(int)` of `FlowGraphBuilder` which sometimes is called with an `int`
obtained from `num_context_variables` of `LocalScope`.

So, changing `int` to `intptr_t` here would require changing it in all those
places.  I don't mind trying to do it, but it may affect some areas that I might
be unaware of.  It would be great to know your opinion on this, Martin.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:5687: RawFunction* Function::converted_closure_function()
const {
On 2017/06/20 12:13:14, kustermann wrote:
> We use python_style_naming iff it's a simple getter/setter, otherwise we use
> CamelCaseNaming.
> 
> (though not all of the code is consistent :-/ )

Yep :)  I followed the style that is used for `implicit_closure_function`,
because they are similar in many ways.  Should I change them both?

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7093: // vector of captured variables.  When being invoked,
converted clousre
On 2017/06/20 12:13:14, kustermann wrote:
> s/closre/closure

Thanks!  Fixed.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7095: // the invocation are passed as the remaining
arguments to that function.
On 2017/06/20 12:13:14, kustermann wrote:
> Maybe some sample code to make it clear how the transformed code looks like:
> 
> Just run the transformer on something simple, such as:
> 
> Function foo(int start) {
>   int i = 0;
>   return () => start + i++;
> }
> 
> use pkg/kernel/bin/dump.dart before/after conversion and paste it here.

Good idea!  I added the example of the Kernel code before and after
transformation.

One thing to note is that I figured that it's better to use
`pkg/front_end/tool/fasta dump-ir ...` to get textual representation of Kernel
code, because it doesn't run transformations itself before showing you the code.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7111: //   * they retrieve the captured variables from
Vectors explicitly, so they
On 2017/06/20 12:13:14, kustermann wrote:
> s/Vectors/[Vector]/
> 
> Maybe use [Vector] always, so it becomes clear it's actually a new heap
object.

Good idea.  Thanks!  Fixed.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7119: // take the rest of the arguments from the
invocation.
On 2017/06/20 12:13:14, kustermann wrote:
> Please add a TODO here for captured type parameters (see other comment)

Thanks!  I put a TODO here and referenced http://dartbug.com/29181.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7176: Array::Handle(zone, Array::New(num_params,
Heap::kOld)));
On 2017/06/20 12:13:14, kustermann wrote:
> You call very often Handle::New(), consider passing in the zone.
> 
> Zone* zone = Thread::Current()->zone();
> 
> Handle::New(zone, ...);

Thanks!  I'll upload the fix in a follow-up Patch Set to this CL.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:7181: // Add implicit closure object as the first
parameter.
On 2017/06/20 12:13:14, kustermann wrote:
> Consider adding a few new-lines here, to make the code more readable.

Thanks!  The code here is pretty dense indeed :)

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.cc#n...
runtime/vm/object.cc:16139: //
closure_function.set_context_scope(Object::empty_context_scope());
On 2017/06/20 12:13:14, kustermann wrote:
> commented code

Thank you!  I should have removed that.  Fixed.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.h#ne...
runtime/vm/object.h:2415: // Return the converted closure function created for
this function.
On 2017/06/20 12:13:14, kustermann wrote:
> s/Return/Returns/
> 
> also above in the old code.

Thanks!  Fixed.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/object.h#ne...
runtime/vm/object.h:2418: void DropUncompiledConvertedClosureFunction() const;
On 2017/06/20 12:13:14, kustermann wrote:
> A reader of this could would be very curious what the difference between a
> "implicit closure function" and a "converted closure function" is.
> 
> Consider adding a length comment somewhere explaining the difference (or add a
> pointer to look where to find the documentation)

I think the comment on `Function::ConvertedClosureFunction` in
runtime/vm/object.cc partially explains the difference.  I'll put a reference to
it here.  I also added a comment above, for `ImplicitClosureFunction` stating
that it is used for tear-offs.

[Dmitry Stefantsov, 2017-06-22 14:30:55]

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.

[kustermann, 2017-06-26 10:32:37]

Since it's not enabled yet and the remaining things are being worked on: lgtm

Though please fix the flow graph builder issue I pointed out already in the last
review.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:3175: const Function& function) {
On 2017/06/22 14:12:52, Dmitry Stefantsov wrote:
> On 2017/06/20 12:13:13, kustermann wrote:
> > Given the significant duplication of code, maybe consider just parametrizing
> the
> > old code?
> 
> I agree, we may benefit from the parametrization.  But I would rather do it in
a
> separate CL.  What are your thoughts, Martin?

Just keep it then.

https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:3198: intptr_t positional_argument_count =
ReadListLength();
On 2017/06/22 14:12:52, Dmitry Stefantsov wrote:
> On 2017/06/20 12:13:14, kustermann wrote:
> > Usually we try to use "const" where possible. (also in other places in the
CL)
> 
> Do you mean something like `const intptr_t positional_argument_count ...`?  I
> can make this change, but this variable declaration is the same like the
similar
> one from `BuildGraphOfImplicitClosureConversion` that I took as the basis. 
> Should I then also make the changes there?

It's fine to just change your code. The benefit of const (or 'final' in dart) is
that one knows there's no other assignment to the variable.

https://codereview.chromium.org/2891053003/diff/280001/runtime/vm/kernel_bina...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2891053003/diff/280001/runtime/vm/kernel_bina...
runtime/vm/kernel_binary_flowgraph.cc:6049: instructions += LoadLocal(result);
As I pointed out before, you should only `LoadLocal(result)` as long as it's on
the stack.

Maybe testing it with a debug build of the VM and low optimization threshold
would reveal it.

[Dmitry Stefantsov, 2017-06-26 13:49:54]

On 2017/06/26 10:32:37, kustermann wrote:
> Since it's not enabled yet and the remaining things are being worked on: lgtm
> 
> Though please fix the flow graph builder issue I pointed out already in the
last
> review.
> 
>
https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
> File runtime/vm/kernel_binary_flowgraph.cc (right):
> 
>
https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
> runtime/vm/kernel_binary_flowgraph.cc:3175: const Function& function) {
> On 2017/06/22 14:12:52, Dmitry Stefantsov wrote:
> > On 2017/06/20 12:13:13, kustermann wrote:
> > > Given the significant duplication of code, maybe consider just
parametrizing
> > the
> > > old code?
> > 
> > I agree, we may benefit from the parametrization.  But I would rather do it
in
> a
> > separate CL.  What are your thoughts, Martin?
> 
> Just keep it then.
> 
>
https://codereview.chromium.org/2891053003/diff/260001/runtime/vm/kernel_bina...
> runtime/vm/kernel_binary_flowgraph.cc:3198: intptr_t positional_argument_count
=
> ReadListLength();
> On 2017/06/22 14:12:52, Dmitry Stefantsov wrote:
> > On 2017/06/20 12:13:14, kustermann wrote:
> > > Usually we try to use "const" where possible. (also in other places in the
> CL)
> > 
> > Do you mean something like `const intptr_t positional_argument_count ...`? 
I
> > can make this change, but this variable declaration is the same like the
> similar
> > one from `BuildGraphOfImplicitClosureConversion` that I took as the basis. 
> > Should I then also make the changes there?
> 
> It's fine to just change your code. The benefit of const (or 'final' in dart)
is
> that one knows there's no other assignment to the variable.
> 
>
https://codereview.chromium.org/2891053003/diff/280001/runtime/vm/kernel_bina...
> File runtime/vm/kernel_binary_flowgraph.cc (right):
> 
>
https://codereview.chromium.org/2891053003/diff/280001/runtime/vm/kernel_bina...
> runtime/vm/kernel_binary_flowgraph.cc:6049: instructions += LoadLocal(result);
> As I pointed out before, you should only `LoadLocal(result)` as long as it's
on
> the stack.
> 
> Maybe testing it with a debug build of the VM and low optimization threshold
> would reveal it.

Thank you for the review, Martin!  I'll add everything that is marked as such in
my previous comments before landing the CL, and fixing the flow graph builder
issue is the most important change there.

When it's ready, I'll ask Kevin for a review.

[Dmitry Stefantsov, 2017-06-29 13:35:59]

Kevin, can you please take a look at this CL?  I've followed Martins suggestions
and changed the code accordingly.  The only two places where I didn't take any
actions were the following:

* I didn't rename converted_closure_function to ConvertedClosureFunction for two
reasons: (1) it looks like simple getter function to me (at least, it's more
simple than implicit_closure_function), so I think it should follow the
convention for getter names, and (2) there's already a method called
ConvertedClosureFunction in the same class, and it generates a function that
will be further retrieved by converted_closure_function.

* I didn't added any Zone instances in New calls, because to me it looks like
all applicable places already have Zone instances.

Jens, can you also take a look and give your opinion on how I integrated the
recent changes in streaming?

Thanks!

[jensj, 2017-06-29 13:43:47]

Integration of recent streaming changes LGTM.

[Dmitry Stefantsov, 2017-07-21 09:45:23]

Description was changed from

==========
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=
==========

to

==========
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
==========

[Dmitry Stefantsov, 2017-07-21 09:45:25]

Committed patchset #23 (id:480001) manually as
54752407253caec67a2c5d52de6331316a9da6f4 (presubmit successful).