Ensure that we don't mark weak heap references in the constant pool array.

Ensure that we don't mark weak heap references in the constant pool array.

Some heap pointer's embedded in optimized code are considered weak. Ensure
that we don't mark them during GC of the ConstantPoolArray.  Also, embed
length metadata in a bitfield, reducing the ConstantPoolArray header size from
five words to two.

R=ulan@chromium.org

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

[rmcilroy, 2014-03-24 12:52:35]

Ulan: Unfortunately your suggestion of simply not marking heap entries in the
Constant Pool didn't work because the VerifyMarking object visitor would
complain that some object's weren't marked.  I've changed approach and now
simply mark whether the constant pool is for optimized code and do the
appropriate checks in object visitor.  PTAL.

[ulan, 2014-03-24 13:17:11]

On 2014/03/24 12:52:35, rmcilroy wrote:
> Ulan: Unfortunately your suggestion of simply not marking heap entries in the
> Constant Pool didn't work because the VerifyMarking object visitor would
> complain that some object's weren't marked.  I've changed approach and now
> simply mark whether the constant pool is for optimized code and do the
> appropriate checks in object visitor.  PTAL.

Interesting! Could you give me command lines that reproduce the failure?

This approach looks good. I have two suggestions:
- instead of bool is_optimized please use an enum because soon IC stubs will
have weak maps too.
- instead of passing the flag through heap and factory allocation functions,
allocate the constant pool with a default flag value that visits all references,
and then update the flag in the constant pool to the correct value in
LCodeGenBase::RegisterWeakObjectsInOptimizedCode.

[rmcilroy, 2014-03-27 12:19:28]

On 2014/03/24 13:17:11, ulan (offline till April 1) wrote:
> On 2014/03/24 12:52:35, rmcilroy wrote:
> > Ulan: Unfortunately your suggestion of simply not marking heap entries in
the
> > Constant Pool didn't work because the VerifyMarking object visitor would
> > complain that some object's weren't marked.  I've changed approach and now
> > simply mark whether the constant pool is for optimized code and do the
> > appropriate checks in object visitor.  PTAL.
> 
> Interesting! Could you give me command lines that reproduce the failure?
> 
> This approach looks good. I have two suggestions:
> - instead of bool is_optimized please use an enum because soon IC stubs will
> have weak maps too.
> - instead of passing the flag through heap and factory allocation functions,
> allocate the constant pool with a default flag value that visits all
references,
> and then update the flag in the constant pool to the correct value in
> LCodeGenBase::RegisterWeakObjectsInOptimizedCode.

Good suggestions, thanks Ulan.  I've uploaded a new patch set with these
suggestions. PTAL.

[ulan, 2014-04-01 19:09:14]

https://codereview.chromium.org/209473006/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/209473006/diff/40001/src/objects.h#newcode3269
src/objects.h:3269: class NumberOfInt64EntriesField: public BitField<int, 0, 10>
{};
How do we guarantee that the number of entries does not overflow?

[rmcilroy, 2014-04-07 14:03:55]

Sorry for the delay - I've been busy with Chrome things.  PTAL.

https://codereview.chromium.org/209473006/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/209473006/diff/40001/src/objects.h#newcode3269
src/objects.h:3269: class NumberOfInt64EntriesField: public BitField<int, 0, 10>
{};
On 2014/04/01 19:09:14, ulan wrote:
> How do we guarantee that the number of entries does not overflow?

Good point (on Arm we never would due to checks in assembler-arm.cc, but we
should ensure this is always the case).  Added checks in heap.cc which return
FatalProcessOutOfMemory if length is wrong.

[ulan, 2014-04-07 14:14:40]

LGTM with one comment:

https://codereview.chromium.org/209473006/diff/80001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/209473006/diff/80001/src/heap.cc#newcode5310
src/heap.cc:5310: v8::internal::Heap::FatalProcessOutOfMemory("invalid array
length", true);
Let's use ASSERT or CHECK instead of FatalProcessOutOfMemory because it is not
OOM.

[rmcilroy, 2014-04-07 14:23:12]

https://codereview.chromium.org/209473006/diff/80001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/209473006/diff/80001/src/heap.cc#newcode5310
src/heap.cc:5310: v8::internal::Heap::FatalProcessOutOfMemory("invalid array
length", true);
On 2014/04/07 14:14:40, ulan wrote:
> Let's use ASSERT or CHECK instead of FatalProcessOutOfMemory because it is not
> OOM.

I was following what AllocateRawFixedArray does with FatalProcessOutOfMemory.  I
can change it to CHECK if you prefer (not ASSERT though since I want this to
happen in release too).

[ulan, 2014-04-07 14:52:01]

https://codereview.chromium.org/209473006/diff/80001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/209473006/diff/80001/src/heap.cc#newcode5310
src/heap.cc:5310: v8::internal::Heap::FatalProcessOutOfMemory("invalid array
length", true);
On 2014/04/07 14:23:12, rmcilroy wrote:
> On 2014/04/07 14:14:40, ulan wrote:
> > Let's use ASSERT or CHECK instead of FatalProcessOutOfMemory because it is
not
> > OOM.
> 
> I was following what AllocateRawFixedArray does with FatalProcessOutOfMemory. 
I
> can change it to CHECK if you prefer (not ASSERT though since I want this to
> happen in release too).

I would use CHECK in AllocateRawFixedArray too, but I see how it can be
connected to OOM: allocating a fixed array with kMaxLength elements (128M
elements) would likely cause OOM.

Here exceeding kMaxEntriesPerType (1K) is probably the result of a bug, and is
not related to OOM. So a simple CHECK seems more suitable.

[rmcilroy, 2014-04-07 20:19:17]

https://codereview.chromium.org/209473006/diff/80001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/209473006/diff/80001/src/heap.cc#newcode5310
src/heap.cc:5310: v8::internal::Heap::FatalProcessOutOfMemory("invalid array
length", true);
On 2014/04/07 14:52:01, ulan wrote:
> On 2014/04/07 14:23:12, rmcilroy wrote:
> > On 2014/04/07 14:14:40, ulan wrote:
> > > Let's use ASSERT or CHECK instead of FatalProcessOutOfMemory because it is
> not
> > > OOM.
> > 
> > I was following what AllocateRawFixedArray does with
FatalProcessOutOfMemory. 
> I
> > can change it to CHECK if you prefer (not ASSERT though since I want this to
> > happen in release too).
> 
> I would use CHECK in AllocateRawFixedArray too, but I see how it can be
> connected to OOM: allocating a fixed array with kMaxLength elements (128M
> elements) would likely cause OOM.
> 
> Here exceeding kMaxEntriesPerType (1K) is probably the result of a bug, and is
> not related to OOM. So a simple CHECK seems more suitable.

Sounds good - done.

[ulan, 2014-04-08 08:20:20]

LGTM

[rmcilroy, 2014-04-08 10:01:07]

Committed patchset #6 manually as r20573 (presubmit successful).