Assign unique IDs to FunctionLiterals

Assign unique IDs to FunctionLiterals

They're supposed to be stable across several parse passes, so we'll also
store them in the associated SharedFunctionInfos

To achieve this, the PreParser and Parser need to generated the same number of
FunctionLiterals. To achieve this, we teach the PreParser about desuggaring of
class literals.

For regular functions, the function IDs are assigned in the order they occur in
the source. For arrow functions, however, we only know that it's an arrow function
after parsing the parameter list, and so the ID assigned to the arrow function is
larger than the IDs assigned to functions defined in the parameter list. This
implies that we have to reset the function ID counter to before the parameter list
when re-parsing an arrow function. To be able to do this, we store the number of
function literals found in the parameter list of arrow functions as well.

BUG=v8:5589
Committed: https://crrev.com/cfebe6034c39562681c11e8381448f4934972429
Cr-Commit-Position: refs/heads/master@{#41309}

[jochen (gone - plz use gerrit), 2016-11-18 04:45:32]

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

[commit-bot: I haz the power, 2016-11-18 04:45:42]

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-11-18 04:48:27]

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

[commit-bot: I haz the power, 2016-11-18 04:48:28]

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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16426)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16387)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12758)

[jochen (gone - plz use gerrit), 2016-11-18 04:58:35]

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

[commit-bot: I haz the power, 2016-11-18 04:58:37]

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-11-18 05:21:27]

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

[commit-bot: I haz the power, 2016-11-18 05:21:27]

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

[jochen (gone - plz use gerrit), 2016-11-18 09:12:27]

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

[commit-bot: I haz the power, 2016-11-18 09:12:28]

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-11-18 09:20:07]

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

[jochen (gone - plz use gerrit), 2016-11-18 09:20:08]

ptal

[commit-bot: I haz the power, 2016-11-18 09:37:26]

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

[commit-bot: I haz the power, 2016-11-18 09:37:27]

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

[marja, 2016-11-18 10:48:04]

Initial set of comments; however I'll need to look at this closer to understand
this...

https://codereview.chromium.org/2481163002/diff/140001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2481163002/diff/140001/src/ast/ast.h#newcode3491
src/ast/ast.h:3491: FunctionLiteral::kShouldLazyCompile, 0, false, true, 0);
What does 0 mean here? Invalid? Or just some default value? How about defining a
constant for it?

Why is it ok that multiple FunctionLiterals created via this code path have the
same ID? Why doesn't that break anything? (Maybe add a comment which answers
that question.)

https://codereview.chromium.org/2481163002/diff/140001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2481163002/diff/140001/src/factory.cc#newcode...
src/factory.cc:2319: share->set_function_literal_num(-1);
Aha, so 0 probably is not "invalid" since we have -1 here... defining constants
for these would make it clearer :)

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/parse-info.h
File src/parsing/parse-info.h (right):

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/parse-info...
src/parsing/parse-info.h:235: int function_literal_num_;
How about using function_literal_id instead of function_literal_num in all
places? Afaik "num" is mostly used in the context of "number of blobs", e.g.,
num_contexts, num_allocatable_double_registers etc.

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

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/parser-bas...
src/parsing/parser-base.h:2712: if (function_literal_num ==
function_literal_num_) {
Looks like we could just put the function literal getting into the arrow branch
where we know we're parsing an arrow function.

[marja, 2016-11-18 10:51:10]

Ahh, I just realized you cannot do the id getting in the arrow branch because
you want to number the function before possible other functions in its param
list.

Maybe add a comment about that?

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/preparser....
src/parsing/preparser.h:1090: class_info->constructor =
PreParserExpression::Empty();
Hmm, why does this matter?

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/preparser....
src/parsing/preparser.h:1102: bool has_instance_fields =
Where's the equivalent thing for these on the parser side?

[jochen (gone - plz use gerrit), 2016-11-18 15:12:01]

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

[commit-bot: I haz the power, 2016-11-18 15:12:05]

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-11-18 15:52:20]

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

[commit-bot: I haz the power, 2016-11-18 15:52:20]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-11-21 08:03:30]

