Preparse inner functions.

Preparse inner functions.

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.

Committed: https://crrev.com/e1341ca8fa486bb2c9e4236672a64ec7756a164d
Cr-Commit-Position: refs/heads/master@{#39469}

[marja, 2016-09-14 12:28:40]

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

[commit-bot: I haz the power, 2016-09-14 12:28:52]

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-14 12:38:06]

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

[commit-bot: I haz the power, 2016-09-14 12:38:10]

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-14 12:40:59]

Description was changed from

==========
Proof of concept: PreParser declaring variables.

(WIP - not ready for review)

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

to

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

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

[marja, 2016-09-14 12:41:25]

Description was changed from

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

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

to

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

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

[commit-bot: I haz the power, 2016-09-14 12:52:00]

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

[commit-bot: I haz the power, 2016-09-14 12:52:01]

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_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[marja, 2016-09-14 18:34:24]

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

[commit-bot: I haz the power, 2016-09-14 18:34:26]

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-14 18:49:06]

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

[commit-bot: I haz the power, 2016-09-14 18:49:08]

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_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[marja, 2016-09-15 09:40:26]

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

[marja, 2016-09-15 09:41:04]

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

[commit-bot: I haz the power, 2016-09-15 09:41:13]

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-15 11:03:36]

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

[commit-bot: I haz the power, 2016-09-15 11:03:44]

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-15 11:34:28]

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

[commit-bot: I haz the power, 2016-09-15 11:34:28]

Dry run: This issue passed the CQ dry run.

[marja, 2016-09-15 11:37:29]

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

[marja, 2016-09-15 11:37:30]

ptal

Note: I didn't yet refactor parsing modes but added TODOs
instead. This is because I want to be able to revert back to the old behavior by
just turning the flag off. If I also refactor the conditions on when exactly to
lazy parse / use temp zone, that is irrevokable (by the flag). The conditions
are quite subtle and I already got them wrong while developing...

Results: This doesn't seem to be exercised by our benchmarks all that well, but
I tested by wrapping a big js file (from
https://bugs.chromium.org/p/chromium/issues/detail?id=417697 )
inside (function() { ... }) (note: not executed, but triggers eager
compilation!). Run time master: 3.8 s, run time mine: 3.2 s.

[marja, 2016-09-15 11:38:41]

marja@chromium.org changed reviewers:
+ nikolaos@chromium.org

[marja, 2016-09-15 11:38:42]

+nikolaos too, in case you'd like to have a look too

[marja, 2016-09-15 11:39:10]

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

[commit-bot: I haz the power, 2016-09-15 11:39:18]

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-15 11:40:30]

And some roadmap into the patch sets:

- Try jobs are in patch set 19 (which is the same as patch set 21)
- Patch set 20 turns the flag off, just for running try jobs for that combo too.

I plan to land this with the flag *on*.

[adamk, 2016-09-15 18:20:09]

lgtm % a nit and a question

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser....
src/parsing/preparser.cc:97: // scope_state_. All scopes above the
function_scoepe are ignored by the
Typo: function_scoepe

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser....
src/parsing/preparser.cc:563: factory.set_zone(zone());
Why is this necessary? AstNodeFactory's constructor looks like it takes the zone
from ast_value_factory().

[nednguyen, 2016-09-15 19:57:38]

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

[nednguyen, 2016-09-15 19:58:10]

On 2016/09/15 18:20:09, adamk wrote:
> lgtm % a nit and a question
> 
>
https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser.cc
> File src/parsing/preparser.cc (right):
> 
>
https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser....
> src/parsing/preparser.cc:97: // scope_state_. All scopes above the
> function_scoepe are ignored by the
> Typo: function_scoepe
> 
>
https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser....
> src/parsing/preparser.cc:563: factory.set_zone(zone());
> Why is this necessary? AstNodeFactory's constructor looks like it takes the
zone
> from ast_value_factory().

I am busy with perf at the moment so will get back to this next week. Please
ping me if you don't see any replies by then

[vogelheim, 2016-09-16 08:13:05]

lgtm w/ some nitpicks...

I'm a bit confused about the parenthesized function thing; please have another
look at that.

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

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.cc#...
src/parsing/parser.cc:2981:
scope()->AllowsLazyParsingWithoutUnresolvedVariables();
Does this eliminate the force-eager-compile-for-parenthesized-functions things?

