Fix Heap::IsHeapIterable.

Fix Heap::IsHeapIterable.

We only consider heap iterable if the new space is empty (in addition to the exisiting old space check).

The change also moves the iterability forcing + allocation prevention gadgets to HeapIterator so that it is impossible to miss them when iterating the heap.

R=hpayer@chromium.org
BUG=373283
LOG=N

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

[Hannes Payer (out of office), 2014-05-18 18:25:43]

https://codereview.chromium.org/285693006/diff/1/src/heap.cc
File src/heap.cc (left):

https://codereview.chromium.org/285693006/diff/1/src/heap.cc#oldcode4316
src/heap.cc:4316: void Heap::EnsureHeapIsIterable() {
Let's rename it to MakeIterable.

https://codereview.chromium.org/285693006/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/285693006/diff/1/src/runtime.cc#newcode13149
src/runtime.cc:13149: heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
"%DebugConstructedBy");
Call MakeIterable instead.

https://codereview.chromium.org/285693006/diff/1/src/runtime.cc#newcode13225
src/runtime.cc:13225: heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
"%DebugConstructedBy");
Call MakeIterable instead.

[Jarin, 2014-05-19 06:06:42]

https://codereview.chromium.org/285693006/diff/1/src/heap.cc
File src/heap.cc (left):

https://codereview.chromium.org/285693006/diff/1/src/heap.cc#oldcode4316
src/heap.cc:4316: void Heap::EnsureHeapIsIterable() {
On 2014/05/18 18:25:43, Hannes Payer wrote:
> Let's rename it to MakeIterable.

Done.

https://codereview.chromium.org/285693006/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/285693006/diff/1/src/runtime.cc#newcode13149
src/runtime.cc:13149: heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
"%DebugConstructedBy");
On 2014/05/18 18:25:43, Hannes Payer wrote:
> Call MakeIterable instead.

I think this should still call CollectAllGarbage because we need the garbage
collection to discard dead objects. MakeHeapIterable does not guarantee there
will be GC.

https://codereview.chromium.org/285693006/diff/1/src/runtime.cc#newcode13225
src/runtime.cc:13225: heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
"%DebugConstructedBy");
On 2014/05/18 18:25:43, Hannes Payer wrote:
> Call MakeIterable instead.

See above.

[Hannes Payer (out of office), 2014-05-19 08:31:12]

LGTM

https://codereview.chromium.org/285693006/diff/20001/src/heap.cc
File src/heap.cc (left):

https://codereview.chromium.org/285693006/diff/20001/src/heap.cc#oldcode4312
src/heap.cc:4312: !old_data_space()->was_swept_conservatively());
Now I am not sure if we should remove that method. Why don't we keep it and add
new_space()->top() == new_space->bottom(). This would be very conservative. I
leave that up to you.

https://codereview.chromium.org/285693006/diff/20001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/285693006/diff/20001/src/heap.h#newcode767
src/heap.h:767: void MakeHeapIterable();
I would call it MakeIterable, since it is a method on heap.

[Jarin, 2014-05-20 11:56:57]

On 2014/05/19 08:31:12, Hannes Payer wrote:
> LGTM
> 
> https://codereview.chromium.org/285693006/diff/20001/src/heap.cc
> File src/heap.cc (left):
> 
> https://codereview.chromium.org/285693006/diff/20001/src/heap.cc#oldcode4312
> src/heap.cc:4312: !old_data_space()->was_swept_conservatively());
> Now I am not sure if we should remove that method. Why don't we keep it and
add
> new_space()->top() == new_space->bottom(). This would be very conservative. I
> leave that up to you.
> 
> https://codereview.chromium.org/285693006/diff/20001/src/heap.h
> File src/heap.h (right):
> 
> https://codereview.chromium.org/285693006/diff/20001/src/heap.h#newcode767
> src/heap.h:767: void MakeHeapIterable();
> I would call it MakeIterable, since it is a method on heap.

As you suggested, I have resurrected IsHeapIterable and made it require that the
new space is empty.

I have also moved the DisallowHeapAllocation gadget and the MakeHeapIterable
call to the HeapIterator class.

[Hannes Payer (out of office), 2014-05-20 13:06:09]

wohooo, double LGTM

https://codereview.chromium.org/285693006/diff/60001/src/liveedit.cc
File src/liveedit.cc (right):

https://codereview.chromium.org/285693006/diff/60001/src/liveedit.cc#newcode946
src/liveedit.cc:946: DisallowHeapAllocation no_allocation;
should be coverd by HeapIterator iterator(heap); please remove it

[Jarin, 2014-05-20 13:19:50]

Committed patchset #5 manually as r21387 (presubmit successful).