Reduce the number of captured variables in async code, by only capturing local

Reduce the number of captured variables in async code, by only capturing local
variables that are in scope at each await location.

R=hausner@google.com

Committed: https://github.com/dart-lang/sdk/commit/1abc4ce8e91def90e642fe7bf6ffa4668acd3c83

[regis, 2015-09-02 20:58:03]

regis@google.com changed reviewers:
+ hausner@google.com

[hausner, 2015-09-02 22:07:55]

LGTM with comments.

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

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:49: LocalScope* function_top)
Rename function_top_ to async_temp_scope_ so the naming is consistent
throughout?

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:65: const char* await_temp_prefix =
Symbols::AwaitTempVarPrefix().ToCString();
Does it make sense to define this string constant as a symbol, yet the only way
we use it is to convert it to a C string to make another symbol?

You could do this instead:

symbol = String::ZoneHandle(String::NewFormatted("%d", temp_cnt_));
symbol = Symbol::FromConcat(Symbols::AwaitTempVarPrefix(), symbol);

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

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:8505: ASSERT(var != NULL);  // Not yet marked as captured.
I'm not sure the comment makes sense when read out of context of this CL. (It
does now because I see that the is_captured() has been removed from the
assertion.)

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

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/scopes.cc#newcod...
runtime/vm/scopes.cc:623: ASSERT(scope->function_level() == function_level());
Tiny detail: you could do the assertion outside of the loop and assert that
top_scope->function_level() == function_level().

[regis, 2015-09-03 01:28:04]

Thanks!

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

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:49: LocalScope* function_top)
On 2015/09/02 22:07:55, hausner wrote:
> Rename function_top_ to async_temp_scope_ so the naming is consistent
> throughout?

Done.

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:65: const char* await_temp_prefix =
Symbols::AwaitTempVarPrefix().ToCString();
On 2015/09/02 22:07:55, hausner wrote:
> Does it make sense to define this string constant as a symbol, yet the only
way
> we use it is to convert it to a C string to make another symbol?
> 
> You could do this instead:
> 
> symbol = String::ZoneHandle(String::NewFormatted("%d", temp_cnt_));
> symbol = Symbol::FromConcat(Symbols::AwaitTempVarPrefix(), symbol);

Done.

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

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:8505: ASSERT(var != NULL);  // Not yet marked as captured.
On 2015/09/02 22:07:55, hausner wrote:
> I'm not sure the comment makes sense when read out of context of this CL. (It
> does now because I see that the is_captured() has been removed from the
> assertion.)

Comment removed.

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

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/scopes.cc#newcod...
runtime/vm/scopes.cc:623: ASSERT(scope->function_level() == function_level());
On 2015/09/02 22:07:55, hausner wrote:
> Tiny detail: you could do the assertion outside of the loop and assert that
> top_scope->function_level() == function_level().

Done.

[regis, 2015-09-03 01:29:37]

Committed patchset #2 (id:20001) manually as
1abc4ce8e91def90e642fe7bf6ffa4668acd3c83 (presubmit successful).

[srdjan, 2015-09-03 01:50:47]

srdjan@google.com changed reviewers:
+ srdjan@google.com

[srdjan, 2015-09-03 01:50:47]

DBC

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

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:65: const char* await_temp_prefix =
Symbols::AwaitTempVarPrefix().ToCString();
On 2015/09/02 22:07:55, hausner wrote:
> Does it make sense to define this string constant as a symbol, yet the only
way
> we use it is to convert it to a C string to make another symbol?
> 
> You could do this instead:
> 
> symbol = String::ZoneHandle(String::NewFormatted("%d", temp_cnt_));
> symbol = Symbol::FromConcat(Symbols::AwaitTempVarPrefix(), symbol);

This is problematic as it allocates a String in the new space.

[regis, 2015-09-03 16:48:38]

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

https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer....
runtime/vm/ast_transformer.cc:65: const char* await_temp_prefix =
Symbols::AwaitTempVarPrefix().ToCString();
On 2015/09/03 01:50:47, srdjan wrote:
> On 2015/09/02 22:07:55, hausner wrote:
> > Does it make sense to define this string constant as a symbol, yet the only
> way
> > we use it is to convert it to a C string to make another symbol?
> > 
> > You could do this instead:
> > 
> > symbol = String::ZoneHandle(String::NewFormatted("%d", temp_cnt_));
> > symbol = Symbol::FromConcat(Symbols::AwaitTempVarPrefix(), symbol);
> 
> This is problematic as it allocates a String in the new space.

The previous version also allocates a new string, but does not hold onto it
either. Shouldn't the methods of Symbols copy the input from new space to old
space before returning a new Symbol?
Or maybe I do not understand the problem you are mentioning.

[srdjan, 2015-09-03 17:37:12]

On 2015/09/03 16:48:38, regis wrote:
>
https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer.cc
> File runtime/vm/ast_transformer.cc (right):
> 
>
https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer....
> runtime/vm/ast_transformer.cc:65: const char* await_temp_prefix =
> Symbols::AwaitTempVarPrefix().ToCString();
> On 2015/09/03 01:50:47, srdjan wrote:
> > On 2015/09/02 22:07:55, hausner wrote:
> > > Does it make sense to define this string constant as a symbol, yet the
only
> > way
> > > we use it is to convert it to a C string to make another symbol?
> > > 
> > > You could do this instead:
> > > 
> > > symbol = String::ZoneHandle(String::NewFormatted("%d", temp_cnt_));
> > > symbol = Symbol::FromConcat(Symbols::AwaitTempVarPrefix(), symbol);
> > 
> > This is problematic as it allocates a String in the new space.
> 
> The previous version also allocates a new string, but does not hold onto it
> either. Shouldn't the methods of Symbols copy the input from new space to old
> space before returning a new Symbol?
> Or maybe I do not understand the problem you are mentioning.

String::NewFormatted creates a String object in new space, which is a problem
for background compilation.
I will take care of it.

[regis, 2015-09-03 17:45:44]

On 2015/09/03 17:37:12, srdjan wrote:
> On 2015/09/03 16:48:38, regis wrote:
> >
>
https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer.cc
> > File runtime/vm/ast_transformer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1308163006/diff/1/runtime/vm/ast_transformer....
> > runtime/vm/ast_transformer.cc:65: const char* await_temp_prefix =
> > Symbols::AwaitTempVarPrefix().ToCString();
> > On 2015/09/03 01:50:47, srdjan wrote:
> > > On 2015/09/02 22:07:55, hausner wrote:
> > > > Does it make sense to define this string constant as a symbol, yet the
> only
> > > way
> > > > we use it is to convert it to a C string to make another symbol?
> > > > 
> > > > You could do this instead:
> > > > 
> > > > symbol = String::ZoneHandle(String::NewFormatted("%d", temp_cnt_));
> > > > symbol = Symbol::FromConcat(Symbols::AwaitTempVarPrefix(), symbol);
> > > 
> > > This is problematic as it allocates a String in the new space.
> > 
> > The previous version also allocates a new string, but does not hold onto it
> > either. Shouldn't the methods of Symbols copy the input from new space to
old
> > space before returning a new Symbol?
> > Or maybe I do not understand the problem you are mentioning.
> 
> String::NewFormatted creates a String object in new space, which is a problem
> for background compilation.
> I will take care of it.

I see. You will find uses of String::NewFormatted in parser.cc as well.
Thanks for taking care of it.