Put Scopes into temporary Zone (second try)

Put Scopes into temporary Zone (second try)

When parsing a eagerly-parsed-but-lazily-compiled function, we
used to put some of its AST nodes into a discardable Zone. This
CL puts the function Scope, its inner Scopes and the related AST
nodes (Declarations, VariableProxys) into the temporary Zone
too. This reduces peak memory usage and enables future work to
keep the temporary Zone around for later compilation.

BUG=

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

[marja, 2016-08-04 11:00:15]

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

[commit-bot: I haz the power, 2016-08-04 11:00:22]

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-08-04 11:03:28]

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

[commit-bot: I haz the power, 2016-08-04 11:03:29]

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

[marja, 2016-08-04 12:05:44]

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

[commit-bot: I haz the power, 2016-08-04 12:05: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-08-04 12:09:08]

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

[marja, 2016-08-04 12:09:09]

ptal

Here's the second try for the Scopes-Zones CL.

The bug was that function.length was not preserved properly. I added a test for
it, and some DCHECKs that other Scope properties are preserved.

The problem is that there's no way to force that AnalyzePartially preserves the
properties needed by SharedFunctionInfo::InitFromFunctionLiteral :(

One possibility would be to pass a null Scope to FunctionLiteral ctor when we're
parsing lazily, and gather the needed data already in
Parser::ParseFunctionLiteral, and pass them to FunctionLiteral ctor too. That
would ensure that we don't read from the Scope in
SharedFunctionInfo::InitFromFunctionLiteral. Would that approach be ok for you?

I'm still working on the test in a separate CL.

[commit-bot: I haz the power, 2016-08-04 12:10:46]

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

[commit-bot: I haz the power, 2016-08-04 12:10:47]

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-08-04 12:20:24]

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

[commit-bot: I haz the power, 2016-08-04 12:20:26]

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

[marja, 2016-08-04 12:22:32]

Forgot to say; the diffs should contain the fixes so hopefully they're easy to
review.

[commit-bot: I haz the power, 2016-08-04 12:39:05]

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

[commit-bot: I haz the power, 2016-08-04 12:39:06]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-08-04 16:49:16]

I'd be interested in seeing that the null scope approach would look like. And I
did just run into a similar bug myself, when I tried to change how Scopes
calculate NeedsHomeObject. So I can see the benefit to giving FunctionLiteral
explicit signals about all the info it needs.

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

https://codereview.chromium.org/2210243002/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:994: migrate_to->force_context_allocation_ =
force_context_allocation_;
Besides fixing the DCHECK, what effect does this have?

[marja, 2016-08-04 18:48:51]

Summarizing discussion w/ adamk@:

- The "null scope" thing I suggested doesn't fly; we don't want to bloat
FunctionLiteral.
- We didn't think of any other remotely elegant way to enforce that possible new
fields in Scope get properly migrated.
- As discussed, I added a comment in ScopeInfo::InitFromFunctionLiteral
- We also decided to migrate has_forced_context_allocation_ for now, because
it's nontrivial to see it would be recomputed correctly.

[marja, 2016-08-04 18:49:05]

s/didn't think/couldn't think/

[adamk, 2016-08-04 18:49:22]

lgtm

and thanks for summarizing our discussion

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

https://codereview.chromium.org/2210243002/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:994: migrate_to->force_context_allocation_ =
force_context_allocation_;
On 2016/08/04 16:49:15, adamk wrote:
> Besides fixing the DCHECK, what effect does this have?

As discussed, I'm fine with migrating this.

[marja, 2016-08-04 18:53:10]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2016-08-04 18:53:14]

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-04 19:15:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-04 19:16:03]

Description was changed from

==========
Put Scopes into temporary Zone (second try)

When parsing a eagerly-parsed-but-lazily-compiled function, we
used to put some of its AST nodes into a discardable Zone. This
CL puts the function Scope, its inner Scopes and the related AST
nodes (Declarations, VariableProxys) into the temporary Zone
too. This reduces peak memory usage and enables future work to
keep the temporary Zone around for later compilation.

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

to

==========
Put Scopes into temporary Zone (second try)

When parsing a eagerly-parsed-but-lazily-compiled function, we
used to put some of its AST nodes into a discardable Zone. This
CL puts the function Scope, its inner Scopes and the related AST
nodes (Declarations, VariableProxys) into the temporary Zone
too. This reduces peak memory usage and enables future work to
keep the temporary Zone around for later compilation.

BUG=

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

[commit-bot: I haz the power, 2016-08-04 19:16:04]

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