Description was changed from

==========
Assign unique IDs to FunctionLiterals

They're supposed to be stable across several parse passes, so we'll also
store them in the associated SharedFunctionInfos

BUG=v8:5589
==========

to

==========
Assign unique IDs to FunctionLiterals

They're supposed to be stable across several parse passes, so we'll also
store them in the associated SharedFunctionInfos

To achieve this, the PreParser and Parser need to generated the same number of
FunctionLiterals. To achieve this, we teach the PreParser about desuggaring of
class literals.

For regular functions, the function IDs are assigned in the order they occur in
the source. For arrow functions, however, we only know that it's an arrow
function
after parsing the parameter list, and so the ID assigned to the arrow function
is larger than the IDs assigned to functions defined in the parameter list. This
implies that we have to reset the function ID counter to before the parameter
list
when re-parsing an arrow function. To be able to do this, we store the number of
function literals found in the parameter list of arrow functions as well.

BUG=v8:5589
==========

[jochen (gone - plz use gerrit), 2016-11-21 08:03:47]

https://codereview.chromium.org/2481163002/diff/140001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2481163002/diff/140001/src/ast/ast.h#newcode3491
src/ast/ast.h:3491: FunctionLiteral::kShouldLazyCompile, 0, false, true, 0);
On 2016/11/18 at 10:48:04, marja wrote:
> What does 0 mean here? Invalid? Or just some default value? How about defining
a constant for it?
> 
> Why is it ok that multiple FunctionLiterals created via this code path have
the same ID? Why doesn't that break anything? (Maybe add a comment which answers
that question.)

-1 means invalid, zero means top-level function. added named constants

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/parse-info.h
File src/parsing/parse-info.h (right):

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/parse-info...
src/parsing/parse-info.h:235: int function_literal_num_;
On 2016/11/18 at 10:48:04, marja wrote:
> How about using function_literal_id instead of function_literal_num in all
places? Afaik "num" is mostly used in the context of "number of blobs", e.g.,
num_contexts, num_allocatable_double_registers etc.

done

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

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/parser-bas...
src/parsing/parser-base.h:2712: if (function_literal_num ==
function_literal_num_) {
On 2016/11/18 at 10:48:04, marja wrote:
> Looks like we could just put the function literal getting into the arrow
branch where we know we're parsing an arrow function.

I changed the code to not allocate a function literal if we don't need one

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/preparser....
src/parsing/preparser.h:1090: class_info->constructor =
PreParserExpression::Empty();
On 2016/11/18 at 10:51:10, marja wrote:
> Hmm, why does this matter?

When the ParserBase defines the constructor, it sets has_seen_constructor to
true and invokes DeclareClassProperty. However, all further calls to
DeclareClassProperty will also have has_seen_constructor set to true, so we can
need to also look at constructor. This is more or less the same code that Parser
has

https://codereview.chromium.org/2481163002/diff/140001/src/parsing/preparser....
src/parsing/preparser.h:1102: bool has_instance_fields =
On 2016/11/18 at 10:51:10, marja wrote:
> Where's the equivalent thing for these on the parser side?

In Parser::RewriteClassLiteral - it's exactly the same code

[jochen (gone - plz use gerrit), 2016-11-21 08:04:21]

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

[commit-bot: I haz the power, 2016-11-21 08:04:26]

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-11-21 08:32:31]

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

[commit-bot: I haz the power, 2016-11-21 08:32:31]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-11-21 10:54:58]

Description was changed from

==========
Assign unique IDs to FunctionLiterals

They're supposed to be stable across several parse passes, so we'll also
store them in the associated SharedFunctionInfos

To achieve this, the PreParser and Parser need to generated the same number of
FunctionLiterals. To achieve this, we teach the PreParser about desuggaring of
class literals.

For regular functions, the function IDs are assigned in the order they occur in
the source. For arrow functions, however, we only know that it's an arrow
function
after parsing the parameter list, and so the ID assigned to the arrow function
is larger than the IDs assigned to functions defined in the parameter list. This
implies that we have to reset the function ID counter to before the parameter
list
when re-parsing an arrow function. To be able to do this, we store the number of
function literals found in the parameter list of arrow functions as well.

