Put Scopes into temporary Zone

Put Scopes into temporary Zone

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/eaebdd858b466057ccc39894a172c9b66868e8f7
Cr-Commit-Position: refs/heads/master@{#38232}

[marja, 2016-07-29 11:04:17]

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

[commit-bot: I haz the power, 2016-07-29 11:04:20]

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

[marja, 2016-07-29 11:13:07]

Description was changed from

==========
Put Scopes into temporary Zone (first step).

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

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

[marja, 2016-07-29 11:52:53]

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

[commit-bot: I haz the power, 2016-07-29 11:52:58]

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-07-29 12:06:30]

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

[commit-bot: I haz the power, 2016-07-29 12:06:31]

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

[marja, 2016-07-29 12:14:37]

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

[commit-bot: I haz the power, 2016-07-29 12:14:42]

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

[marja, 2016-07-29 12:18:49]

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

[marja, 2016-07-29 12:18:50]

Alright, here's a new version, going a bit further (so putting the actual
function Scope into the temp Zone (not just some weird middle-level Scope). I
was fearing the diff would be horrible but it's actually pretty okay!

adamk@, now that you seem to be back, would you like to review this? titzer@,
vogelheim@, you're ofc welcome to have a look too.

Design doc is here:
https://docs.google.com/a/chromium.org/document/d/1JLYNxNw70NpfBnNA7Baq1rRsp8...

Re: tests; I added one test I noticed was missing. Then I inserted a bunch of
DCHECKs to make sure that the Scope chain is sane in the end: that all Scopes
belong to the same Zone, and that whenever we create an AST node to be put into
a Scope, they are from the same Zone.

[marja, 2016-07-29 12:24:25]

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

[commit-bot: I haz the power, 2016-07-29 12:24:31]

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

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

[commit-bot: I haz the power, 2016-07-29 12:52:48]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-07-29 22:12:51]

This is looking good (and I'd be happy to keep doing reviews in this area!)

Some questions below.

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

https://codereview.chromium.org/2193793002/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:902: // any new AstNodes, since none of the Scopes involved
are backed up by
Is there an obvious place to add a DCHECK for this invariant?

https://codereview.chromium.org/2193793002/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:930: proxy->is_this() ? Variable::THIS : Variable::NORMAL,
proxy->position(),
I had to go read some code to understand why you didn't have to deal with
Variable::ARGUMENTS and Variable::FUNCTION. Seems kind of wrong that we re-use
the Variable::Kind enum for this purpose, but maybe it's no worse than a
boolean.

https://codereview.chromium.org/2193793002/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:935: PropagateScopeInfo(false);
This is called before variable resolution in Scope::Analyze, but I guess that
doesn't matter here because we don't actually care what kind of binding we have?
Either we find a variable or not.

https://codereview.chromium.org/2193793002/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:946: migrate_to->language_mode_ = language_mode_;
One thing I'd recommend before submitting would be to comment out bits of this
function (like these flags) and make sure that something breaks. If it doesn't,
needs more tests :)

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

https://codereview.chromium.org/2193793002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:4513: CheckConflictingVarDeclarations(scope, CHECK_OK);
It scares me that we're still operating on the temp scope at this point, since
it's in a strange state (it's been disconnected from its outer scope, its
unresolved variables are all wrong). I'd feel better if AnalyzePartially() were
the last thing we did before discarding the zone (and thus the scope). Would
that work?

[marja, 2016-08-01 08:45:47]

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

[commit-bot: I haz the power, 2016-08-01 08:45:56]

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-01 08:46:12]

The CQ bit was unchecked by marja@chromium.org

[marja, 2016-08-01 08:51:07]

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

[commit-bot: I haz the power, 2016-08-01 08:51:13]

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-01 09:20:54]

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

[commit-bot: I haz the power, 2016-08-01 09:20:55]

Dry run: This issue passed the CQ dry run.

[marja, 2016-08-01 10:47:59]

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

[commit-bot: I haz the power, 2016-08-01 10:48:01]

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-01 11:01:49]

Thanks for comments!

[marja, 2016-08-01 11:02:11]

Wut, why didn't that send my draft comments... trying again.

[marja, 2016-08-01 11:02:50]

Aha, this sends my draft comments.

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

