Scope tree serialization and ScopeIterator cleanup.

Scope tree serialization and ScopeIterator cleanup.

The intention is to store enough scope information for the debugger to
handle stack allocation of block scoped variables introduced by
http://codereview.chromium.org/7860045/ .

This CL is based on
http://codereview.chromium.org/7904008/ .

Committed: http://code.google.com/p/v8/source/detail?r=9673

[Steven, 2011-09-20 10:21:21]

PTAL.

[Kevin Millikin (Chromium), 2011-10-05 08:43:36]

I like the approach.  I think it's clear that both ScopeInfo and the
ScopeIterator used by the debugger could be cleaned up.  Many of my comments are
tracking suggestions for future cleanup.

I did not look closely at the ScopeIterator yet, I'm doing that now.

http://codereview.chromium.org/7979001/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7979001/diff/1/src/parser.cc#newcode474
src/parser.cc:474:
scope->SetSourceBegStatementPos(parser_->scanner().location().beg_pos);
I'm leery of setting this implicitly.  It's too easy for someone who just wants
to modify the parser to misunderstand what's going on.

If we commit this, we must have cctests that will break if the wrong positions
are recorded.

It's a bit less convenient, but better, to have explicit calls in the parser to
set the positions.

http://codereview.chromium.org/7979001/diff/1/src/parser.cc#newcode3889
src/parser.cc:3889: scope->SetSourceBegStatementPos(start_pos);
It's a bit warty that some scopes have positions set explicitly and some
implicitly.

I recommend always doing it explicitly as described above.

If it should be implicit, then it should also be that way here (e.g., by having
the LexicalScope constructor and destructor set the start and end positions).

This code can also be improved.  Since the scope records start/end positions, we
don't really need redundant local variables for them.

We also might get away with not passing them as extra parameters to
FunctionLiteral (or even storing them in FunctionLiteral) since they're
redundantly on the scope.

http://codereview.chromium.org/7979001/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7979001/diff/1/src/runtime.cc#newcode10934
src/runtime.cc:10934: enum ScopeType {
Can this be the same as Scope::Type?

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc
File src/scopeinfo.cc (right):

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode164
src/scopeinfo.cc:164: // - calls eval boolean flag
This is a separate change, but this class should be cleaned up.

(1) The encoding should be represent in code (with something other than the
magic number below), and not just in this comment.  I suggest named constants
like all the other HeapObject layouts.

(2) It's a small improvement, but we can pack the flags and type into a single
pointer.

(3) The streaming interface of the Read/Write functions is just clumsy and
doesn't really seem necessary.  A straight random access interface makes it more
clear what's going on, I think.

(4) Related to 3, it's probably simplest to have a fixed part up front, and all
the variable sized stuff at the end.  This should give the same performance with
a much simpler layout and implementation.

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode262
src/scopeinfo.cc:262: ScopeInfo<Allocator>::ScopeInfo(SerializedScopeInfo* data)
Also, for future refactoring.  Do we need the ability to construct a ScopeInfo
from a SerializedScopeInfo?  (Do we need ScopeInfo at all?)

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode462
src/scopeinfo.cc:462: p = ReadInt(p, &type);
Future refactoring: if you decide to keep the streaming interface, the default
for Read is wrong.  It should be:

Scope::Type type = ReadInt(&p);

So you can just write here:

return ReadInt(&p);

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode475
src/scopeinfo.cc:475: return -1;
-1 is RelocInfo::kNoPosition?

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h
File src/scopeinfo.h (right):

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode34
src/scopeinfo.h:34: #include "scopes.h"
Maybe we should avoid this?  It's not yet a circularity, but it risks one and
they cause us a lot of headaches.  The Scope and ScopeInfo are almost
independent except for this enum.  I think I'd rather have a ScopeType with all
the other enums in v8globals.h so the classes are not coupled (unless you can
convince me otherwise :))

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode39
src/scopeinfo.h:39: // Scope information represents information about a
functions's
This comment seems out of date.  And the Historical note is unnecessary (and not
entirely true).

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode132
src/scopeinfo.h:132: // Start position of this scope in the source string.
I think we want a crisp characterization of start and end positions, either as a
comment in the scopes or in the parser where we set it, or both.  Is it the
position of the curly brace?  Of the first/last character inside the curlies?

