Don't use different function scopes when parsing with temp zones

Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone. Then we carefully copied back data to the main zone. This CL changes it so that the scope is just fixed up to only contain data from the main zone. That avoids additional copies and additional allocations; while not increasing the care that needs to be taken. This will also make it easier to abort preparsing while parsing using a temp zone.

BUG=

Committed: https://crrev.com/f41e7ebd62b32e861b6aa14ad8bfce3018d03c3c
Committed: https://crrev.com/669719d5fb22ffcd533c7cae82115079010578f8
Cr-Original-Commit-Position: refs/heads/master@{#39800}
Cr-Commit-Position: refs/heads/master@{#39828}

[Toon Verwaest, 2016-09-26 14:24:29]

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

[Toon Verwaest, 2016-09-26 14:24:30]

ptal

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

https://codereview.chromium.org/2368313002/diff/1/src/ast/scopes.cc#newcode956
src/ast/scopes.cc:956: // DCHECK_EQ(factory->zone(), zone());
We should probably just drop this?

[marja, 2016-09-27 07:14:02]

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

[marja, 2016-09-27 07:14:03]

I'd like adamk@ to review too, since the DCHECKS / Zone-Zone-pointer design
originated from discussions w/ him.

This approach has leaner code, but more pointers to a different Zone :/

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

https://codereview.chromium.org/2368313002/diff/1/src/ast/scopes.cc#newcode956
src/ast/scopes.cc:956: // DCHECK_EQ(factory->zone(), zone());
On 2016/09/26 14:24:30, Toon Verwaest wrote:
> We should probably just drop this?

Hmmmm... I think this was a nice DCHECK for guarding that we don't have Zone ->
different Zone pointers in Scope. It sounds risky to remove that invariant.
Would it be possible to do an equivalent check that we're in a consistent state
after AnalyzePartially? Maybe not b/c VariableProxy's don't know their Zone
(yet)?

https://codereview.chromium.org/2368313002/diff/1/src/ast/scopes.cc#newcode1211
src/ast/scopes.cc:1211: ResetAfterPreparsing();
This is somewhat surprising now since we haven't necessarily preparsed... but as
the "full parse but throw the code away" option is going away, maybe it's fine.

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

https://codereview.chromium.org/2368313002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:2846: if (use_temp_zone || is_lazy_top_level_function) {
This sounds wrong... is_lazy_inner_function maybe?

[marja, 2016-09-27 11:17:34]

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

https://codereview.chromium.org/2368313002/diff/1/src/ast/scopes.cc#newcode956
src/ast/scopes.cc:956: // DCHECK_EQ(factory->zone(), zone());
Suggestion: flag scopes w/ needs_migration, here we DHCECK(needs_migration_ ||
factory->zone() == zone());, then in AnalyzePartially we set needs_migration_ =
false; and finally as part of scope analysis somewhere we
DCHECK(!needs_migration);

That would solve my worry about having dangling pointers to a different Zone.

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

https://codereview.chromium.org/2368313002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:2846: if (use_temp_zone || is_lazy_top_level_function) {
On 2016/09/27 07:14:03, marja wrote:
> This sounds wrong... is_lazy_inner_function maybe?

.... and I was confused; use_temp_zone contains that.

[Toon Verwaest, 2016-09-27 12:48:34]

Description was changed from

==========
Don't different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

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

to

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

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

[Toon Verwaest, 2016-09-27 13:01:44]

Addressed comment. I'll need to merge in the abort-preparsing CL that's in the
CQ before I land though.

[marja, 2016-09-27 13:08:08]

this version lgtm

(adamk@, I think we came up with an elegant solution to the question I was
deferring to you, so, if you're busy, no need to look further into this)

[Toon Verwaest, 2016-09-27 14:06:38]

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

[commit-bot: I haz the power, 2016-09-27 14:06:52]

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

[Toon Verwaest, 2016-09-27 14:10:40]

Bugs required significant enough changes to require a re-lgtm. Please ptal
again.

Bugs:
- block scopes were allocated in the zone of the outer scope. This is wrong in
the case of temp zones if we don't manually insert a scope that switches zone.
Now instead I just pass in the correct zone to the scope constructor from the
parser.

- variable allocation also manually looked at new_target_ etc. To make sure we
won't break it again in the future when we add a var, now I skip allocation
whenever we hit a preparsed scope (could also mean fully parsed scope with a
temp zone)

- is_lazily_parsed wasn't properly set for temp-zone-parsed functions, since it
really just meant parsed with the preparser. That means we were allocating scope
infos for such functions, even though nobody needs them. Now I set the flag in
ResetAfterPreparsing depending on the abort flag; which means it's also set for
temp-zone-parsed (partially analyzed) scopes.

[commit-bot: I haz the power, 2016-09-27 14:44:43]

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

[commit-bot: I haz the power, 2016-09-27 14:44:44]

Dry run: This issue passed the CQ dry run.

[marja, 2016-09-27 18:54:41]

lgtm modulo comment

cc jochen@ because this touches the "scope infos for preparsed funcs" logic

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

https://codereview.chromium.org/2368313002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:571: DCHECK(!needs_migration_ || inner_scope->zone() !=
zone());
Hmm? Why not DCHECK(needs_migration_ || inner_scope->zone() == zone()); ?

Let's see.. your DCHECK says:
either needs_migration_ is false or zones differ (or both)
so it's disallowing zones to be same if needs_migration_

I guess that makes sense too (that asserts that the Scope which needs migration
really is on the Zone boundary) but I think the more interesting check is that
Zones match unless needs_migration_.

So why not have both:
DCHECK(needs_migration_ == (inner_scope->zone() != zone()); ?

[adamk, 2016-09-27 20:33:17]

lgtm, but one question and one nit.

https://codereview.chromium.org/2368313002/diff/180001/src/ast/scopes.cc
File src/ast/scopes.cc (left):

https://codereview.chromium.org/2368313002/diff/180001/src/ast/scopes.cc#oldc...
src/ast/scopes.cc:1217: has_rest_ = false;
Was this just unnecessary?

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

https://codereview.chromium.org/2368313002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:490: bool needs_migration_ : 1;
Can we remove these ": 1"s from these debug-only fields? They just look like
noise to me (I doubt we care about the size of a debug version of Scope...)

[Toon Verwaest, 2016-09-27 20:41:44]

https://codereview.chromium.org/2368313002/diff/180001/src/ast/scopes.cc
File src/ast/scopes.cc (left):

https://codereview.chromium.org/2368313002/diff/180001/src/ast/scopes.cc#oldc...
src/ast/scopes.cc:1217: has_rest_ = false;
On 2016/09/27 20:33:17, adamk wrote:
> Was this just unnecessary?

It was necessary to make allocation of these scopes work; but now we explicitly
never allocate them to reduce risk of accidental breakage later. Otherwise we'd
have to keep all special variables synced here like new_target_ etc.

[Toon Verwaest, 2016-09-28 02:15:13]

Addressed comments

[Toon Verwaest, 2016-09-28 02:15:17]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-09-28 02:15:17]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2368313002/#ps200001
(title: "Address comments")

[commit-bot: I haz the power, 2016-09-28 02:15:29]

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-09-28 02:42:23]

Description was changed from

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

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

to

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

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

[commit-bot: I haz the power, 2016-09-28 02:42:24]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-28 02:42:39]

