When parsing inner functions, try to allocate in a temporary Zone

Parsing especially large nested functions takes up more memory than necessary.
Inner functions must be eagerly parsed for scope analysis, but the full AST is
also kept around even though it's not needed.

This CL mitigates this problem by allocating some AstNodes of the inner function
to a temporary Zone which is deallocated once the scope information has been
built. The remaining nodes (such as VariableProxy) must persist until scope
analysis actually happens, and have to be allocated to a parser-persistent Zone.

BUG=417697
LOG=N

Committed: https://crrev.com/33ec0b79b8ea60dcccf1d445b0cbd2eed8e1a165
Cr-Commit-Position: refs/heads/master@{#30685}

[conradw, 2015-09-09 11:35:45]

Patchset #2 (id:20001) has been deleted

[conradw, 2015-09-09 12:39:43]

conradw@chromium.org changed reviewers:
+ mstarzinger@chromium.org, rossberg@chromium.org, titzer@chromium.org

[conradw, 2015-09-09 12:39:44]

PTAL

Of the "worst" allocations mentioned here..
https://code.google.com/p/chromium/issues/detail?id=417697#c14

Allocations that are discarded:
NewAssignment, NewNumberLiteral, NewBinaryOperation, NewExpressionStatement,
NewProperty, NewCall

Allocations that aren't discarded as a result of this change:
NewVariableProxy (required for scope analysis), NewUnknown (what is this?),
NewNumber (created by AstValueFactory and might be discardable)

[titzer, 2015-09-09 19:48:16]

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

https://codereview.chromium.org/1304923004/diff/60001/src/ast.h#newcode3280
src/ast.h:3280: : zone_(ast_value_factory->zone()),
Can we rename this zone to be clearer as well?

[conradw, 2015-09-10 10:55:27]

Patchset #4 (id:80001) has been deleted

[conradw, 2015-09-10 11:17:29]

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

https://codereview.chromium.org/1304923004/diff/60001/src/ast.h#newcode3280
src/ast.h:3280: : zone_(ast_value_factory->zone()),
On 2015/09/09 19:48:16, titzer wrote:
> Can we rename this zone to be clearer as well?

Done.

[Michael Starzinger, 2015-09-10 12:16:54]

Looking good, just one suggestion.

https://codereview.chromium.org/1304923004/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1304923004/diff/100001/src/parser.cc#newcode4241
src/parser.cc:4241: factory()->set_zone(outer_zone);
Suggestion as discsussed offline: This explicit resetting to the previous zone
could become brittle should the thing it is scoping become more complex. Also
exposing AstFactory::set_zone can be abused. One counter proposal would be to
slightly change the API. I was thinking about something along the lines of:

{
  Zone temp_zone;
  bool can_use_temp_zone = ...;
  AstNodeFactory::ZoneScope scope(factory(), &temp_zone, can_use_temp_zone);
  ...
}

This way we gain two things:
1) There is no need to expose set_zone() to the general public, the new scope is
the only one being able to change the zone.
2) Even if the thing being covered by the scope gets more complex and early
bailout return statements are added, the resetting to the original zone and the
destruction of the body still happens.

WDYT?

[conradw, 2015-09-10 13:05:50]

Patchset #5 (id:120001) has been deleted

[conradw, 2015-09-10 13:11:51]

Patchset #5 (id:140001) has been deleted

[conradw, 2015-09-10 13:12:40]

https://codereview.chromium.org/1304923004/diff/100001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1304923004/diff/100001/src/parser.cc#newcode4241
src/parser.cc:4241: factory()->set_zone(outer_zone);
On 2015/09/10 12:16:54, Michael Starzinger wrote:
> Suggestion as discsussed offline: This explicit resetting to the previous zone
> could become brittle should the thing it is scoping become more complex. Also
> exposing AstFactory::set_zone can be abused. One counter proposal would be to
> slightly change the API. I was thinking about something along the lines of:
> 
> {
>   Zone temp_zone;
>   bool can_use_temp_zone = ...;
>   AstNodeFactory::ZoneScope scope(factory(), &temp_zone, can_use_temp_zone);
>   ...
> }
> 
> This way we gain two things:
> 1) There is no need to expose set_zone() to the general public, the new scope
is
> the only one being able to change the zone.
> 2) Even if the thing being covered by the scope gets more complex and early
> bailout return statements are added, the resetting to the original zone and
the
> destruction of the body still happens.
> 
> WDYT?

Sounds good! Done.

[Michael Starzinger, 2015-09-10 14:06:36]

LGTM from my end.

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

https://codereview.chromium.org/1304923004/diff/180001/src/ast.h#newcode3696
src/ast.h:3696: // HeapObjects which need to persist until scope analysis must
be allocated in
nit: s/HeapObjects/Zone objects/ ... because HeapObjects are never allocated in
a zone.

[conradw, 2015-09-10 14:15:56]

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

https://codereview.chromium.org/1304923004/diff/180001/src/ast.h#newcode3696
src/ast.h:3696: // HeapObjects which need to persist until scope analysis must
be allocated in
On 2015/09/10 14:06:36, Michael Starzinger wrote:
> nit: s/HeapObjects/Zone objects/ ... because HeapObjects are never allocated
in
> a zone.

Oops! Done.

[conradw, 2015-09-10 14:39:02]

The CQ bit was checked by conradw@chromium.org

[conradw, 2015-09-10 14:39:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1304923004/#ps200001
(title: "get terminology less wrong in comments")

[commit-bot: I haz the power, 2015-09-10 14:39:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304923004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304923004/200001

[commit-bot: I haz the power, 2015-09-10 14:41:03]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-09-10 14:41:26]

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