If so, we need to update the comments. If not, I don't understand the code. :)

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.cc#...
src/parsing/parser.cc:3088: if (is_lazy_top_level_function) {
For my understanding..

this: DCHECK(is_lazy_top_level_function != is_lazy_inner_function);
or this: DCHECK(!(is_lazy_top_level_function && is_lazy_inner_function));
?

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.cc#...
src/parsing/parser.cc:3127: } else if (!is_lazy_top_level_function) {
style nitpick: This code was awkward before, since there's several blocks of
almost doing the same thin. Maybe we can merge this and the previous if-block,
since the only difference is the assignment to body. Something like:

f (is_lazy_inner_function && FLAG_laze_...) {
  lazy parse.
} else {
  eager parser. Throw away body if is_lazy_inner_function
}

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.h#n...
src/parsing/parser.h:1053: friend class DiscardableZoneScope;
Why?

[marja, 2016-09-16 08:38:16]

thanks for review!

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

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.cc#...
src/parsing/parser.cc:2981:
scope()->AllowsLazyParsingWithoutUnresolvedVariables();
On 2016/09/16 08:13:04, vogelheim wrote:
> Does this eliminate the force-eager-compile-for-parenthesized-functions
things?
> 
> If so, we need to update the comments. If not, I don't understand the code. :)

As discussed offline, the logic is intact (eager_compile_hint above).

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.cc#...
src/parsing/parser.cc:3088: if (is_lazy_top_level_function) {
On 2016/09/16 08:13:04, vogelheim wrote:
> For my understanding..
> 
> this: DCHECK(is_lazy_top_level_function != is_lazy_inner_function);
> or this: DCHECK(!(is_lazy_top_level_function && is_lazy_inner_function));
> ?

The latter will work. It's also possible both are false (e.g., when we eagerly
compile). As discussed offline, I didn't add the DCHECK. In addition, I'll
refactor these once the flag is gone; now I wanted to be extra sure that when
the flag is off, the same-thing-than-before happens, so I couldn't modify these
conditions.

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.cc#...
src/parsing/parser.cc:3127: } else if (!is_lazy_top_level_function) {
On 2016/09/16 08:13:04, vogelheim wrote:
> style nitpick: This code was awkward before, since there's several blocks of
> almost doing the same thin. Maybe we can merge this and the previous if-block,
> since the only difference is the assignment to body. Something like:
> 
> f (is_lazy_inner_function && FLAG_laze_...) {
>   lazy parse.
> } else {
>   eager parser. Throw away body if is_lazy_inner_function
> }

Will be changed after the flag is gone.

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/parser.h#n...
src/parsing/parser.h:1053: friend class DiscardableZoneScope;
On 2016/09/16 08:13:04, vogelheim wrote:
> Why?

Because of reusable_preparser_

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser....
src/parsing/preparser.cc:97: // scope_state_. All scopes above the
function_scoepe are ignored by the
On 2016/09/15 18:20:09, adamk wrote:
> Typo: function_scoepe

Done.

https://codereview.chromium.org/2322243002/diff/400001/src/parsing/preparser....
src/parsing/preparser.cc:563: factory.set_zone(zone());
On 2016/09/15 18:20:08, adamk wrote:
> Why is this necessary? AstNodeFactory's constructor looks like it takes the
zone
> from ast_value_factory().

Added a comment:
// Setting the Zone is necessary because zone_ might be the temp Zone, and
// AstValueFactory doesn't know about it.

[marja, 2016-09-16 09:26:00]

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

[commit-bot: I haz the power, 2016-09-16 09:26:09]

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

[nickie, 2016-09-16 09:47:49]

It looks OK but I'm not to be trusted on this, as I don't think I understand
well these parts.

[commit-bot: I haz the power, 2016-09-16 09:57:58]

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

[commit-bot: I haz the power, 2016-09-16 09:57:59]

Dry run: This issue passed the CQ dry run.

[marja, 2016-09-16 09:59:33]

The CQ bit was checked by marja@chromium.org

[marja, 2016-09-16 09:59:34]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2322243002/#ps440001
(title: "rebased")

[commit-bot: I haz the power, 2016-09-16 09:59:39]

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-16 10:01:51]

Description was changed from

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

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

to

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

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

[commit-bot: I haz the power, 2016-09-16 10:01:53]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-09-16 10:02:42]

Description was changed from

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

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

to

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

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.

Committed: https://crrev.com/e1341ca8fa486bb2c9e4236672a64ec7756a164d
Cr-Commit-Position: refs/heads/master@{#39469}
==========

[commit-bot: I haz the power, 2016-09-16 10:02:43]

Patchset 23 (id:??) landed as
https://crrev.com/e1341ca8fa486bb2c9e4236672a64ec7756a164d
Cr-Commit-Position: refs/heads/master@{#39469}

[marja, 2016-09-16 10:43:08]

A revert of this CL (patchset #23 id:440001) has been created in
https://codereview.chromium.org/2349473004/ by marja@chromium.org.

The reason for reverting is: This approach is not good - breaks when we
recompile..