Take ast node id counting away from Isolate.

Take ast node id counting away from Isolate.

When we're going to parse multiple scripts in parallel, we cannot have the
Isolate count the ast node ids.

Now the counter is stored in CompilationInfo instead. This is because we need to
add ast nodes after parsing too.

R=rossberg@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=23301

[marja, 2014-08-21 11:22:30]

rossberg, ptal

wingo, fyi, this is somewhat related to the "extra pass for bailout IDs" stuff
you're looking at; though, the ast node ids are used for other things as well
afaics, so we might not be able to get rid of doing some id counting during the
parsing pass...

[rossberg, 2014-08-21 11:32:02]

https://codereview.chromium.org/490173002/diff/40001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/490173002/diff/40001/src/ast.h#newcode220
src/ast.h:220: static int GetNextId(int* id) { return ReserveIdRange(id, 1); }
Instead of passing raw int pointers around everywhere, can we encapsulate that
into a small IdGen class? This and the below method could then naturally move
there, and the class would prevent the rest of the code from messing up the
underlying counter variable.

[marja, 2014-08-21 11:58:52]

I put the IdGen class inside AstNode, since the name AstNode::IdGen makes
sense.. it's the "ast node id generator".

https://codereview.chromium.org/490173002/diff/40001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/490173002/diff/40001/src/ast.h#newcode220
src/ast.h:220: static int GetNextId(int* id) { return ReserveIdRange(id, 1); }
On 2014/08/21 11:32:01, rossberg wrote:
> Instead of passing raw int pointers around everywhere, can we encapsulate that
> into a small IdGen class? This and the below method could then naturally move
> there, and the class would prevent the rest of the code from messing up the
> underlying counter variable.

Done.

[rossberg, 2014-08-21 12:21:04]

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

https://codereview.chromium.org/490173002/diff/60001/src/ast.h#newcode197
src/ast.h:197: void set_id(int id) { id_ = id; }
Hm, instead of exposing the internal state like this, can't you have a copy
constructor for IdGen, and then save and restore the id_gen object as a whole
where necessary, instead of its internal state?

[marja, 2014-08-21 12:44:37]

... now you can decide which patch set you like better... :)

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

https://codereview.chromium.org/490173002/diff/60001/src/ast.h#newcode197
src/ast.h:197: void set_id(int id) { id_ = id; }
On 2014/08/21 12:21:04, rossberg wrote:
> Hm, instead of exposing the internal state like this, can't you have a copy
> constructor for IdGen, and then save and restore the id_gen object as a whole
> where necessary, instead of its internal state?

Sure I can, but it's pretty horrible IMO (see the latest patch set). :)

Also, having a copy ctor and assignment operator public (they should've been
private in my original version) creates an opportunity for bugs where somebody
passes AstNode::IdGens by value which usually doesn't make sense...

[rossberg, 2014-08-21 13:43:35]

I like it slightly better, but admittedly it's ugly either way. I leave it up to
you. If you go back to the other version then indeed, make copy & assignment
private.

https://codereview.chromium.org/490173002/diff/80001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/490173002/diff/80001/src/ast.h#newcode190
src/ast.h:190: IdGen& operator=(const IdGen& other) {
I don't think copy and assignment operators need explicit definitions, they
should be generated by default.

https://codereview.chromium.org/490173002/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/490173002/diff/80001/src/parser.cc#newcode751
src/parser.cc:751: *(info->ast_node_id_gen()) = AstNode::IdGen(0);
Isn't this default constructed in the CompilationInfo class? Are we reusing comp
infos in weird ways?

https://codereview.chromium.org/490173002/diff/80001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/490173002/diff/80001/src/parser.h#newcode383
src/parser.h:383: function_state->saved_id_gen_ =
*(function_state->ast_node_id_gen_);
Nit: redundant (and IMO confusing) parens here and in all the other places.

[marja, 2014-08-22 07:59:07]

Let's go with the copying version. wingo@'s stuff might change this slightly
anyway...

https://codereview.chromium.org/490173002/diff/80001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/490173002/diff/80001/src/ast.h#newcode190
src/ast.h:190: IdGen& operator=(const IdGen& other) {
On 2014/08/21 13:43:35, rossberg wrote:
> I don't think copy and assignment operators need explicit definitions, they
> should be generated by default.

Done.

https://codereview.chromium.org/490173002/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/490173002/diff/80001/src/parser.cc#newcode751
src/parser.cc:751: *(info->ast_node_id_gen()) = AstNode::IdGen(0);
On 2014/08/21 13:43:35, rossberg wrote:
> Isn't this default constructed in the CompilationInfo class? Are we reusing
comp
> infos in weird ways?

This would only change things if we parse with the same CompilationInfo twice.
That shouldn't happen in the current state. I was trying to be overly defensive
(in case somebody wants to parse the same compilationinfo again in the future)
and not change the behavior in that case. Removed this line.

https://codereview.chromium.org/490173002/diff/80001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/490173002/diff/80001/src/parser.h#newcode383
src/parser.h:383: function_state->saved_id_gen_ =
*(function_state->ast_node_id_gen_);
On 2014/08/21 13:43:35, rossberg wrote:
> Nit: redundant (and IMO confusing) parens here and in all the other places.

Done.

[rossberg, 2014-08-22 08:47:58]

lgtm

https://codereview.chromium.org/490173002/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/490173002/diff/80001/src/parser.cc#newcode751
src/parser.cc:751: *(info->ast_node_id_gen()) = AstNode::IdGen(0);
On 2014/08/22 07:59:07, marja wrote:
> On 2014/08/21 13:43:35, rossberg wrote:
> > Isn't this default constructed in the CompilationInfo class? Are we reusing
> comp
> > infos in weird ways?
> 
> This would only change things if we parse with the same CompilationInfo twice.
> That shouldn't happen in the current state. I was trying to be overly
defensive
> (in case somebody wants to parse the same compilationinfo again in the future)
> and not change the behavior in that case. Removed this line.

Right, I don't think we should ever do such reuse. I'm pretty sure more things
would break anyway.

https://codereview.chromium.org/490173002/diff/100001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/490173002/diff/100001/src/ast.h#newcode188
src/ast.h:188: explicit IdGen(int id) : id_(id) {}
Nit: could also merge the two constructors into one with default argument 0.

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

https://codereview.chromium.org/490173002/diff/100001/src/parser.cc#newcode349
src/parser.cc:349: saved_ast_node_id_gen_ = *(parser_->ast_node_id_gen_);
Nit: some places left with redundant parens

[marja, 2014-08-22 08:57:13]

thx for review

https://codereview.chromium.org/490173002/diff/100001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/490173002/diff/100001/src/ast.h#newcode188
src/ast.h:188: explicit IdGen(int id) : id_(id) {}
On 2014/08/22 08:47:58, rossberg wrote:
> Nit: could also merge the two constructors into one with default argument 0.

Done.

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

https://codereview.chromium.org/490173002/diff/100001/src/parser.cc#newcode349
src/parser.cc:349: saved_ast_node_id_gen_ = *(parser_->ast_node_id_gen_);
On 2014/08/22 08:47:58, rossberg wrote:
> Nit: some places left with redundant parens

Done.

[marja, 2014-08-22 11:12:43]

Committed patchset #8 manually as 23301 (presubmit successful).