https://codereview.chromium.org/2193793002/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:902: // any new AstNodes, since none of the Scopes involved
are backed up by
On 2016/07/29 22:12:51, adamk wrote:
> Is there an obvious place to add a DCHECK for this invariant?

I added a DCHECK_NOT_NULL(factory); statement in LookupFunctionVar which was the
only one along this code path.

They're not super useful though, since we'd crash anyway if we try to use a
nullptr as AstNodeFactory. It's a matter of style whether we should just let it
crash or DCHECK / CHECK nullity. :)

https://codereview.chromium.org/2193793002/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:930: proxy->is_this() ? Variable::THIS : Variable::NORMAL,
proxy->position(),
On 2016/07/29 22:12:51, adamk wrote:
> I had to go read some code to understand why you didn't have to deal with
> Variable::ARGUMENTS and Variable::FUNCTION. Seems kind of wrong that we re-use
> the Variable::Kind enum for this purpose, but maybe it's no worse than a
> boolean.

Yeah, this is not good, and it's also nontrivial to see whether other
VariableProxy flags such as is_assigned() need to be migrated or not.

I added a methods into AstNodeFactory / VariableProxy to do the cloning in a
clean way. This way, if there is a new field added to VariableProxy, it's more
probable that the cloning functions get updated properly.

https://codereview.chromium.org/2193793002/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:935: PropagateScopeInfo(false);
On 2016/07/29 22:12:51, adamk wrote:
> This is called before variable resolution in Scope::Analyze, but I guess that
> doesn't matter here because we don't actually care what kind of binding we
have?
> Either we find a variable or not.

I moved this call up, just to keep it more unified; there was no particular
reason to do it in this order.

https://codereview.chromium.org/2193793002/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:946: migrate_to->language_mode_ = language_mode_;
On 2016/07/29 22:12:51, adamk wrote:
> One thing I'd recommend before submitting would be to comment out bits of this
> function (like these flags) and make sure that something breaks. If it
doesn't,
> needs more tests :)

Done: I didn't have these things from the start, and only added them because
something broke :)

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

https://codereview.chromium.org/2193793002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:4513: CheckConflictingVarDeclarations(scope, CHECK_OK);
On 2016/07/29 22:12:51, adamk wrote:
> It scares me that we're still operating on the temp scope at this point, since
> it's in a strange state (it's been disconnected from its outer scope, its
> unresolved variables are all wrong). I'd feel better if AnalyzePartially()
were
> the last thing we did before discarding the zone (and thus the scope). Would
> that work?

Done.

[commit-bot: I haz the power, 2016-08-01 11:16:34]

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

[commit-bot: I haz the power, 2016-08-01 11:16:35]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-08-01 17:58:41]

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

[titzer, 2016-08-01 17:58:44]

Looks mostly good for me. Unit tests would increase confidence that this is
correct.

https://codereview.chromium.org/2193793002/diff/180001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2193793002/diff/180001/src/ast/ast.h#newcode3278
src/ast/ast.h:3278: VariableProxy* CloneVariableProxy(VariableProxy* proxy) {
s/Clone/Copy/ ?

Not sure it makes much difference, but it's one less character.
Also, comment should say, "in this Zone" instead of "in a different Zone".

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

https://codereview.chromium.org/2193793002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:592: void CollectUnresolvableLocals(VariableProxy**
still_unresolved,
Since this is a public method, is it possible to write a unit test for it?

https://codereview.chromium.org/2193793002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:622: void AnalyzePartially(Scope* migrate_to, AstNodeFactory*
ast_node_factory);
Same here. Is it possible to write a unittest that cooks up a fake scope and
calls this method on it?

[adamk, 2016-08-01 18:34:33]

lgtm % nits

titzer's suggestion of unittests is interesting, it'd be a new direction for
most of this stuff...I'm not sure how I feel about it yet.

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

https://codereview.chromium.org/2193793002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:932: // Recrate the VariableProxy.
s/Recrate/Recreate/

https://codereview.chromium.org/2193793002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:933: DCHECK_EQ(proxy->is_resolved(), false);
Nit: I'd prefer DCHECK(!proxy->is_resolved()) :)

[adamk, 2016-08-01 18:41:38]