http://codereview.chromium.org/7979001/diff/1/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/7979001/diff/1/src/scopes.cc#newcode215
src/scopes.cc:215: source_beg_statement_pos_ = RelocInfo::kNoPosition;
You could consider setting positions for all scopes.  On the one hand, and
invalid position clearly says we don't care and is better than an incorrect one.

On the other hand, it could be useful to have in general (e.g., to remove the
redundant positions from function literals and so forth).

http://codereview.chromium.org/7979001/diff/1/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode210
src/scopes.h:210: int SourceBegStatementPos() const { return
source_beg_statement_pos_; }
I don't like the name :)

First, it doesn't follow the hacker_style naming convention for simple accessors
from the style guide, and it doesn't follow the API naming convention for the
rest of the class.

Second, abbreviations like "beg" and "pos" don't read very well.  And "Source"
and "Statement" are just noise words, though should be implicit unless there's
some other kind of positions we care about.

Suggestion:

int start_position() const { return start_position_; }

http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode216
src/scopes.h:216: int SourceEndStatementPos() const { return
source_end_statement_pos_; }
Likewise:

int end_position() const { return end_position_; }

http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode289
src/scopes.h:289: ZoneList<Scope*>* InnerScopes() { return &inner_scopes_; }
ZoneList<Scope*>* inner_scopes() { return &inner_scopes_; }

[Steven, 2011-10-06 19:09:27]

Thanks for the thorough review. I addressed all of your comments except for
refactorings that should be done in another CL. I'm a bit concerned about the
memory usage though. This change now stores the complete scope tree of a
function even though this information is only needed when debugging. And there
we don't care about performance anyway. So instead I would really consider what
we talked about earlier: reparsing the source and redoing the variable
resolution + allocation to recreate the scope tree. What do you think?

http://codereview.chromium.org/7979001/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7979001/diff/1/src/parser.cc#newcode474
src/parser.cc:474:
scope->SetSourceBegStatementPos(parser_->scanner().location().beg_pos);
Doing it explicitly now as it also gives me more control over which positions I
actually record. Also added a cctest for that.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> I'm leery of setting this implicitly.  It's too easy for someone who just
wants
> to modify the parser to misunderstand what's going on.
> 
> If we commit this, we must have cctests that will break if the wrong positions
> are recorded.
> 
> It's a bit less convenient, but better, to have explicit calls in the parser
to
> set the positions.

http://codereview.chromium.org/7979001/diff/1/src/parser.cc#newcode3889
src/parser.cc:3889: scope->SetSourceBegStatementPos(start_pos);
Removed the local variables and removed them from FunctionLiteral.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> It's a bit warty that some scopes have positions set explicitly and some
> implicitly.
> 
> I recommend always doing it explicitly as described above.
> 
> If it should be implicit, then it should also be that way here (e.g., by
having
> the LexicalScope constructor and destructor set the start and end positions).
> 
> This code can also be improved.  Since the scope records start/end positions,
we
> don't really need redundant local variables for them.
> 
> We also might get away with not passing them as extra parameters to
> FunctionLiteral (or even storing them in FunctionLiteral) since they're
> redundantly on the scope.

http://codereview.chromium.org/7979001/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7979001/diff/1/src/runtime.cc#newcode10934
src/runtime.cc:10934: enum ScopeType {
Almost. The ScopeIterator so far makes a difference about the innermost function
scope (ScopeTypeLocal) and outer function scopes (ScopeTypeClosure). This would
have to be unified if you want to use Scope::Type. What do you think?

On 2011/10/05 08:43:36, Kevin Millikin wrote:
> Can this be the same as Scope::Type?

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc
File src/scopeinfo.cc (right):

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode164
src/scopeinfo.cc:164: // - calls eval boolean flag
Yes I agree. We will talk about this cleanup later again.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> This is a separate change, but this class should be cleaned up.
> 
> (1) The encoding should be represent in code (with something other than the
> magic number below), and not just in this comment.  I suggest named constants
> like all the other HeapObject layouts.
> 
> (2) It's a small improvement, but we can pack the flags and type into a single
> pointer.
> 
> (3) The streaming interface of the Read/Write functions is just clumsy and
> doesn't really seem necessary.  A straight random access interface makes it
more
> clear what's going on, I think.
> 
> (4) Related to 3, it's probably simplest to have a fixed part up front, and
all
> the variable sized stuff at the end.  This should give the same performance
with
> a much simpler layout and implementation.

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode262
src/scopeinfo.cc:262: ScopeInfo<Allocator>::ScopeInfo(SerializedScopeInfo* data)
No we can unify those classes, they only differ slightly in the functionality
they expose.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> Also, for future refactoring.  Do we need the ability to construct a ScopeInfo
> from a SerializedScopeInfo?  (Do we need ScopeInfo at all?)

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.cc#newcode475
src/scopeinfo.cc:475: return -1;
Yes it is. Using the named constant now.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> -1 is RelocInfo::kNoPosition?

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h
File src/scopeinfo.h (right):

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode34
src/scopeinfo.h:34: #include "scopes.h"
Yes, I totally agree.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> Maybe we should avoid this?  It's not yet a circularity, but it risks one and
> they cause us a lot of headaches.  The Scope and ScopeInfo are almost
> independent except for this enum.  I think I'd rather have a ScopeType with
all
> the other enums in v8globals.h so the classes are not coupled (unless you can
> convince me otherwise :))

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode39
src/scopeinfo.h:39: // Scope information represents information about a
functions's
Done. But could use more love when cleaning up ScopeInfo/SerializedScopeInfo.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> This comment seems out of date.  And the Historical note is unnecessary (and
not
> entirely true).