Description was changed from

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

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

to

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

BUG=

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

[commit-bot: I haz the power, 2016-09-28 02:42:40]

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

[Toon Verwaest, 2016-09-28 09:21:16]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2379533003/ by verwaest@chromium.org.

The reason for reverting is: Revert due to asm.js slowdown.

[Toon Verwaest, 2016-09-28 12:55:36]

Description was changed from

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

BUG=

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

to

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

BUG=

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

[Toon Verwaest, 2016-09-28 13:09:58]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-09-28 13:09:59]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2368313002/#ps260001
(title: "Make sure arguments_ is properly analyzed as well, since it's used as a
signal for optimization")

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

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-09-28 13:36:36]

Description was changed from

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

BUG=

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

to

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

BUG=

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

[commit-bot: I haz the power, 2016-09-28 13:36:37]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-09-28 13:36:58]

Description was changed from

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

BUG=

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

to

==========
Don't use different function scopes when parsing with temp zones

Previously we'd have a scope in the main zone, and another in the temp zone.
Then we carefully copied back data to the main zone. This CL changes it so that
the scope is just fixed up to only contain data from the main zone. That avoids
additional copies and additional allocations; while not increasing the care that
needs to be taken. This will also make it easier to abort preparsing while
parsing using a temp zone.

BUG=

Committed: https://crrev.com/f41e7ebd62b32e861b6aa14ad8bfce3018d03c3c
Committed: https://crrev.com/669719d5fb22ffcd533c7cae82115079010578f8
Cr-Original-Commit-Position: refs/heads/master@{#39800}
Cr-Commit-Position: refs/heads/master@{#39828}
==========

[commit-bot: I haz the power, 2016-09-28 13:36:59]

Patchset 14 (id:??) landed as
https://crrev.com/669719d5fb22ffcd533c7cae82115079010578f8
Cr-Commit-Position: refs/heads/master@{#39828}