On 2016/08/01 18:34:33, adamk wrote:
> lgtm % nits
> 
> titzer's suggestion of unittests is interesting, it'd be a new direction for
> most of this stuff...I'm not sure how I feel about it yet.

What I meant to imply here is that I don't think this CL needs to block on
unittests, since that's not something we've generally done for Scope/Parsing
work.

And when I say "I'm not sure how I feel about it", I mean the idea of
introducing unittests for Scope and Parser (which could be valuable, or could be
problematic, depending on how it's approached); I think it's a bigger topic than
this CL.

[marja, 2016-08-02 07:42:37]

Thanks for comments!

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

https://codereview.chromium.org/2193793002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:932: // Recrate the VariableProxy.
On 2016/08/01 18:34:33, adamk wrote:
> s/Recrate/Recreate/

Done.

https://codereview.chromium.org/2193793002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:933: DCHECK_EQ(proxy->is_resolved(), false);
On 2016/08/01 18:34:33, adamk wrote:
> Nit: I'd prefer DCHECK(!proxy->is_resolved()) :)

Ha, I'm only using this form because some code reviewers insisted... :)

But I changed it to the form you prefer; it seems to be more common in V8.

https://codereview.chromium.org/2193793002/diff/180001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2193793002/diff/180001/src/ast/ast.h#newcode3278
src/ast/ast.h:3278: VariableProxy* CloneVariableProxy(VariableProxy* proxy) {
On 2016/08/01 17:58:43, titzer wrote:
> s/Clone/Copy/ ?
> 
> Not sure it makes much difference, but it's one less character.
> Also, comment should say, "in this Zone" instead of "in a different Zone".

Done.

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

https://codereview.chromium.org/2193793002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:592: void CollectUnresolvableLocals(VariableProxy**
still_unresolved,
On 2016/08/01 17:58:43, titzer wrote:
> Since this is a public method, is it possible to write a unit test for it?

Oops, this was public by accident, making it private.

The test class can anyway be a friend of Scope, so I don't mean to reduce
testability by making it private. :)

https://codereview.chromium.org/2193793002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:622: void AnalyzePartially(Scope* migrate_to, AstNodeFactory*
ast_node_factory);
On 2016/08/01 17:58:43, titzer wrote:
> Same here. Is it possible to write a unittest that cooks up a fake scope and
> calls this method on it?

I'll look into that after this CL to see how much code around Scope we'd need to
unit test it. verwaest@ is going to rewrite the whole scope analysis, so we need
to see what amount of unit testing is reasonable before the rewrite. I'm
optimistic that it won't require much additional machinery, but as adamk@
suggested, I won't block landing this CL on that test.

[marja, 2016-08-02 07:43:28]

The CQ bit was checked by marja@chromium.org

[marja, 2016-08-02 07:43:28]

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

[commit-bot: I haz the power, 2016-08-02 07:43:34]

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-02 08:06:09]

Description was changed from

==========
Put Scopes into temporary Zone

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

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

[commit-bot: I haz the power, 2016-08-02 08:06:11]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-02 08:08:41]

Description was changed from

==========
Put Scopes into temporary Zone

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

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/eaebdd858b466057ccc39894a172c9b66868e8f7
Cr-Commit-Position: refs/heads/master@{#38232}
==========

[commit-bot: I haz the power, 2016-08-02 08:08:42]

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

[adamk, 2016-08-02 16:13:09]

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

https://codereview.chromium.org/2193793002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:933: DCHECK_EQ(proxy->is_resolved(), false);
On 2016/08/02 07:42:37, marja wrote:
> On 2016/08/01 18:34:33, adamk wrote:
> > Nit: I'd prefer DCHECK(!proxy->is_resolved()) :)
> 
> Ha, I'm only using this form because some code reviewers insisted... :)
> 
> But I changed it to the form you prefer; it seems to be more common in V8.

CHECK_EQ makes sense to me if we want to know what the "bad" value was. But for
booleans it makes no difference.

[marja, 2016-08-02 18:11:50]

I'm going to revert this, because it broke some Node.js tests. I'll make sure to
add tests for the breaking cases to our tests before landing again...

[marja, 2016-08-02 18:13:55]

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

The reason for reverting is: Broke Node.js tests (test-require-dot etc.)

.