Move serialized scope info from Code object to SharedFunctionInfo.

Move serialized scope info from Code object to SharedFunctionInfo.

The scope info is now stored in a FixedArray referenced from SharedFunctionInfo.



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

[Vitaly Repeshko, 2010-07-08 13:23:17]

I think it'd be nice to (later) create real ScopeInfoObject inheriting from
FixedArray and move the quick accessors (CallsEval, NumberOfStackSlots, etc.)
there.


-- Vitaly

http://codereview.chromium.org/2918001/diff/1/21
File src/compiler.cc (right):

http://codereview.chromium.org/2918001/diff/1/21#newcode157
src/compiler.cc:157: static Handle<Object> CreateScopeInfo(CompilationInfo*
info) {
This should only receive info->scope(). Also consider making it a static
function in ScopeInfo.

http://codereview.chromium.org/2918001/diff/1/26
File src/frames.cc (right):

http://codereview.chromium.org/2918001/diff/1/26#newcode535
src/frames.cc:535: Handle<Object> scope_info(Heap::empty_fixed_array());
ScopeInfo::EmptyObject()

http://codereview.chromium.org/2918001/diff/1/7
File src/heap.cc (right):

http://codereview.chromium.org/2918001/diff/1/7#newcode2322
src/heap.cc:2322: if
(ScopeInfo<>::HasHeapAllocatedLocals(function_info->scope_info()))
Interesting... This means we could actually flush the code object while
retaining its scope info. IIRC, Rico added this check later in a follow-up fix
to his code flushing change, so he should have the numbers on how much we can
save this way. If the savings are significant, we should consider whether to
implement this separation now or to file a bug and implement it later.

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

http://codereview.chromium.org/2918001/diff/1/32#newcode159
src/scopeinfo.cc:159: // - NULL (sentinel)
While we're at it, these sentinels look like a waste. We don't seem to *really*
depend on their presence and that's 12 bytes per JS function.

http://codereview.chromium.org/2918001/diff/1/32#newcode334
src/scopeinfo.cc:334: Object* data = *Factory::NewFixedArray(length, TENURED);
Either use Factory and handles or Heap and raw pointers. Factory methods can
cause garbage collection. Heap methods can return a failure and it should be
propagated and handled by the callers of ScopeInfo::Serialize(). Since this
happens during compilation I think using Factory and handles is preferred. Still
you don't have to use handles in the write functions. Just add
AssertNoAllocation.

Handle<Object> ScopeInfo::Serialize() {
  const int extra_slots = ...;
  int length = ...;
  Handle<Object> data = Factory::New...();
  AssertNoAllocation nogc;
  Object** p0 = GetDataStart(data)
  Object** p = p0;
  // ... Write ...
  return data;
}

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

http://codereview.chromium.org/2918001/diff/1/15#newcode61
src/scopeinfo.h:61: Object* Serialize();
Serialize() returning an Object pointer looks a bit weird. Maybe ToObject()?

If you agree, please rephrase all other mentions of serialization here.

http://codereview.chromium.org/2918001/diff/1/15#newcode64
src/scopeinfo.h:64: static Object* SerializeEmpty();
EmptyObject()?

[Vladislav Kaznacheev, 2010-07-08 14:31:42]

http://codereview.chromium.org/2918001/diff/1/21
File src/compiler.cc (right):

http://codereview.chromium.org/2918001/diff/1/21#newcode157
src/compiler.cc:157: static Handle<Object> CreateScopeInfo(CompilationInfo*
info) {
On 2010/07/08 13:23:17, Vitaly wrote:
> This should only receive info->scope(). Also consider making it a static
> function in ScopeInfo.

Moved to ScopeInfo

http://codereview.chromium.org/2918001/diff/1/26
File src/frames.cc (right):

http://codereview.chromium.org/2918001/diff/1/26#newcode535
src/frames.cc:535: Handle<Object> scope_info(Heap::empty_fixed_array());
On 2010/07/08 13:23:17, Vitaly wrote:
> ScopeInfo::EmptyObject()

Done.

http://codereview.chromium.org/2918001/diff/1/7
File src/heap.cc (right):

http://codereview.chromium.org/2918001/diff/1/7#newcode2322
src/heap.cc:2322: if
(ScopeInfo<>::HasHeapAllocatedLocals(function_info->scope_info()))
On 2010/07/08 13:23:17, Vitaly wrote:
> Interesting... This means we could actually flush the code object while
> retaining its scope info. IIRC, Rico added this check later in a follow-up fix
> to his code flushing change, so he should have the numbers on how much we can
> save this way. If the savings are significant, we should consider whether to
> implement this separation now or to file a bug and implement it later.

I will look into it right after this patch is committed.

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

http://codereview.chromium.org/2918001/diff/1/32#newcode159
src/scopeinfo.cc:159: // - NULL (sentinel)
Agree. Will address that when creating the new class for the heap object.

On 2010/07/08 13:23:17, Vitaly wrote:
> While we're at it, these sentinels look like a waste. We don't seem to
*really*
> depend on their presence and that's 12 bytes per JS function.

http://codereview.chromium.org/2918001/diff/1/32#newcode334
src/scopeinfo.cc:334: Object* data = *Factory::NewFixedArray(length, TENURED);
On 2010/07/08 13:23:17, Vitaly wrote:
> Either use Factory and handles or Heap and raw pointers. Factory methods can
> cause garbage collection. Heap methods can return a failure and it should be
> propagated and handled by the callers of ScopeInfo::Serialize(). Since this
> happens during compilation I think using Factory and handles is preferred.
Still
> you don't have to use handles in the write functions. Just add
> AssertNoAllocation.
> 
> Handle<Object> ScopeInfo::Serialize() {
>   const int extra_slots = ...;
>   int length = ...;
>   Handle<Object> data = Factory::New...();
>   AssertNoAllocation nogc;
>   Object** p0 = GetDataStart(data)
>   Object** p = p0;
>   // ... Write ...
>   return data;
> }

Done.

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

http://codereview.chromium.org/2918001/diff/1/15#newcode61
src/scopeinfo.h:61: Object* Serialize();
We agreed that I should refactor the static methods below into instance methods
of the new class (ScopeInfoObject or something like that) inheriting from
FixedArray
On 2010/07/08 13:23:17, Vitaly wrote:
> Serialize() returning an Object pointer looks a bit weird. Maybe ToObject()?
> 
> If you agree, please rephrase all other mentions of serialization here.

http://codereview.chromium.org/2918001/diff/1/15#newcode64
src/scopeinfo.h:64: static Object* SerializeEmpty();
On 2010/07/08 13:23:17, Vitaly wrote:
> EmptyObject()?

Done.

[Vitaly Repeshko, 2010-07-08 21:17:39]

LGTM

Please don't forget the postponed issues :) Maybe create a bug mentioning them
all.


Thanks
Vitaly

http://codereview.chromium.org/2918001/diff/33002/12015
File src/scopeinfo.h (right):

http://codereview.chromium.org/2918001/diff/33002/12015#newcode156
src/scopeinfo.h:156: static Handle<Object> CreateHeapObject(Scope* scope);
Since this uses ZoneScopeInfo only as a temporary storage and it's an
implementation detail really, I think CreateHeapObject and EmptyHeap object
functions should be in ScopeInfo.

[Vladislav Kaznacheev, 2010-07-09 08:12:38]

I have made just a few changes but for some reason the latest patch set seems to
think that every file changed.

http://codereview.chromium.org/2918001/diff/33002/12015
File src/scopeinfo.h (right):

http://codereview.chromium.org/2918001/diff/33002/12015#newcode156
src/scopeinfo.h:156: static Handle<Object> CreateHeapObject(Scope* scope);
On 2010/07/08 21:17:39, Vitaly wrote:
> Since this uses ZoneScopeInfo only as a temporary storage and it's an
> implementation detail really, I think CreateHeapObject and EmptyHeap object
> functions should be in ScopeInfo.

Done. Also removed ZoneScopeInfo class altogether as it was only needed for
forward declarations when it was passed to CreateCode.