Teach Scopes whether they will end up being lazily compiled or not

Teach Scopes whether they will end up being lazily compiled or not

For now keep the logic in compiler.cc and add a DCHECK that the scopes
and compiler.cc agree.

Use this knowledge to only created ScopeInfos for literals we'll
actually compile.

BUG=v8:5394, v8:5422
R=marja@chromium.org,verwaest@chromium.org

Committed: https://crrev.com/940efafd3d9989ca6baedb232ace979bef699d7b
Cr-Commit-Position: refs/heads/master@{#40074}

[jochen (gone - plz use gerrit), 2016-10-06 11:30:15]

https://codereview.chromium.org/2399833002/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (left):

https://codereview.chromium.org/2399833002/diff/1/src/parsing/parser-base.h#o...
src/parsing/parser-base.h:232: // clang-format off
we shouldn't randomly disable clang-format, but stick to the coding style

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

https://codereview.chromium.org/2399833002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:4710: //   [lookahead notin {{, function, class, let
[}] Expression[In, ?Yield] ;
clang-format.py can't deal with this character..

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

generally lg

https://codereview.chromium.org/2399833002/diff/1/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2399833002/diff/1/src/ast/ast.cc#newcode292
src/ast/ast.cc:292: scope()->SetShouldCompileLazily(false);
FunctionLiteral::set_should_eagerly_compile but Scope::SetShouldCompileLazily?

Would it be possible to have both the same way around?

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

https://codereview.chromium.org/2399833002/diff/1/src/ast/scopes.cc#newcode1924
src/ast/scopes.cc:1924: isolate, scope->is_function_scope() ?
AnalyzeMode::kRegular : mode,
Why is dropping to kRegular correct here?

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

https://codereview.chromium.org/2399833002/diff/1/src/ast/scopes.h#newcode426
src/ast/scopes.h:426: // as lazy.
Why "except for inner function scopes"?

If we compile a function lazily, don't we always compile its inner functions
lazily too?

(Also, in the comment, it's not clear what "lazy" means, I think we should be
clear with the terminology... it can mean either "lazily parsed" or "lazily
compiled" or "something that was previously lazily parsed" or "something we
happen to reparse for some other reason".)

https://codereview.chromium.org/2399833002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2399833002/diff/1/src/compiler.cc#newcode1434
src/compiler.cc:1434: info->parse_info()->set_will_serialize(false);
Why is this needed, shouldn't there be a default that we're not going to
serialize?

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

https://codereview.chromium.org/2399833002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1463: bool serializer_enabled_;
Seems unnecessary to need these bools in ParserBase.

They don't change, and they only affect the laziness decision, so I think it
would be enough to store one boolean which is set in the ctor.

Actually, we already have allow_lazy so why not use that? allow_lazy is already
a combination of various things which might disable laziness, and these are just
more of those things.

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

https://codereview.chromium.org/2399833002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:2816: scope->SetShouldCompileLazily(true);
Would it be feasible to pass this information in FunctionLiteral ctor (which
would then call scope->SetShouldCompileLazily) so that we don't need to
duplicate this in all places where FunctionLiterals are created?

[jochen (gone - plz use gerrit), 2016-10-06 15:04:47]

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

[commit-bot: I haz the power, 2016-10-06 15:05:03]

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

[jochen (gone - plz use gerrit), 2016-10-06 15:05:04]

ptal

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

https://codereview.chromium.org/2399833002/diff/1/src/ast/scopes.cc#newcode1924
src/ast/scopes.cc:1924: isolate, scope->is_function_scope() ?
AnalyzeMode::kRegular : mode,
On 2016/10/06 at 11:53:06, marja wrote:
> Why is dropping to kRegular correct here?

because the scope iterator that uses the AnalyzeMode::kDebugger only looks at
the current function and all its inner non-function scopes:
https://cs.chromium.org/chromium/src/v8/src/debug/debug-scopes.cc?rcl=0&l=816

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

https://codereview.chromium.org/2399833002/diff/1/src/ast/scopes.h#newcode426
src/ast/scopes.h:426: // as lazy.
On 2016/10/06 at 11:53:06, marja wrote:
> Why "except for inner function scopes"?
> 
> If we compile a function lazily, don't we always compile its inner functions
lazily too?
> 
> (Also, in the comment, it's not clear what "lazy" means, I think we should be
clear with the terminology... it can mean either "lazily parsed" or "lazily
compiled" or "something that was previously lazily parsed" or "something we
happen to reparse for some other reason".)

yeah, but not the other way round... if we compile it eagerly, we don't
necessarily compile inner functions lazily (but all inner scopes of the current
function)

https://codereview.chromium.org/2399833002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2399833002/diff/1/src/compiler.cc#newcode1434
src/compiler.cc:1434: info->parse_info()->set_will_serialize(false);
On 2016/10/06 at 11:53:06, marja wrote:
> Why is this needed, shouldn't there be a default that we're not going to
serialize?

In EnsureDeoptimizationSupport we might recompile the unoptimized code. For
this, we might have to enable the serializer.

The CompilationInfo "unoptimized", however, uses the same ParseInfo as the
optimized job will use, so I have to reset that flag now that it lives on
ParseInfo and no longer on CompilationInfo.

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

https://codereview.chromium.org/2399833002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1463: bool serializer_enabled_;
On 2016/10/06 at 11:53:06, marja wrote:
> Seems unnecessary to need these bools in ParserBase.
> 
> They don't change, and they only affect the laziness decision, so I think it
would be enough to store one boolean which is set in the ctor.
> 
> Actually, we already have allow_lazy so why not use that? allow_lazy is
already a combination of various things which might disable laziness, and these
are just more of those things.

allow_lazy is for native scripts for example - we need to parse them to find all
native functions. However, we still can compile lazily. Anyways, moving those
variables to parser works

[commit-bot: I haz the power, 2016-10-06 15:09:49]

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

[commit-bot: I haz the power, 2016-10-06 15:09:50]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10290)

[jochen (gone - plz use gerrit), 2016-10-06 15:19:08]

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

[commit-bot: I haz the power, 2016-10-06 15:19:19]

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-10-06 15:23:47]

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