BUG=v8:5589
==========

to

==========
Assign unique IDs to FunctionLiterals

They're supposed to be stable across several parse passes, so we'll also
store them in the associated SharedFunctionInfos

To achieve this, the PreParser and Parser need to generated the same number of
FunctionLiterals. To achieve this, we teach the PreParser about desuggaring of
class literals.

For regular functions, the function IDs are assigned in the order they occur in
the source. For arrow functions, however, we only know that it's an arrow
function
after parsing the parameter list, and so the ID assigned to the arrow function
is
larger than the IDs assigned to functions defined in the parameter list. This
implies that we have to reset the function ID counter to before the parameter
list
when re-parsing an arrow function. To be able to do this, we store the number of
function literals found in the parameter list of arrow functions as well.

BUG=v8:5589
==========

[Toon Verwaest, 2016-11-21 14:41:43]

We really should't be storing 2 smis in the SharedFunctionInfos, that's pretty
expensive memory-wise. SFI are very common. We should be able to deal with arrow
functions during parsing already in some way.

I see the following ways:
- first just count the number of sub-functions and then fill in the actual IDs
later
- let the rewriter fix up function IDs
- only do the former when reparsing arrow functions

I see a lot of issues with approaches 2 and 3 though. The first is probably
easiest, but needs a separate phase (or piggyback on e.g., AstNumbering).

[jochen (gone - plz use gerrit), 2016-11-23 14:39:45]

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

[commit-bot: I haz the power, 2016-11-23 14:39:48]

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-11-23 14:40:04]

I did option 3) ptal

[jochen (gone - plz use gerrit), 2016-11-23 14:42:17]

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

[commit-bot: I haz the power, 2016-11-23 14:42: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-11-23 15:09:33]

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

[commit-bot: I haz the power, 2016-11-23 15:09:34]

Dry run: This issue passed the CQ dry run.

[marja, 2016-11-25 13:14:56]

