Allow lazy parsing of functions nested in eager compiled functions

Allow lazy parsing of functions nested in eager compiled functions

This is blocked on https://bugs.chromium.org/p/v8/issues/detail?id=5484

BUG=v8:5501
Committed: https://crrev.com/14bf19af64314a559d23bb54f2d942e9227a1fe8
Cr-Commit-Position: refs/heads/master@{#40167}

[Toon Verwaest, 2016-10-10 11:50:01]

Description was changed from

==========
Allow lazy parsing of functions nested in eager compiled functions

BUG=v8:5501
==========

to

==========
Allow lazy parsing of functions nested in eager compiled functions

This is blocked on https://bugs.chromium.org/p/v8/issues/detail?id=5484

BUG=v8:5501
==========

[Toon Verwaest, 2016-10-10 11:50:02]

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

[Toon Verwaest, 2016-10-10 11:50:38]

ptal

[marja, 2016-10-10 11:53:50]

this lgtm (once unblocked)

[Toon Verwaest, 2016-10-10 11:54:28]

verwaest@chromium.org changed reviewers:
+ jochen@chromium.org
- eisinger@chromium.org

[jochen (gone - plz use gerrit), 2016-10-10 12:33:25]

https://codereview.chromium.org/2405813002/diff/1/src/parsing/parse-info.cc
File src/parsing/parse-info.cc (right):

https://codereview.chromium.org/2405813002/diff/1/src/parsing/parse-info.cc#n...
src/parsing/parse-info.cc:42: set_allow_lazy_parsing(true);
what's the difference between set_lazy and set_allow_lazy_parsing given that the
struct is called ParseInfo?

[Toon Verwaest, 2016-10-10 14:15:18]

ptal again. Now I put the diff between the old and new code behind the
FLAG_lazy_inner_functions. That way we can already land it and build on top of
this.

[Toon Verwaest, 2016-10-10 14:18:52]

https://codereview.chromium.org/2405813002/diff/1/src/parsing/parse-info.cc
File src/parsing/parse-info.cc (right):

https://codereview.chromium.org/2405813002/diff/1/src/parsing/parse-info.cc#n...
src/parsing/parse-info.cc:42: set_allow_lazy_parsing(true);
On 2016/10/10 12:33:25, jochen wrote:
> what's the difference between set_lazy and set_allow_lazy_parsing given that
the
> struct is called ParseInfo?

is_lazy is a misnomer. It should be called "eager_parse_function". It's when you
have a Handle<JSFunction> and you want to parse/compile it; as opposed to a
script.

[marja, 2016-10-11 07:56:47]

I'm not sure if this is correct...

https://codereview.chromium.org/2405813002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2405813002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3928: // handling in Scope::ResolveVariable needs to
change.
So now you're lazy-parsing inner arrow functions, right? This comment is now out
of date, plus the is_this fix is needed (something should fail if you omit that,
right? - maybe we're missing a test...)

https://codereview.chromium.org/2405813002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3929: bool is_lazy_top_level_function =
Hmm, this variable name sounds like you're *not* parsing the inner arrow
functions.

But as mode() == PARSE_LAZILY can now be true inside a function too, I think you
are!

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

https://codereview.chromium.org/2405813002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2653: (FLAG_lazy_inner_functions
You're dropping the function_type == FunctionLiteral::kDeclaration condition
here. Why?

What's the invariant - is this supposed to keep the current behavior whenever
!FLAG_lazy_inner_functions?

[marja, 2016-10-11 07:58:28]

Oh, also, what does your regression example (pinterest?) say about this CL with
and without the flag?

[Toon Verwaest, 2016-10-11 10:44:00]

https://codereview.chromium.org/2405813002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2405813002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3928: // handling in Scope::ResolveVariable needs to
change.
On 2016/10/11 07:56:47, marja wrote:
> So now you're lazy-parsing inner arrow functions, right? This comment is now
out
> of date, plus the is_this fix is needed (something should fail if you omit
that,
> right? - maybe we're missing a test...)

Nothing changed here. Previously we'd have PARSE_LAZILY only on the top-level
(not in a nested function), and AllowLazyParsingWithoutUnresolvedVariables()
just looked for block scopes. Now we can have PARSE_LAZILY in inner functions,
which means we also need to blacklist function-scopes.
AllowLazyParsingWithoutUnresolvedVariables now does that. However, that means we
couldn't start at formal_parameters.scope which == the function scope generated
for the arrow function. That's why I had to move this code up, so it skips the
function scope.

https://codereview.chromium.org/2405813002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:3929: bool is_lazy_top_level_function =
On 2016/10/11 07:56:47, marja wrote:
> Hmm, this variable name sounds like you're *not* parsing the inner arrow
> functions.
> 
> But as mode() == PARSE_LAZILY can now be true inside a function too, I think
you
> are!

This variable means exactly the same as in ParseFunctionLiteral. I guess you
were confused because of the previous question?

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

https://codereview.chromium.org/2405813002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2653: (FLAG_lazy_inner_functions
On 2016/10/11 07:56:47, marja wrote:
> You're dropping the function_type == FunctionLiteral::kDeclaration condition
> here. Why?
> 
> What's the invariant - is this supposed to keep the current behavior whenever
> !FLAG_lazy_inner_functions?

I don't see the reason why FunctionLiteral::kDeclaration was there in the first
place mostly. The invariant is that we shouldn't temp-zone parse if allow_lazy()
is set to false; which is currently true when we "ParseLazy" (parse a function).
I'm ok with putting it back, but I don't think it's necessary.

[marja, 2016-10-11 10:52:11]

lgtm

[marja, 2016-10-11 10:52:42]

(I was confused about the function scopes, now we indeed don't rely on mode()
any more but AllowsLazyCompilationWithoutContext.)

[Toon Verwaest, 2016-10-11 11:40:36]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-10-11 11:40:37]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2405813002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-11 11:40:44]

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-10-11 12:13:43]

Description was changed from

==========
Allow lazy parsing of functions nested in eager compiled functions

This is blocked on https://bugs.chromium.org/p/v8/issues/detail?id=5484

BUG=v8:5501
==========

to

==========
Allow lazy parsing of functions nested in eager compiled functions

This is blocked on https://bugs.chromium.org/p/v8/issues/detail?id=5484

BUG=v8:5501
==========

[commit-bot: I haz the power, 2016-10-11 12:13:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-11 12:14:00]

Description was changed from

==========
Allow lazy parsing of functions nested in eager compiled functions

This is blocked on https://bugs.chromium.org/p/v8/issues/detail?id=5484

BUG=v8:5501
==========

to

==========
Allow lazy parsing of functions nested in eager compiled functions

This is blocked on https://bugs.chromium.org/p/v8/issues/detail?id=5484

BUG=v8:5501

Committed: https://crrev.com/14bf19af64314a559d23bb54f2d942e9227a1fe8
Cr-Commit-Position: refs/heads/master@{#40167}
==========

[commit-bot: I haz the power, 2016-10-11 12:14:01]

Patchset 3 (id:??) landed as
https://crrev.com/14bf19af64314a559d23bb54f2d942e9227a1fe8
Cr-Commit-Position: refs/heads/master@{#40167}