Out-of-line constant pool on Arm: Stage 2 - Introduce ConstantPoolArray object.

Out-of-line constant pool on Arm: Stage 2 - Introduce ConstantPoolArray object.

Second stage of implementing an out-of-line constant pool on Arm.  This CL
Introduces the ConstantPoolArray object which will be used as the backing
store of out-of-line constant pools.  Nothing uses this object yet.

BUG=
R=ulan@chromium.org

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

[rmcilroy, 2013-08-07 17:53:30]

Hi Ulan,

I haven't done much testing on this yet, but just wanted send you a CL for the
basics of introducing the ConstantPoolArray object we talked about yesterday to
check I'm going in the right direction.  Please let me know if this looks
reasonable?

[ulan, 2013-08-09 08:52:54]

Looks good.

https://codereview.chromium.org/22601003/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/22601003/diff/1/src/heap.cc#newcode5709
src/heap.cc:5709: if (length < 0) {
ASSERT(length > 0), instead of this check?

https://codereview.chromium.org/22601003/diff/1/src/heap.cc#newcode5713
src/heap.cc:5713: AllocationSpace space = (pretenure == TENURED) ?
OLD_DATA_SPACE : NEW_SPACE;
It's probably better to pretenure, because code objects are not in new space,
and a const pool will have the same life-time as the corresponding code object.

https://codereview.chromium.org/22601003/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/22601003/diff/1/src/objects-inl.h#newcode2043
src/objects-inl.h:2043: ASSERT(map() != HEAP->fixed_cow_array_map() &&
Why not use map() == HEAP->constant_pool_array_map() ?

[rmcilroy, 2013-08-16 10:27:37]

Addressed comments.  PTAL

https://codereview.chromium.org/22601003/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/22601003/diff/1/src/heap.cc#newcode5709
src/heap.cc:5709: if (length < 0) {
On 2013/08/09 08:52:54, ulan wrote:
> ASSERT(length > 0), instead of this check?

I was following AllocateRawFixedArray which does this, but changed to ASSERT.

https://codereview.chromium.org/22601003/diff/1/src/heap.cc#newcode5713
src/heap.cc:5713: AllocationSpace space = (pretenure == TENURED) ?
OLD_DATA_SPACE : NEW_SPACE;
On 2013/08/09 08:52:54, ulan wrote:
> It's probably better to pretenure, because code objects are not in new space,
> and a const pool will have the same life-time as the corresponding code
object.

Done.

https://codereview.chromium.org/22601003/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/22601003/diff/1/src/objects-inl.h#newcode2043
src/objects-inl.h:2043: ASSERT(map() != HEAP->fixed_cow_array_map() &&
On 2013/08/09 08:52:54, ulan wrote:
> Why not use map() == HEAP->constant_pool_array_map() ?

Done.

[Rodolph Perfetta, 2013-08-16 12:22:44]

More of a side question, do you plan to handle references in the constant pool?

[rmcilroy, 2013-08-16 14:24:00]

On 2013/08/16 12:22:44, Rodolph Perfetta wrote:
> More of a side question, do you plan to handle references in the constant
pool?

Not at present.  Do we put references into the inline constant pools at present?
I didn't think we did but please correct me if I'm wrong.  Maybe in future we
could add references to this ool constant pool, but we would need to modify the
ConstantPoolArray to enable the GC to tell which values were references and
which aren't.

[Rodolph Perfetta, 2013-09-16 10:11:29]

On 2013/08/16 14:24:00, rmcilroy wrote:
> On 2013/08/16 12:22:44, Rodolph Perfetta wrote:
> > More of a side question, do you plan to handle references in the constant
> pool?
> 
> Not at present.  Do we put references into the inline constant pools at
present?
> I didn't think we did but please correct me if I'm wrong.  Maybe in future we
> could add references to this ool constant pool, but we would need to modify
the
> ConstantPoolArray to enable the GC to tell which values were references and
> which aren't.

Sorry for the late reply, your reply didn't show up in my inbox.

Yes, reference can be stored in the literal pool, the rules are:
 * ARMv6: every reference is in the literal pool
 * ARMv7 but not a qualcomm chip: all patchable reference (CODE_TARGET, etc) are
in the literal pool, others are "moved" unless you are building snapshots or
predictable_code_size() is true (I believe this covers full-codegen)
 * ARMv7 and qualcomm chip: references are "moved" unless you are building
snapshots or predictable_code_size() is true.

If you were to store all the references in the out of line constant pool, I
believe that would:
 * simplify the code as there would be only one mechanism to load them (though
you would have to benchmark the impact on Snapdragon)
 * improve performance as the code page does not need to be mapped in the D-TLB
too

Cheers,
Rodolph.

[rmcilroy, 2013-09-26 12:14:03]

Updated to enable pointers to be stored in the ConstantPoolArray object and add
tests. PTAL.

[ulan, 2013-09-27 12:39:10]

LGTM with comments. Michael, do GC changes look good?

https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h#newcode...
src/objects-inl.h:2066: MaybeObject* ConstantPoolArray::get(int index) {
Let's either call this function Get to indicate that it does a lot of work (like
potential GC) or replace its uses with get_x_entry versions and remove this
function.

https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc#ne...
src/objects-printer.cc:644: }
The case for pointers is missing.

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

https://codereview.chromium.org/22601003/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:455: void
ConstantPoolArray::ConstantPoolIterateBody(ObjectVisitor* v) {
Better to move it in object.cc or object-inl.h

https://codereview.chromium.org/22601003/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3027
src/objects.h:3027: // ConstantPoolArray describes fixed-sized arrays constant
pool entires.
Did you mean:
fixed-sized arrays _with_ constant pool entires?

https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3033
src/objects.h:3033: //   [first_int32_index()] ... [length]: 32 bit entries
[length-1] ?

[rmcilroy, 2013-10-01 11:21:51]

https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h#newcode...
src/objects-inl.h:2066: MaybeObject* ConstantPoolArray::get(int index) {
On 2013/09/27 12:39:10, ulan wrote:
> Let's either call this function Get to indicate that it does a lot of work
(like
> potential GC) or replace its uses with get_x_entry versions and remove this
> function.

Removed the function instead.

https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc#ne...
src/objects-printer.cc:644: }
On 2013/09/27 12:39:10, ulan wrote:
> The case for pointers is missing.

Dang I noticed that after uploading and added it, but obviously forgot to
re-upload the CL... Done.

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

https://codereview.chromium.org/22601003/diff/20001/src/objects-visiting-inl....
src/objects-visiting-inl.h:455: void
ConstantPoolArray::ConstantPoolIterateBody(ObjectVisitor* v) {
On 2013/09/27 12:39:10, ulan wrote:
> Better to move it in object.cc or object-inl.h

Moved to objects.cc.

https://codereview.chromium.org/22601003/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3027
src/objects.h:3027: // ConstantPoolArray describes fixed-sized arrays constant
pool entires.
On 2013/09/27 12:39:10, ulan wrote:
> Did you mean:
> fixed-sized arrays _with_ constant pool entires?

Done.

https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3033
src/objects.h:3033: //   [first_int32_index()] ... [length]: 32 bit entries
On 2013/09/27 12:39:10, ulan wrote:
> [length-1] ?

Done.

[rmcilroy, 2013-10-02 12:46:04]

On 2013/10/01 11:21:51, rmcilroy wrote:
> https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h
> File src/objects-inl.h (right):
> 
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h#newcode...
> src/objects-inl.h:2066: MaybeObject* ConstantPoolArray::get(int index) {
> On 2013/09/27 12:39:10, ulan wrote:
> > Let's either call this function Get to indicate that it does a lot of work
> (like
> > potential GC) or replace its uses with get_x_entry versions and remove this
> > function.
> 
> Removed the function instead.
> 
> https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc
> File src/objects-printer.cc (right):
> 
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc#ne...
> src/objects-printer.cc:644: }
> On 2013/09/27 12:39:10, ulan wrote:
> > The case for pointers is missing.
> 
> Dang I noticed that after uploading and added it, but obviously forgot to
> re-upload the CL... Done.
> 
> https://codereview.chromium.org/22601003/diff/20001/src/objects-visiting-inl.h
> File src/objects-visiting-inl.h (right):
> 
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-visiting-inl....
> src/objects-visiting-inl.h:455: void
> ConstantPoolArray::ConstantPoolIterateBody(ObjectVisitor* v) {
> On 2013/09/27 12:39:10, ulan wrote:
> > Better to move it in object.cc or object-inl.h
> 
> Moved to objects.cc.
> 
> https://codereview.chromium.org/22601003/diff/20001/src/objects.h
> File src/objects.h (right):
> 
> https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3027
> src/objects.h:3027: // ConstantPoolArray describes fixed-sized arrays constant
> pool entires.
> On 2013/09/27 12:39:10, ulan wrote:
> > Did you mean:
> > fixed-sized arrays _with_ constant pool entires?
> 
> Done.
> 
> https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3033
> src/objects.h:3033: //   [first_int32_index()] ... [length]: 32 bit entries
> On 2013/09/27 12:39:10, ulan wrote:
> > [length-1] ?
> 
> Done.

Michael, any GC related comments?

[rmcilroy, 2013-10-07 09:31:07]

On 2013/10/02 12:46:04, rmcilroy wrote:
> On 2013/10/01 11:21:51, rmcilroy wrote:
> > https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h
> > File src/objects-inl.h (right):
> > 
> >
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h#newcode...
> > src/objects-inl.h:2066: MaybeObject* ConstantPoolArray::get(int index) {
> > On 2013/09/27 12:39:10, ulan wrote:
> > > Let's either call this function Get to indicate that it does a lot of work
> > (like
> > > potential GC) or replace its uses with get_x_entry versions and remove
this
> > > function.
> > 
> > Removed the function instead.
> > 
> > https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc
> > File src/objects-printer.cc (right):
> > 
> >
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc#ne...
> > src/objects-printer.cc:644: }
> > On 2013/09/27 12:39:10, ulan wrote:
> > > The case for pointers is missing.
> > 
> > Dang I noticed that after uploading and added it, but obviously forgot to
> > re-upload the CL... Done.
> > 
> >
https://codereview.chromium.org/22601003/diff/20001/src/objects-visiting-inl.h
> > File src/objects-visiting-inl.h (right):
> > 
> >
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-visiting-inl....
> > src/objects-visiting-inl.h:455: void
> > ConstantPoolArray::ConstantPoolIterateBody(ObjectVisitor* v) {
> > On 2013/09/27 12:39:10, ulan wrote:
> > > Better to move it in object.cc or object-inl.h
> > 
> > Moved to objects.cc.
> > 
> > https://codereview.chromium.org/22601003/diff/20001/src/objects.h
> > File src/objects.h (right):
> > 
> >
https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3027
> > src/objects.h:3027: // ConstantPoolArray describes fixed-sized arrays
constant
> > pool entires.
> > On 2013/09/27 12:39:10, ulan wrote:
> > > Did you mean:
> > > fixed-sized arrays _with_ constant pool entires?
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3033
> > src/objects.h:3033: //   [first_int32_index()] ... [length]: 32 bit entries
> > On 2013/09/27 12:39:10, ulan wrote:
> > > [length-1] ?
> > 
> > Done.
> 
> Michael, any GC related comments?

Ping?

[rmcilroy, 2013-10-14 12:21:58]

On 2013/10/07 09:31:07, rmcilroy wrote:
> On 2013/10/02 12:46:04, rmcilroy wrote:
> > On 2013/10/01 11:21:51, rmcilroy wrote:
> > > https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h
> > > File src/objects-inl.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-inl.h#newcode...
> > > src/objects-inl.h:2066: MaybeObject* ConstantPoolArray::get(int index) {
> > > On 2013/09/27 12:39:10, ulan wrote:
> > > > Let's either call this function Get to indicate that it does a lot of
work
> > > (like
> > > > potential GC) or replace its uses with get_x_entry versions and remove
> this
> > > > function.
> > > 
> > > Removed the function instead.
> > > 
> > > https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc
> > > File src/objects-printer.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-printer.cc#ne...
> > > src/objects-printer.cc:644: }
> > > On 2013/09/27 12:39:10, ulan wrote:
> > > > The case for pointers is missing.
> > > 
> > > Dang I noticed that after uploading and added it, but obviously forgot to
> > > re-upload the CL... Done.
> > > 
> > >
> https://codereview.chromium.org/22601003/diff/20001/src/objects-visiting-inl.h
> > > File src/objects-visiting-inl.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/22601003/diff/20001/src/objects-visiting-inl....
> > > src/objects-visiting-inl.h:455: void
> > > ConstantPoolArray::ConstantPoolIterateBody(ObjectVisitor* v) {
> > > On 2013/09/27 12:39:10, ulan wrote:
> > > > Better to move it in object.cc or object-inl.h
> > > 
> > > Moved to objects.cc.
> > > 
> > > https://codereview.chromium.org/22601003/diff/20001/src/objects.h
> > > File src/objects.h (right):
> > > 
> > >
> https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3027
> > > src/objects.h:3027: // ConstantPoolArray describes fixed-sized arrays
> constant
> > > pool entires.
> > > On 2013/09/27 12:39:10, ulan wrote:
> > > > Did you mean:
> > > > fixed-sized arrays _with_ constant pool entires?
> > > 
> > > Done.
> > > 
> > >
> https://codereview.chromium.org/22601003/diff/20001/src/objects.h#newcode3033
> > > src/objects.h:3033: //   [first_int32_index()] ... [length]: 32 bit
entries
> > > On 2013/09/27 12:39:10, ulan wrote:
> > > > [length-1] ?
> > > 
> > > Done.
> > 
> > Michael, any GC related comments?
> 
> Ping?


Ping? Please take a look.

[Michael Starzinger, 2013-10-14 12:35:49]

The object visitors are looking good to me. Sorry for not responding, since I
was only CCed, my filters ate the ping. Please feel free to ping me directly (or
add me as a reviewer) next time.

[rmcilroy, 2013-10-14 12:56:08]

On 2013/10/14 12:35:49, Michael Starzinger wrote:
> The object visitors are looking good to me. Sorry for not responding, since I
> was only CCed, my filters ate the ping. Please feel free to ping me directly
(or
> add me as a reviewer) next time.

Thanks for taking a look. Sorry, I thought Ulan had added you as a reviewer not
just CCed.  Cheers.

Ulan: could you land this for me please?

[ulan, 2013-10-14 13:35:25]

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