http://codereview.chromium.org/7979001/diff/1/src/scopeinfo.h#newcode132
src/scopeinfo.h:132: // Start position of this scope in the source string.
Added a description to scopes.h. PTAL there.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> I think we want a crisp characterization of start and end positions, either as
a
> comment in the scopes or in the parser where we set it, or both.  Is it the
> position of the curly brace?  Of the first/last character inside the curlies?

http://codereview.chromium.org/7979001/diff/1/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/7979001/diff/1/src/scopes.cc#newcode215
src/scopes.cc:215: source_beg_statement_pos_ = RelocInfo::kNoPosition;
It is set for all scopes during initial parsing, but not necessarily when
deserializing the outer scope chain. There we only get a SerializedScopeInfo for
function and block scopes, but not for with or catch scopes.
I removed the redundant positions from function literal. It now gets the
positions from the scope and which are always valid.
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> You could consider setting positions for all scopes.  On the one hand, and
> invalid position clearly says we don't care and is better than an incorrect
one.
> 
> On the other hand, it could be useful to have in general (e.g., to remove the
> redundant positions from function literals and so forth).

http://codereview.chromium.org/7979001/diff/1/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode210
src/scopes.h:210: int SourceBegStatementPos() const { return
source_beg_statement_pos_; }
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> I don't like the name :)
> 
> First, it doesn't follow the hacker_style naming convention for simple
accessors
> from the style guide, and it doesn't follow the API naming convention for the
> rest of the class.
> 
> Second, abbreviations like "beg" and "pos" don't read very well.  And "Source"
> and "Statement" are just noise words, though should be implicit unless there's
> some other kind of positions we care about.
> 
> Suggestion:
> 
> int start_position() const { return start_position_; }

Done.

http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode216
src/scopes.h:216: int SourceEndStatementPos() const { return
source_end_statement_pos_; }
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> Likewise:
> 
> int end_position() const { return end_position_; }

Done.

http://codereview.chromium.org/7979001/diff/1/src/scopes.h#newcode289
src/scopes.h:289: ZoneList<Scope*>* InnerScopes() { return &inner_scopes_; }
On 2011/10/05 08:43:36, Kevin Millikin wrote:
> ZoneList<Scope*>* inner_scopes() { return &inner_scopes_; }

Done.

[Kevin Millikin (Chromium), 2011-10-13 14:34:11]

On 2011/10/06 19:09:27, Steven wrote:
> Thanks for the thorough review. I addressed all of your comments except for
> refactorings that should be done in another CL. I'm a bit concerned about the
> memory usage though. This change now stores the complete scope tree of a
> function even though this information is only needed when debugging. And there
> we don't care about performance anyway. So instead I would really consider
what
> we talked about earlier: reparsing the source and redoing the variable
> resolution + allocation to recreate the scope tree. What do you think?

I think you've convinced me (to reparse and reconstruct scopes).  It also has
the advantage of being quite local.

[Steven, 2011-10-14 15:11:37]

Now reparsing the code. And I removed the scope tree and the source positions
from the SerializedScopeInfo again. Please take another look.

[Steven, 2011-10-17 15:18:02]

Rebased this to tip of tree. Please take a look again.

New are block scopes which are introduced for for-loops with let bound iteration
variables, so you might want to look more carefully there.

[Kevin Millikin (Chromium), 2011-10-18 08:10:24]

Very nice.  LGTM.