Separate Cell and PropertyCell spaces

Separate Cell and PropertyCell spaces

This makes it possible to store additional information on property cells, for example Type and optimized Code dependencies.

R=mstarzinger@chromium.org

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

[danno, 2013-06-07 15:39:52]

PTAL

[Michael Starzinger, 2013-06-11 17:37:34]

One high-level thing: I know it is probably tedious to change, but would it be
possible to name the newly added space "property_cell_space" or even shorter
just "property_space"?

[Michael Starzinger, 2013-06-12 13:23:25]

Detailed comments and nits.

https://codereview.chromium.org/16631002/diff/12001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:7352: Handle<Map> cell_map(
nit: Should fit into one line now. Also, better use ...

Handle<Map> cell_map = masm->isolate()->factory()->cell_map();

https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc...
src/arm/stub-cache-arm.cc:2860: __ ldr(scratch3(),
nit: Should fit into one line now.

https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc...
src/arm/stub-cache-arm.cc:2866: __ str(value(),
nit: Should fit into one line now.

https://codereview.chromium.org/16631002/diff/12001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/16631002/diff/12001/src/assembler.h#newcode257
src/assembler.h:257: GLOBAL_PROPERTY_CELL,
Can we also rename this enum constant to "CELL"?

https://codereview.chromium.org/16631002/diff/12001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/heap.cc#newcode392
src/heap.cc:392: PrintPID("JSGlobalPropertyCell space,         used: %6"
V8_PTR_PREFIX "d KB"
Can we align this printf() format string so that it is aligned with the others?
Otherwise the printout of --trace-gc-verbose looks ugly. This should be easily
possible after the space has been renamed.

https://codereview.chromium.org/16631002/diff/12001/src/heap.cc#newcode6815
src/heap.cc:6815: new JSGlobalPropertyCellSpace(this, max_old_generation_size_,
CELL_SPACE);
The CELL_SPACE allocation id here is bogus.

https://codereview.chromium.org/16631002/diff/12001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:7935: Handle<Map>
cell_map(masm->isolate()->heap()->cell_map());
See nit on other architecture.

https://codereview.chromium.org/16631002/diff/12001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/mark-compact.cc#newco...
src/mark-compact.cc:3399: (JSGlobalPropertyCell::kValueOffset - kHeapObjectTag);
Even though the values overlap I would recommend to use Cell::kValueOffset here.

https://codereview.chromium.org/16631002/diff/12001/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/16631002/diff/12001/src/objects-visiting-inl....
src/objects-visiting-inl.h:248: void
StaticMarkingVisitor<StaticVisitor>::VisitGlobalPropertyCell(
I know it is probably a bit of additional work, but could we rename these static
visitor methods to VisitCell as well? Alternatively we can also do this in a
follow-up CL, in which case we should put in a TODO here or somewhere in this
file.

https://codereview.chromium.org/16631002/diff/12001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/objects.cc#newcode1574
src/objects.cc:1574: accumulator->Add("JsGlobalPropertyCell for ");
nit: Let's at least drop the "Js" prefix in this printed string.

https://codereview.chromium.org/16631002/diff/12001/src/objects.h
File src/objects.h (left):

https://codereview.chromium.org/16631002/diff/12001/src/objects.h#oldcode8562
src/objects.h:8562: // [value]: value of the global property.
I actually like the comment lines and sometimes grep for "[foo]" in the file,
can I get the comment line back? Or I could just learn to grep for
"DECL_ACCESSORS" but unfortunately we don't have that macro for int accessors.

https://codereview.chromium.org/16631002/diff/12001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode351
src/objects.h:351: V(CELL_TYPE)                                                 
               \
Can we add the "Cell" type to the type hierarchy ASCII-art at the top if this
file?

https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode8602
src/objects.h:8602: void set_type(Type* value, WriteBarrierMode mode =
UPDATE_WRITE_BARRIER);
Can we use the DECL_ACCESSORS macro to declare this pair here?

https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode8620
src/objects.h:8620: kValueOffset,
nit: Should fit into three lines if we remove the first line-break.

https://codereview.chromium.org/16631002/diff/12001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:6940: masm->isolate()->heap()->cell_map());
See nit for other architecture.

[danno, 2013-06-12 14:24:27]

Please take another look.

https://codereview.chromium.org/16631002/diff/12001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:7352: Handle<Map> cell_map(
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> nit: Should fit into one line now. Also, better use ...
> 
> Handle<Map> cell_map = masm->isolate()->factory()->cell_map();

Done.

https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc...
src/arm/stub-cache-arm.cc:2860: __ ldr(scratch3(),
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> nit: Should fit into one line now.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc...
src/arm/stub-cache-arm.cc:2866: __ str(value(),
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> nit: Should fit into one line now.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/16631002/diff/12001/src/assembler.h#newcode257
src/assembler.h:257: GLOBAL_PROPERTY_CELL,
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> Can we also rename this enum constant to "CELL"?

Done.

https://codereview.chromium.org/16631002/diff/12001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/heap.cc#newcode392
src/heap.cc:392: PrintPID("JSGlobalPropertyCell space,         used: %6"
V8_PTR_PREFIX "d KB"
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> Can we align this printf() format string so that it is aligned with the
others?
> Otherwise the printout of --trace-gc-verbose looks ugly. This should be easily
> possible after the space has been renamed.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/heap.cc#newcode6815
src/heap.cc:6815: new JSGlobalPropertyCellSpace(this, max_old_generation_size_,
CELL_SPACE);
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> The CELL_SPACE allocation id here is bogus.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:7935: Handle<Map>
cell_map(masm->isolate()->heap()->cell_map());
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> See nit on other architecture.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/mark-compact.cc#newco...
src/mark-compact.cc:3399: (JSGlobalPropertyCell::kValueOffset - kHeapObjectTag);
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> Even though the values overlap I would recommend to use Cell::kValueOffset
here.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/16631002/diff/12001/src/objects-visiting-inl....
src/objects-visiting-inl.h:248: void
StaticMarkingVisitor<StaticVisitor>::VisitGlobalPropertyCell(
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> I know it is probably a bit of additional work, but could we rename these
static
> visitor methods to VisitCell as well? Alternatively we can also do this in a
> follow-up CL, in which case we should put in a TODO here or somewhere in this
> file.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/objects.cc#newcode1574
src/objects.cc:1574: accumulator->Add("JsGlobalPropertyCell for ");
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> nit: Let's at least drop the "Js" prefix in this printed string.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/objects.h
File src/objects.h (left):

https://codereview.chromium.org/16631002/diff/12001/src/objects.h#oldcode8562
src/objects.h:8562: // [value]: value of the global property.
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> I actually like the comment lines and sometimes grep for "[foo]" in the file,
> can I get the comment line back? Or I could just learn to grep for
> "DECL_ACCESSORS" but unfortunately we don't have that macro for int accessors.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode351
src/objects.h:351: V(CELL_TYPE)                                                 
               \
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> Can we add the "Cell" type to the type hierarchy ASCII-art at the top if this
> file?

Done.

https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode8602
src/objects.h:8602: void set_type(Type* value, WriteBarrierMode mode =
UPDATE_WRITE_BARRIER);
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> Can we use the DECL_ACCESSORS macro to declare this pair here?

No, unfortunately not. Note that they are not inlined. The problem is that we
would have type include type.h in objects-inl.h, and that's not possible due to
circular dependencies.

https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode8620
src/objects.h:8620: kValueOffset,
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> nit: Should fit into three lines if we remove the first line-break.

Done.

https://codereview.chromium.org/16631002/diff/12001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/16631002/diff/12001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:6940: masm->isolate()->heap()->cell_map());
On 2013/06/12 13:23:25, Michael Starzinger wrote:
> See nit for other architecture.

Done.

[Michael Starzinger, 2013-06-12 14:34:30]

LGTM.

[danno, 2013-06-12 15:04:18]

Committed patchset #9 manually as r15089 (presubmit successful).