Always immediately propagate flags outwards rather than relying on PropagateScopeInfo

Always immediately propagate flags outwards rather than relying on PropagateScopeInfo

- Now "inner_scope_uses_eval_" is also set of scopes that call eval themselves.
- AllowLazyParsing doesn't check force_eager_compilation_ anymore.
- Both inner_scope_uses_eval_ and force_eager_compilation_ are propagated
  outwards immediately when set.

BUG=v8:5209
Committed: https://crrev.com/6ed87bfbc22f67a4054f9cd767b335eaaddc16a1
Cr-Commit-Position: refs/heads/master@{#38797}

[Toon Verwaest, 2016-08-22 12:49:08]

verwaest@chromium.org changed reviewers:
+ neis@chromium.org

[Toon Verwaest, 2016-08-22 12:49:08]

ptal

[neis, 2016-08-22 13:21:47]

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

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:884: }
Can you motivate this change? At least superficially it makes sense that one is
only allowed to parse lazily if one is allowed to compile lazily.

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h#newcod...
src/ast/scopes.h:232: 
Do I understand correctly that you are setting inner_scope_calls_eval_ for the
original scope too in order to shorten some conditionals elsewhere?

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h#newcod...
src/ast/scopes.h:837: // Make this closure and all outer closures are eagerly
compiled.
There's a word missing here.

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h#newcod...
src/ast/scopes.h:838: void ForceEagerCompilation() {
Can you add something like DCHECK(IsClosureScope) here? Or move the
GetClosureScope into the beginning of the loop.

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h#newcod...
src/ast/scopes.h:841: force_eager_compilation_ = true;
s->force_eager_compilation_

Should this have caused test failures?

[Toon Verwaest, 2016-08-22 14:13:29]

Addressed comments.

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

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:884: }
On 2016/08/22 13:21:47, neis wrote:
> Can you motivate this change? At least superficially it makes sense that one
is
> only allowed to parse lazily if one is allowed to compile lazily.

It's not calling AllowsLazyCompilation on the function it's going to parse, but
the outer function. So that call doesn't make sense :)

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h#newcod...
src/ast/scopes.h:232: 
On 2016/08/22 13:21:47, neis wrote:
> Do I understand correctly that you are setting inner_scope_calls_eval_ for the
> original scope too in order to shorten some conditionals elsewhere?

Yes. All uses of inner_scope_calls_eval_ always did inner_scope_calls_eval_ ||
scope_calls_eval_

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h#newcod...
src/ast/scopes.h:837: // Make this closure and all outer closures are eagerly
compiled.
On 2016/08/22 13:21:47, neis wrote:
> There's a word missing here.

Done.

https://codereview.chromium.org/2269603002/diff/20001/src/ast/scopes.h#newcod...
src/ast/scopes.h:838: void ForceEagerCompilation() {
On 2016/08/22 13:21:47, neis wrote:
> Can you add something like DCHECK(IsClosureScope) here? Or move the
> GetClosureScope into the beginning of the loop.

Done.

[neis, 2016-08-22 14:20:54]

lgtm

[Toon Verwaest, 2016-08-22 14:24:13]

verwaest@chromium.org changed reviewers:
+ vogelheim@chromium.org

[Toon Verwaest, 2016-08-22 14:24:13]

vogelheim: ptal parser

[vogelheim, 2016-08-22 16:16:30]

lgtm.

parser/ looks fine; but one nitpick on scopes. :)

https://codereview.chromium.org/2269603002/diff/40001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2269603002/diff/40001/src/ast/scopes.h#newcod...
src/ast/scopes.h:501: bool scope_calls_eval_ : 1;
Is scope_calls_eval_ still used?

If so: Does that still work, since RecordCallEval no longer sets it?
If not: delete?

[Toon Verwaest, 2016-08-22 16:42:22]

RecordCallEval does still set it, and it is still used independently of
inner_scope_calls_eval. If inner_scope_calls_eval, you need to context-allocate
everything and mark all variables as "maybe assigned". If scope-calls-eval, you
may need to check the context-extension.

[Toon Verwaest, 2016-08-22 16:44:18]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-22 16:44:18]

The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2269603002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-22 16:44:22]

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-08-22 17:18:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-22 17:19:10]

Description was changed from

==========
Always immediately propagate flags outwards rather than relying on
PropagateScopeInfo

- Now "inner_scope_uses_eval_" is also set of scopes that call eval themselves.
- AllowLazyParsing doesn't check force_eager_compilation_ anymore.
- Both inner_scope_uses_eval_ and force_eager_compilation_ are propagated
  outwards immediately when set.

BUG=v8:5209
==========

to

==========
Always immediately propagate flags outwards rather than relying on
PropagateScopeInfo

- Now "inner_scope_uses_eval_" is also set of scopes that call eval themselves.
- AllowLazyParsing doesn't check force_eager_compilation_ anymore.
- Both inner_scope_uses_eval_ and force_eager_compilation_ are propagated
  outwards immediately when set.

BUG=v8:5209

Committed: https://crrev.com/6ed87bfbc22f67a4054f9cd767b335eaaddc16a1
Cr-Commit-Position: refs/heads/master@{#38797}
==========

[commit-bot: I haz the power, 2016-08-22 17:19:11]

Patchset 4 (id:??) landed as
https://crrev.com/6ed87bfbc22f67a4054f9cd767b335eaaddc16a1
Cr-Commit-Position: refs/heads/master@{#38797}

[adamk, 2016-08-22 19:12:07]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2016-08-22 19:12:08]

https://codereview.chromium.org/2269603002/diff/60001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2269603002/diff/60001/src/ast/scopes.h#newcod...
src/ast/scopes.h:226: void RecordEvalCall() {
Note that in sloppy mode we make two calls to RecordEvalCall in the parser, so
this will now walk the scope chain twice. I guess there's no particular harm to
this, but something to keep an eye out for.

[Toon Verwaest, 2016-08-22 19:24:22]

Good to know. If it becomes an issue we should just check if any scope along the
way already has the relevant flag set. For now I assumed that could be slower
than just setting. I assumed it doesn't matter that much either way. Better
paying the slightly higher price when using the feature than always paying a
little.