lgtm, but pls wait for verwaest@'s review.

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser....
src/parsing/preparser.cc:95: ResetFunctionLiteralId();
Had to think a bit about this... so this is because the function ids are only
used for counting the number of inner functions here, right? So they don't
affect what IDs any entities actually get. Can you add a comment about that.

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser....
src/parsing/preparser.h:1086: if (class_info->has_seen_constructor &&
This function could still use a comment... as far as I understand it now, it
sets up class_info in such a way that RewriteClassLiteral can do its thing. And
the thing cannot be done here since it's too early.

RewriteClassLiteral checks instance_field_initializers, so here we add stuff to
it...

But I still don't get why we need to set class_info->constructor. What part
checks it? RewriteClassLiteral doesn't. ELI5?

[jochen (gone - plz use gerrit), 2016-11-28 08:12:08]

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser....
src/parsing/preparser.cc:95: ResetFunctionLiteralId();
On 2016/11/25 at 13:14:56, marja wrote:
> Had to think a bit about this... so this is because the function ids are only
used for counting the number of inner functions here, right? So they don't
affect what IDs any entities actually get. Can you add a comment about that.

done

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser....
src/parsing/preparser.h:1086: if (class_info->has_seen_constructor &&
On 2016/11/25 at 13:14:56, marja wrote:
> This function could still use a comment... as far as I understand it now, it
sets up class_info in such a way that RewriteClassLiteral can do its thing. And
the thing cannot be done here since it's too early.
> 
> RewriteClassLiteral checks instance_field_initializers, so here we add stuff
to it...
> 
> But I still don't get why we need to set class_info->constructor. What part
checks it? RewriteClassLiteral doesn't. ELI5?

I guess the contract of DeclareClassProperty is just too confusing.. from the
invocation on where the encountered property is the constructor,
has_seen_constructor is true. To be able to tell the single constructor apart
from other class literals that might get added to instance_field_initializers,
we need a second bit of information that tells us that we observed the switch of
has_seen_constructor from false to true.

Anyway, I've now added an explicit "is_constructor" parameter to clarify this.

[jochen (gone - plz use gerrit), 2016-11-28 08:12:28]

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

[commit-bot: I haz the power, 2016-11-28 08:12:36]

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-11-28 08:42:12]

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

[commit-bot: I haz the power, 2016-11-28 08:42:13]

Dry run: This issue passed the CQ dry run.

[marja, 2016-11-28 09:10:54]

still lgtm (I was confused about what DeclareClassProperty does, esp the
preparser version...)

[Toon Verwaest, 2016-11-28 11:12:34]

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser....
src/parsing/preparser.h:1105: if (has_default_constructor)
GetNextFunctionLiteralId();
Do we really need an ID for the default constructor? THe default constructor
doesn't *really* live in the script. You can't break in it or anything ...

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser....
src/parsing/preparser.h:1107: // Account for initializer function.
Can we e.g., put breakpoints in the initializer by putting it on the field
initializer expression?

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

https://codereview.chromium.org/2481163002/diff/240001/src/parsing/parser-bas...
src/parsing/parser-base.h:4150: ClassLiteralPropertyT property =
ParseClassPropertyDefinition(
This is getting a little out of hand, but I guess can be improved in a follow-up
cl.

[jochen (gone - plz use gerrit), 2016-11-28 11:26:50]

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/2481163002/diff/220001/src/parsing/preparser....
src/parsing/preparser.h:1105: if (has_default_constructor)
GetNextFunctionLiteralId();
On 2016/11/28 at 11:12:33, Toon Verwaest wrote:
> Do we really need an ID for the default constructor? THe default constructor
doesn't *really* live in the script. You can't break in it or anything ...

they get lazily compiled, so they end up in the SFI cache etc.. the alternative
to giving them function IDs is adding special cases for them all over the place
(like DoParseFunction already has)

[Toon Verwaest, 2016-11-28 11:37:58]

Ok then, let's keep it like this for now. That's quite broken though...
lgtm

[jochen (gone - plz use gerrit), 2016-11-28 11:38:21]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2016-11-28 11:38:28]

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-11-28 11:40:23]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480333101534840,
"parent_rev": "021ef90fe03a66773b28aa97d1b5422c8825d115", "commit_rev":
"0009db55898cf03f0dad9420aee6fcb12ce79997"}

[commit-bot: I haz the power, 2016-11-28 11:40:27]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-11-28 11:40:57]

Description was changed from

==========
Assign unique IDs to FunctionLiterals

They're supposed to be stable across several parse passes, so we'll also
store them in the associated SharedFunctionInfos

To achieve this, the PreParser and Parser need to generated the same number of
FunctionLiterals. To achieve this, we teach the PreParser about desuggaring of
class literals.

For regular functions, the function IDs are assigned in the order they occur in
the source. For arrow functions, however, we only know that it's an arrow
function
after parsing the parameter list, and so the ID assigned to the arrow function
is
larger than the IDs assigned to functions defined in the parameter list. This
implies that we have to reset the function ID counter to before the parameter
list
when re-parsing an arrow function. To be able to do this, we store the number of
function literals found in the parameter list of arrow functions as well.

BUG=v8:5589
==========

to

==========
Assign unique IDs to FunctionLiterals

They're supposed to be stable across several parse passes, so we'll also
store them in the associated SharedFunctionInfos

To achieve this, the PreParser and Parser need to generated the same number of
FunctionLiterals. To achieve this, we teach the PreParser about desuggaring of
class literals.

For regular functions, the function IDs are assigned in the order they occur in
the source. For arrow functions, however, we only know that it's an arrow
function
after parsing the parameter list, and so the ID assigned to the arrow function
is
larger than the IDs assigned to functions defined in the parameter list. This
implies that we have to reset the function ID counter to before the parameter
list
when re-parsing an arrow function. To be able to do this, we store the number of
function literals found in the parameter list of arrow functions as well.

BUG=v8:5589

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

[commit-bot: I haz the power, 2016-11-28 11:40:59]

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