[commit-bot: I haz the power, 2016-10-06 15:23:48]

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

[jochen (gone - plz use gerrit), 2016-10-06 15:36:02]

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

[commit-bot: I haz the power, 2016-10-06 15:36:11]

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-10-06 16:04:26]

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

[commit-bot: I haz the power, 2016-10-06 16:04:26]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-06 16:36:08]

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

[commit-bot: I haz the power, 2016-10-06 16:36:10]

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-10-06 17:08:15]

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

[commit-bot: I haz the power, 2016-10-06 17:08:16]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-06 17:17:25]

perf results
(https://storage.cloud.google.com/chrome-v8/perf-try/2399833002/80001/v8_linux...)
are kinda inconclusive (which is what I expected - this change should be more or
less a noop, except that we allocate less scope infos)

[jochen (gone - plz use gerrit), 2016-10-06 17:18:26]

e.g. splay and splay latency go up a bit in octane, and down a bit in jetstream,
and vice versa for mandreel and mandreel latency

[marja, 2016-10-07 08:25:59]

https://codereview.chromium.org/2399833002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2399833002/diff/1/src/compiler.cc#newcode1434
src/compiler.cc:1434: info->parse_info()->set_will_serialize(false);
Offline discussion: this code for resetting the flag is actually conceptually
close to the place where the flag will potentially be set, but it's hard to see
since the setting is potentially done in EnableDeoptimizationSupport.

Maybe add a comment?

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

https://codereview.chromium.org/2399833002/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1929: // scope, we can go back to the regular analysis.
Strictly speaking, this comment explains "what" but it doesn't explain "why".

How about:

"The ScopeIterator which uses the AnalyzeMode::kDebugger only expects to find
ScopeInfos for the current function and all its inner non-function scopes (see
ScopeIterator::GetNestedScopeChain)."

https://codereview.chromium.org/2399833002/diff/80001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2399833002/diff/80001/src/compiler.cc#newcode...
src/compiler.cc:1434: info->parse_info()->set_will_serialize(false);
Comment suggestion:

We reuse the same ParseInfo in the future, and might have set_will_serialize in
EnableDeoptimizationSupport, so reset it here.

https://codereview.chromium.org/2399833002/diff/80001/src/compiler.cc#newcode...
src/compiler.cc:1794: if (literal->ShouldEagerCompile() &&
literal->should_be_used_once_hint()) {
Wait, isn't this always true now, since we are in the "else" branch of "if
(!literal->ShouldEagerCompile())"?

Also I think here you change the behavior and I cannot tell if it's correct or
not.

So previously we would only MarkToBeExecutedOnce if the reason for eagerly
compiling was literal->should_eager_compile(), so, it was not enough that !lazy.

But now you're collapsing both kind of conditions into one, so you cannot here
differentiate between different reasons for eagerly compiling.

But if it works, fair enough. :) It might be that the previous code was overly
cautious about not calling MarkToBeExecutedOnce unnecessarily.

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

https://codereview.chromium.org/2399833002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2805: 
Do you feel strongly about adding this line? :)

