Cleanup ScopeInfo and SerializedScopeInfo.

Cleanup ScopeInfo and SerializedScopeInfo.

Both classes have been merged into a single ScopeInfo
class that implements the functionality from both.

This CL does not adapt the broken gdb-jit interface.

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

[Steven, 2011-10-20 08:38:03]

PTAL.

[Kevin Millikin (Chromium), 2011-11-02 16:31:19]

My comments are mainly about the style of objects.h and about the use of handles
in the ScopeInfo API.

http://codereview.chromium.org/8352039/diff/8001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/8352039/diff/8001/src/heap.cc#newcode4417
src/heap.cc:4417: maybe_result->ToObject(&result);
No need to call ToObject twice, is there?

This can be more simply be written:

ScopeInfo* scope_info;
MaybeObject* maybe_result = AllocateFixedArray(length, TENURED);
if (!maybe_result->To(&scope_info)) return maybe_result;
scope_info->set_map(scope_info_map());
return scope_info;

http://codereview.chromium.org/8352039/diff/8001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3110
src/objects.h:3110: 
Presubmit check is not going to like this extra blank line.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3111
src/objects.h:3111: static ScopeInfo* cast(Object* object) {
We normally do this this way:

in objects.h:

static inline ScopeInfo* cast(Object* object);

and in objects-inl.h:

CAST_ACCESSOR(ScopeInfo)

(I actually would support moving CAST_ACCESSOR into objects.h and just using it
in the class body, but that's a separate change.)

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3147
src/objects.h:3147: int NumberOfContextSlots();
I've always found it a bit confusing to count the context header as "slots". 
Perhaps we should make this so it does not count the size of the header; or else
count the header size and rename it to something like ContextLength.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3159
src/objects.h:3159: Handle<String> function_name();
There's a strange mix of hacker_style and CamelCase function names in here.  I
can't figure out why, that's distracting, so maybe we should adopt one or the
other (hopefully CamelCase).

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3187
src/objects.h:3187: int ContextSlotIndex(String* name, VariableMode* mode =
NULL);
I'm not thrilled with the default parameter.  It does seem to fit the allowable
exception (being used to simulate variable length argument lists), but it also
seems unnecessary.

If you make the caller always pass it, it's only inconvenient at a handful of
sites, and then the lookup functions don't have to check for mode != NULL.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3209
src/objects.h:3209: #define SCOPE_INFO_NUMERIC_FIELDS(V)                 \
OK to use a macro to generate these, but there's no need for the higher-order
macro (which isn't undef'd here, kind of scary).  It isn't much worse to have

#define FIELD_ACCESSORS(index, name) \
...
FIELD_ACCESSORS(FLAGS, flags)
FIELD_ACCESSORS(NUM_PARAMETERS, num_parameters)
...
#undef FIELD_ACCESSORS

Where I've taken advantage of the default being 0 for all of them and of the
ability to generate the _INDEX part of the name in the macro.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3230
src/objects.h:3230: enum {
Or, you could keep the higher-order macro, and use it here.  I'd rename it and
also rename the enum tags to fit the current style guide:

enum {
#define DECL_INDEX(index, name) k##index,
  FOR_EACH_NUMERIC_FIELD(DECL_INDEX)
#undef DECL_INDEX
  kVariablePartIndex
};

And you might as well make this enum private (since it's not named it can't
really be part of an signature anyway), and also FunctionVariableInfo, and all
the classes defining the bit fields.

http://codereview.chromium.org/8352039/diff/8001/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/8352039/diff/8001/src/profile-generator.cc#new...
src/profile-generator.cc:2072: String* local_name =
*scope_info->context_local_name(i);
Not your code, but while you're cleaning this up I wonder if we should change
the API of the ScopeInfo so that it doesn't use handles.

I don't like handle scopes in raw pointer code if we can avoid them.  From a
quick inspection, it seems like almost all of the callers of local_name,
function_name, etc. just dereference the newly-created handle immediately.

If that's so, it's way, way better to have a raw pointer return type and make
the handful of callers that want a handle have to create one.

Then we can probably get rid of some silly handle scopes, like the one in this
function.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode49
src/scopeinfo.cc:49: Handle<ScopeInfo> ScopeInfo::Create(Scope* scope) {
Now that you've removed the template parameter, I think you can make some other
functions non-templated too (Scope::CollectUsedVariables is the one I'm thinking
of).

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode50
src/scopeinfo.cc:50: ZoneList<Variable*> variables(32);  // 32 is a wild guess
Again, not your code, but the "wild guess" here seems silly to me.  I'd bet that
most variables are used, so we'd get a both a tighter upper bound, that also
guaranteed no need to grow the list, if we just used the scope's number of
variables (parameters, locals, and temps).

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode60
src/scopeinfo.cc:60: if (!var->is_used()) continue;
It's not clear how "CollectUsedVariables" can collect one that's unused.  Do you
have any idea?

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode189
src/scopeinfo.cc:189: bool ScopeInfo::CallsEval() {
I like

return length() > 0 && CallsEvalField::decode(flags());

but maybe you don't.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode214
src/scopeinfo.cc:214: bool function_name_stack_slot = STACK ==
Hmmm, I could see this requiring two tries to read it correctly.  Break at the
lower precedence operator:

bool function_name_stack_slot =
  STACK == FunctionVariableField::decode(flags());

I sort of like STACK on the right.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode225
src/scopeinfo.cc:225: bool function_name_context_slot = CONTEXT ==
Same comment as just above.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.h
File src/scopeinfo.h (left):

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.h#oldcode38
src/scopeinfo.h:38: // ScopeInfo represents information about different scopes
of a source
It's not very nice to make objects.cc bigger than it is, but at some point we
should move the ScopeInfo implementation into objects.cc because it's declared
in objects.h; and at the same time rename this file context-slot-cache.h.

[Steven, 2011-11-02 18:52:59]

http://codereview.chromium.org/8352039/diff/8001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/8352039/diff/8001/src/heap.cc#newcode4417
src/heap.cc:4417: maybe_result->ToObject(&result);
Removed the second ToObject call. However I kept the reinterpret_cast because
MaybeObject::To calls ScopeInfo::cast which in turn asserts that the object in
question has a scope_info_map which this one does not yet have.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> No need to call ToObject twice, is there?
> 
> This can be more simply be written:
> 
> ScopeInfo* scope_info;
> MaybeObject* maybe_result = AllocateFixedArray(length, TENURED);
> if (!maybe_result->To(&scope_info)) return maybe_result;
> scope_info->set_map(scope_info_map());
> return scope_info;

http://codereview.chromium.org/8352039/diff/8001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3110
src/objects.h:3110: 
tools/presubmit.py didn't complain. Maybe it should?
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> Presubmit check is not going to like this extra blank line.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3111
src/objects.h:3111: static ScopeInfo* cast(Object* object) {
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> We normally do this this way:
> 
> in objects.h:
> 
> static inline ScopeInfo* cast(Object* object);
> 
> and in objects-inl.h:
> 
> CAST_ACCESSOR(ScopeInfo)
> 
> (I actually would support moving CAST_ACCESSOR into objects.h and just using
it
> in the class body, but that's a separate change.)

Done.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3147
src/objects.h:3147: int NumberOfContextSlots();
Renamed it to ContextLength.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> I've always found it a bit confusing to count the context header as "slots". 
> Perhaps we should make this so it does not count the size of the header; or
else
> count the header size and rename it to something like ContextLength.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3159
src/objects.h:3159: Handle<String> function_name();
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> There's a strange mix of hacker_style and CamelCase function names in here.  I
> can't figure out why, that's distracting, so maybe we should adopt one or the
> other (hopefully CamelCase).

Done.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3187
src/objects.h:3187: int ContextSlotIndex(String* name, VariableMode* mode =
NULL);
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> I'm not thrilled with the default parameter.  It does seem to fit the
allowable
> exception (being used to simulate variable length argument lists), but it also
> seems unnecessary.
> 
> If you make the caller always pass it, it's only inconvenient at a handful of
> sites, and then the lookup functions don't have to check for mode != NULL.

Done.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3209
src/objects.h:3209: #define SCOPE_INFO_NUMERIC_FIELDS(V)                 \
Kept the higher order macro but now it generates the Index part of the name and
the default value 0. It's also undef'd now :)

On 2011/11/02 16:31:19, Kevin Millikin wrote:
> OK to use a macro to generate these, but there's no need for the higher-order
> macro (which isn't undef'd here, kind of scary).  It isn't much worse to have
> 
> #define FIELD_ACCESSORS(index, name) \
> ...
> FIELD_ACCESSORS(FLAGS, flags)
> FIELD_ACCESSORS(NUM_PARAMETERS, num_parameters)
> ...
> #undef FIELD_ACCESSORS
> 
> Where I've taken advantage of the default being 0 for all of them and of the
> ability to generate the _INDEX part of the name in the macro.

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3230
src/objects.h:3230: enum {
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> Or, you could keep the higher-order macro, and use it here.  I'd rename it and
> also rename the enum tags to fit the current style guide:
> 
> enum {
> #define DECL_INDEX(index, name) k##index,
>   FOR_EACH_NUMERIC_FIELD(DECL_INDEX)
> #undef DECL_INDEX
>   kVariablePartIndex
> };
> 
> And you might as well make this enum private (since it's not named it can't
> really be part of an signature anyway), and also FunctionVariableInfo, and all
> the classes defining the bit fields.

Done.

http://codereview.chromium.org/8352039/diff/8001/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/8352039/diff/8001/src/profile-generator.cc#new...
src/profile-generator.cc:2072: String* local_name =
*scope_info->context_local_name(i);
Done. However AFAICS this is the only HandleScope that can be removed. But you
might want to check as well.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> Not your code, but while you're cleaning this up I wonder if we should change
> the API of the ScopeInfo so that it doesn't use handles.
> 
> I don't like handle scopes in raw pointer code if we can avoid them.  From a
> quick inspection, it seems like almost all of the callers of local_name,
> function_name, etc. just dereference the newly-created handle immediately.
> 
> If that's so, it's way, way better to have a raw pointer return type and make
> the handful of callers that want a handle have to create one.
> 
> Then we can probably get rid of some silly handle scopes, like the one in this
> function.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode49
src/scopeinfo.cc:49: Handle<ScopeInfo> ScopeInfo::Create(Scope* scope) {
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> Now that you've removed the template parameter, I think you can make some
other
> functions non-templated too (Scope::CollectUsedVariables is the one I'm
thinking
> of).

Done.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode50
src/scopeinfo.cc:50: ZoneList<Variable*> variables(32);  // 32 is a wild guess
Will do this in a new CL. Here we are only interested in stack and context
locals. The only other client is
is FunctionInfoListener::SerializeFunctionScope in liveedit.cc which is only
interested in the context locals. So we can do even better here.

On 2011/11/02 16:31:19, Kevin Millikin wrote:
> Again, not your code, but the "wild guess" here seems silly to me.  I'd bet
that
> most variables are used, so we'd get a both a tighter upper bound, that also
> guaranteed no need to grow the list, if we just used the scope's number of
> variables (parameters, locals, and temps).

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode60
src/scopeinfo.cc:60: if (!var->is_used()) continue;
A bit flip because of faulty hardware :P
Done.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> It's not clear how "CollectUsedVariables" can collect one that's unused.  Do
you
> have any idea?

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode189
src/scopeinfo.cc:189: bool ScopeInfo::CallsEval() {
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> I like
> 
> return length() > 0 && CallsEvalField::decode(flags());
> 
> but maybe you don't.

Done.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode214
src/scopeinfo.cc:214: bool function_name_stack_slot = STACK ==
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> Hmmm, I could see this requiring two tries to read it correctly.  Break at the
> lower precedence operator:
> 
> bool function_name_stack_slot =
>   STACK == FunctionVariableField::decode(flags());
> 
> I sort of like STACK on the right.

Done.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.cc#newcode225
src/scopeinfo.cc:225: bool function_name_context_slot = CONTEXT ==
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> Same comment as just above.

Done.

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.h
File src/scopeinfo.h (left):

http://codereview.chromium.org/8352039/diff/8001/src/scopeinfo.h#oldcode38
src/scopeinfo.h:38: // ScopeInfo represents information about different scopes
of a source
I keep that for a CL on its own, because I have other pending CLs that change
scopeinfo.cc.
On 2011/11/02 16:31:19, Kevin Millikin wrote:
> It's not very nice to make objects.cc bigger than it is, but at some point we
> should move the ScopeInfo implementation into objects.cc because it's declared
> in objects.h; and at the same time rename this file context-slot-cache.h.

http://codereview.chromium.org/8352039/diff/13001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8352039/diff/13001/src/runtime.cc#newcode10795
src/runtime.cc:10795: String* name = scope_info->LocalName(i);
Shall I keep the handle here, to be consistent in handlified code?

[Kevin Millikin (Chromium), 2011-11-03 09:18:37]

LGTM with a few small comments.

http://codereview.chromium.org/8352039/diff/8001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/8352039/diff/8001/src/heap.cc#newcode4417
src/heap.cc:4417: maybe_result->ToObject(&result);
I see what you mean, that's less useful in raw allocation functions.  Still,
nothing needs specifically a ScopeInfo*:

FixedArray* scope_info;
MaybeObject* maybe_scope_info = AllocateFixedArray(length, TENURED);
if (!maybe_scope_info->To(&scope_info)) return maybe_scope_info;
scope_info->set_map(scope_info_map());
return scope_info;

http://codereview.chromium.org/8352039/diff/8001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8352039/diff/8001/src/objects.h#newcode3110
src/objects.h:3110: 
I'm pretty sure the one from Chromium's depot_tools will complain.  That's the
one that's run on the buildbots.

http://codereview.chromium.org/8352039/diff/13001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8352039/diff/13001/src/runtime.cc#newcode10795
src/runtime.cc:10795: String* name = scope_info->LocalName(i);
On 2011/11/02 18:52:59, Steven wrote:
> Shall I keep the handle here, to be consistent in handlified code?

I think so, even though it allocates a handle per loop iteration.

http://codereview.chromium.org/8352039/diff/9027/src/frames.cc
File src/frames.cc (right):

http://codereview.chromium.org/8352039/diff/9027/src/frames.cc#newcode1045
src/frames.cc:1045: if (i < scope_info->NumParameters()) {
OK, but "Num" grinds on me in identifier names, because it reads badly. 
ParameterCount is only one character longer.

[Steven, 2011-11-03 10:32:34]

http://codereview.chromium.org/8352039/diff/8001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/8352039/diff/8001/src/heap.cc#newcode4417
src/heap.cc:4417: maybe_result->ToObject(&result);
Ok I'm convinced. Done.
On 2011/11/03 09:18:37, Kevin Millikin wrote:
> I see what you mean, that's less useful in raw allocation functions.  Still,
> nothing needs specifically a ScopeInfo*:
> 
> FixedArray* scope_info;
> MaybeObject* maybe_scope_info = AllocateFixedArray(length, TENURED);
> if (!maybe_scope_info->To(&scope_info)) return maybe_scope_info;
> scope_info->set_map(scope_info_map());
> return scope_info;

http://codereview.chromium.org/8352039/diff/13001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8352039/diff/13001/src/runtime.cc#newcode10795
src/runtime.cc:10795: String* name = scope_info->LocalName(i);
On 2011/11/03 09:18:37, Kevin Millikin wrote:
> On 2011/11/02 18:52:59, Steven wrote:
> > Shall I keep the handle here, to be consistent in handlified code?
> 
> I think so, even though it allocates a handle per loop iteration.

Done.

http://codereview.chromium.org/8352039/diff/9027/src/frames.cc
File src/frames.cc (right):

http://codereview.chromium.org/8352039/diff/9027/src/frames.cc#newcode1045
src/frames.cc:1045: if (i < scope_info->NumParameters()) {
Yeah we talked about this earlier. Also renamed the other functions.
On 2011/11/03 09:18:38, Kevin Millikin wrote:
> OK, but "Num" grinds on me in identifier names, because it reads badly. 
> ParameterCount is only one character longer.