Preparse inner functions (new try)

Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiBAdRnTiSYhazseNHFg/edit?usp=sharing

BUG=

Committed: https://crrev.com/7c73cf32c60484cdf37c84f1d61b4640e87068d7
Cr-Commit-Position: refs/heads/master@{#39719}

[marja, 2016-09-21 10:47:34]

Description was changed from

==========
Preparse inner functions.

WIP


BUG=
==========

to

==========
Preparse inner functions (new try)

WIP


BUG=
==========

[marja, 2016-09-23 08:16:17]

Description was changed from

==========
Preparse inner functions (new try)

WIP


BUG=
==========

to

==========
Preparse inner functions (new try)



Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

[marja, 2016-09-23 08:18:16]

Description was changed from

==========
Preparse inner functions (new try)



Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

to

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.


Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

[marja, 2016-09-23 08:19:26]

Description was changed from

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.


Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

to

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

[marja, 2016-09-23 08:19:50]

Description was changed from

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

to

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

[marja, 2016-09-23 08:42:24]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 08:42:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 09:00:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 09:00:23]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[marja, 2016-09-23 10:17:38]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 10:17:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2016-09-23 10:30:04]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 10:30:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2016-09-23 10:31:10]

marja@chromium.org changed reviewers:
+ adamk@chromium.org, verwaest@chromium.org

[marja, 2016-09-23 10:32:56]

verwaest@, adamk@: ptal

A new, fixed version, and now there's a design doc, too :)

This is mostly the same as the previous try; the major difference is in
Scope::ResolveVariable (we need to always do the pessimistic context allocation,
even if the inner functions happened to be eagerly compiled during that parse).

vogelheim@, nikolaos@: fyi (you're welcome to review + check out the design doc
too if you want)

[Toon Verwaest, 2016-09-23 11:51:18]

https://codereview.chromium.org/2352593002/diff/240001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2352593002/diff/240001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1595: if (try_to_resolve) {
Can't you just move !FLAG_lazy_inner_functions here? CollectNonLocals should
behave properly with that selection too; they are context allocated. You may
have to adapt tests, because the tests make assumptions about our context
allocation which have changed in this CL. If you adapt tests, add a TODO to
update it again later once we do full context analysis.

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:2716: // Determine whether we can still do the inner
function lazy parsing.
Determine whether we can still lazy parse the inner function ?

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:2732: // Inner functions will be parsed by using a
temporary Zone. After parsing, we
parsed using

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:2820: if (is_lazy_top_level_function) {
What about just merging this with the is_lazy_inner_function below, only
allowing aborting if is_lazy_top_level_function? Basically:

if (is_lazy_function) {
  bookmark scope...
   result = SkipLazy...( , is_lazy_top_level_function, ...) ;
   ...
   if (result == kLazyParsingAborted) {
     DCHECK(is_lazy_top_level_function);
     is_lazy_function = false;
     ...
    } else if (is_lazy_inner_function) {
      // By now this can be moved here since we just need to fetch all
references. Even if we'd do scope resolution it can be done here since I've
already modified DeclareArguments to happen within function parsing rather than
down below.
      scope->AnalyzePartially(main_scope, &previous_zone_ast_node_factory); 
    }
}
if (!is_lazy_function) {
  DCHECK_EQ(scope, main_scope);
  ...eager parse...
}

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:2898: if (is_lazy_inner_function) {
This can be dropped if you handle it as described above.

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:3521: // If we don't need to look at the scope, construct
a dummy scope chain
I don't see why this is necessary now but wasn't before? Nothing changed for
top-level parsing?

If this distinction isn't necessary, it seems like we can drop is_inner_function
as a parameter entirely and keep this function untouched.

[commit-bot: I haz the power, 2016-09-23 12:18:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 12:18:15]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_TIMED_OUT, no build
URL)
  v8_win_rel_ng on master.tryserver.v8 (JOB_TIMED_OUT, no build URL)

[marja, 2016-09-23 14:26:52]

Thanks for comments & discussions

I'm still working on the rest of the comments, but they're somewhat mechanical
(though, not easy), so it makes sense to adamk@ to start having a look too.

https://codereview.chromium.org/2352593002/diff/240001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2352593002/diff/240001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1595: if (try_to_resolve) {
Offline discussion:

If we don't resolve here, CollectNonLocals needs to be rewritten so that it also
resolves variables. (An important side effect of CollectNonLocals is that it
causes context allocation on the variables which need to be context allocated;
this is what LookupRecursive does here.)

However, because the unresolved variables are a linked list, the rewrite is
nontrivial, and also orthogonal to this CL.

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:2716: // Determine whether we can still do the inner
function lazy parsing.
On 2016/09/23 11:51:18, Toon Verwaest wrote:
> Determine whether we can still lazy parse the inner function ?

Done.

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:2732: // Inner functions will be parsed by using a
temporary Zone. After parsing, we
On 2016/09/23 11:51:18, Toon Verwaest wrote:
> parsed using

Done.

[adamk, 2016-09-23 18:21:30]

The only big comment I would've made is on the same section that Toon is asking
for a rewrite of, so I'm happy to re-review once that's reorganized.

https://codereview.chromium.org/2352593002/diff/240001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2352593002/diff/240001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1532: !var->has_forced_context_allocation() &&
!var->is_arguments()) {
I think is_arguments() is now dead.

What's the is_this() about?

https://codereview.chromium.org/2352593002/diff/300001/test/mjsunit/debug-fun...
File test/mjsunit/debug-function-scopes.js (left):

https://codereview.chromium.org/2352593002/diff/300001/test/mjsunit/debug-fun...
test/mjsunit/debug-function-scopes.js:90: var invisible2 = 3;
I take it this now becomes visible?

[marja, 2016-09-26 07:12:42]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 07:12:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 07:14:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 07:14:24]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[marja, 2016-09-26 07:14:41]