(I bother to comment since I had to resolve a conflict against such a change
recently.)

[marja, 2016-10-07 08:27:48]

(lgtm % previous comments)

[jochen (gone - plz use gerrit), 2016-10-07 08:34:35]

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

[jochen (gone - plz use gerrit), 2016-10-07 08:34:37]

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

https://codereview.chromium.org/2399833002/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1929: // scope, we can go back to the regular analysis.
On 2016/10/07 at 08:25:59, marja wrote:
> Strictly speaking, this comment explains "what" but it doesn't explain "why".
> 
> How about:
> 
> "The ScopeIterator which uses the AnalyzeMode::kDebugger only expects to find
ScopeInfos for the current function and all its inner non-function scopes (see
ScopeIterator::GetNestedScopeChain)."

done

https://codereview.chromium.org/2399833002/diff/80001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2399833002/diff/80001/src/compiler.cc#newcode...
src/compiler.cc:1434: info->parse_info()->set_will_serialize(false);
On 2016/10/07 at 08:25:59, marja wrote:
> Comment suggestion:
> 
> We reuse the same ParseInfo in the future, and might have set_will_serialize
in EnableDeoptimizationSupport, so reset it here.

I moved the code it but further up and added a comment

https://codereview.chromium.org/2399833002/diff/80001/src/compiler.cc#newcode...
src/compiler.cc:1794: if (literal->ShouldEagerCompile() &&
literal->should_be_used_once_hint()) {
On 2016/10/07 at 08:25:59, marja wrote:
> Wait, isn't this always true now, since we are in the "else" branch of "if
(!literal->ShouldEagerCompile())"?
> 
> Also I think here you change the behavior and I cannot tell if it's correct or
not.
> 
> So previously we would only MarkToBeExecutedOnce if the reason for eagerly
compiling was literal->should_eager_compile(), so, it was not enough that !lazy.
> 
> But now you're collapsing both kind of conditions into one, so you cannot here
differentiate between different reasons for eagerly compiling.
> 
> But if it works, fair enough. :) It might be that the previous code was overly
cautious about not calling MarkToBeExecutedOnce unnecessarily.

I removed the ShouldEagerCompilePart()

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

https://codereview.chromium.org/2399833002/diff/80001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2805: 
On 2016/10/07 at 08:25:59, marja wrote:
> Do you feel strongly about adding this line? :)
> 
> (I bother to comment since I had to resolve a conflict against such a change
recently.)

haha.

[commit-bot: I haz the power, 2016-10-07 08:34:40]

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-10-07 08:36:09]

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

[commit-bot: I haz the power, 2016-10-07 08:36:10]

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

[jochen (gone - plz use gerrit), 2016-10-07 08:41:10]

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

[commit-bot: I haz the power, 2016-10-07 08:41:21]

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

[marja, 2016-10-07 08:46:01]

still lgtm

[jochen (gone - plz use gerrit), 2016-10-07 08:46:26]

The CQ bit was unchecked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-10-07 08:46:28]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2016-10-07 08:46:40]

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-07 09:12:52]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-07 09:13:12]

Description was changed from

==========
Teach Scopes whether they will end up being lazily compiled or not

For now keep the logic in compiler.cc and add a DCHECK that the scopes
and compiler.cc agree.

Use this knowledge to only created ScopeInfos for literals we'll
actually compile.

BUG=v8:5394,v8:5422
R=marja@chromium.org,verwaest@chromium.org
==========

to

==========
Teach Scopes whether they will end up being lazily compiled or not

For now keep the logic in compiler.cc and add a DCHECK that the scopes
and compiler.cc agree.

Use this knowledge to only created ScopeInfos for literals we'll
actually compile.

BUG=v8:5394,v8:5422
R=marja@chromium.org,verwaest@chromium.org

Committed: https://crrev.com/940efafd3d9989ca6baedb232ace979bef699d7b
Cr-Commit-Position: refs/heads/master@{#40074}
==========

[commit-bot: I haz the power, 2016-10-07 09:13:13]

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