thanks for comments!

https://codereview.chromium.org/2352593002/diff/240001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2352593002/diff/240001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1532: !var->has_forced_context_allocation() &&
!var->is_arguments()) {
On 2016/09/23 18:21:30, adamk wrote:
> I think is_arguments() is now dead.
> 
> What's the is_this() about?

Here I just wanted to be less pessimistic and whitelist some identifier-y things
which are not references to the outer function for sure.

Removed is_arguments and added a comment here: since we don't lazy parse inner
arrow functions, inner functions cannot refer to the outer "this".

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:2820: if (is_lazy_top_level_function) {
On 2016/09/23 11:51:18, Toon Verwaest wrote:
> What about just merging this with the is_lazy_inner_function below, only
> allowing aborting if is_lazy_top_level_function? Basically:
> 
> if (is_lazy_function) {
>   bookmark scope...
>    result = SkipLazy...( , is_lazy_top_level_function, ...) ;
>    ...
>    if (result == kLazyParsingAborted) {
>      DCHECK(is_lazy_top_level_function);
>      is_lazy_function = false;
>      ...
>     } else if (is_lazy_inner_function) {
>       // By now this can be moved here since we just need to fetch all
> references. Even if we'd do scope resolution it can be done here since I've
> already modified DeclareArguments to happen within function parsing rather
than
> down below.
>       scope->AnalyzePartially(main_scope, &previous_zone_ast_node_factory); 
>     }
> }
> if (!is_lazy_function) {
>   DCHECK_EQ(scope, main_scope);
>   ...eager parse...
> }

Done - but in the new version is much more difficult to see that exactly on
branch gets executed.

(This is because of the lazy parsing aborting, so, the structure cannot be just
if - if else - if else - else, but it needs to be more complicated.)

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:2898: if (is_lazy_inner_function) {
On 2016/09/23 11:51:18, Toon Verwaest wrote:
> This can be dropped if you handle it as described above.

Done.

https://codereview.chromium.org/2352593002/diff/240001/src/parsing/parser.cc#...
src/parsing/parser.cc:3521: // If we don't need to look at the scope, construct
a dummy scope chain
On 2016/09/23 11:51:18, Toon Verwaest wrote:
> I don't see why this is necessary now but wasn't before? Nothing changed for
> top-level parsing?
> 
> If this distinction isn't necessary, it seems like we can drop
is_inner_function
> as a parameter entirely and keep this function untouched.

This code was moved from PreParser::PreParseLazyFunction and I'd like to keep
the code w/o the flag as close to master as possible...

[marja, 2016-09-26 07:18:33]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 07:18:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2016-09-26 07:21:35]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 07:21:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2016-09-26 07:21:49]

Patchset #18 (id:340001) has been deleted

[commit-bot: I haz the power, 2016-09-26 07:24:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 07:24:33]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[marja, 2016-09-26 08:31:10]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 08:31:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Toon Verwaest, 2016-09-26 11:58:54]

lgtm. There's still a lot of cleanup to be done after this, but let's do it
incrementally ...

[marja, 2016-09-26 12:00:42]

Thanks for review!

(As discussed w/ adamk@, I'm landing this now that verwaest@ has reviewed.)

[marja, 2016-09-26 12:00:52]

The CQ bit was checked by marja@chromium.org

[marja, 2016-09-26 12:00:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2352593002/#ps420001
(title: "add comment")

[commit-bot: I haz the power, 2016-09-26 12:00:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 12:36:20]

Description was changed from

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

to

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

[commit-bot: I haz the power, 2016-09-26 12:36:21]

Committed patchset #21 (id:420001)

[commit-bot: I haz the power, 2016-09-26 12:36:40]

Description was changed from

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...




BUG=
==========

to

==========
Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiB...

BUG=

Committed: https://crrev.com/7c73cf32c60484cdf37c84f1d61b4640e87068d7
Cr-Commit-Position: refs/heads/master@{#39719}
==========

[commit-bot: I haz the power, 2016-09-26 12:36:41]

Patchset 21 (id:??) landed as
https://crrev.com/7c73cf32c60484cdf37c84f1d61b4640e87068d7
Cr-Commit-Position: refs/heads/master@{#39719}

[Michael Hablich, 2016-09-26 13:53:38]

A revert of this CL (patchset #21 id:420001) has been created in
https://codereview.chromium.org/2373443003/ by hablich@chromium.org.

The reason for reverting is: We currently have some stability issues on Canary.
Let's reland this after we verified that we "fixed" Canary again..