[heap] Cleanup and fix GC flags / add testing infra

[heap] Cleanup and fix GC flags

GC flags are now part of the {Heap} and should be respected by all
sub-components.

Also add a infrastructure to write tests accessing private methods.

Committed: https://crrev.com/a56f537381ddfb8f89394880e1f0fa63a17cd510
Cr-Commit-Position: refs/heads/master@{#30281}

[Michael Lippautz, 2015-08-20 13:06:29]

Hannes: The change we talked about. This is pre-work for fixing chromium:520607.

Benedikt and Michi: Please check the approach for using private methods in a
cctest. It's similar to what gtest does without the need to friend every test
(because it lives in a separate class). The drawback is the additional macro on
top of heap-test.cc to get the declarations. The approach is  also purely local
to heap testing right now.

Michi: This would allow us to get rid of the protected-for-testing declarations
in Heap and the TestHeap class in cctest.h

https://codereview.chromium.org/1301183002/diff/40001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:2341: friend class IncrementalMarking;
Needed because we still have callers using incremental marking directly. (see
below)

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:490: StartMarking();
Let's not reset the flags after kicking off incremental marking.

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
File src/heap/incremental-marking.h (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.h:84: void Start(int flags,
We should avoid calling into incremental marking directly. Let's do this in
another CL.

[Michael Lippautz, 2015-08-20 13:07:25]

mlippautz@chromium.org changed reviewers:
+ bmeurer@chromium.org, hpayer@chromium.org, mstarzinger@chromium.org

[Michael Lippautz, 2015-08-20 13:07:26]

See previous message :)

[Michael Starzinger, 2015-08-20 13:33:37]

Looking good. I like it.

https://codereview.chromium.org/1301183002/diff/40001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/heap.h#newcode28
src/heap/heap.h:28: class HeapTester;
nit: No need for forward declare, friend declarations are implicit forward
declarations (in the correct namespace).

https://codereview.chromium.org/1301183002/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:2294: // Currently set gc flags that are respected by all
components, e.g. Heap,
nit: s/gc/GC/

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:490: StartMarking();
On 2015/08/20 13:06:29, Michael Lippautz wrote:
> Let's not reset the flags after kicking off incremental marking.

Acknowledged. Can we place a TODO in that regard here?

https://codereview.chromium.org/1301183002/diff/40001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:788: // if (was_marked_incrementally_ &&
abort_incremental_marking_) {
Comment looks like a leftover.

https://codereview.chromium.org/1301183002/diff/40001/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/test/cctest/test-debug....
test/cctest/test-debug.cc:7319: int CountNativeContexts();
Would "int v8::internal::CountNativeContexts();" also work here?

[Michael Lippautz, 2015-08-20 15:46:29]

https://codereview.chromium.org/1301183002/diff/40001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/heap.h#newcode28
src/heap/heap.h:28: class HeapTester;
On 2015/08/20 13:33:37, Michael Starzinger wrote:
> nit: No need for forward declare, friend declarations are implicit forward
> declarations (in the correct namespace).

Done.

https://codereview.chromium.org/1301183002/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:2294: // Currently set gc flags that are respected by all
components, e.g. Heap,
On 2015/08/20 13:33:37, Michael Starzinger wrote:
> nit: s/gc/GC/

Done.

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:490: StartMarking();
On 2015/08/20 13:33:37, Michael Starzinger wrote:
> On 2015/08/20 13:06:29, Michael Lippautz wrote:
> > Let's not reset the flags after kicking off incremental marking.
> 
> Acknowledged. Can we place a TODO in that regard here?

It's not a TODO. We should not reset them here, because a MarkSweep finalizing
incremental marking should respect these flags.

https://codereview.chromium.org/1301183002/diff/40001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:788: // if (was_marked_incrementally_ &&
abort_incremental_marking_) {
On 2015/08/20 13:33:37, Michael Starzinger wrote:
> Comment looks like a leftover.

Done.

https://codereview.chromium.org/1301183002/diff/40001/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/test/cctest/test-debug....
test/cctest/test-debug.cc:7319: int CountNativeContexts();
On 2015/08/20 13:33:37, Michael Starzinger wrote:
> Would "int v8::internal::CountNativeContexts();" also work here?

Nope, not possible in C++ as far as I know. Fully qualified names only work for
entities that already have been declared.

[Michael Starzinger, 2015-08-20 15:52:04]

LGTM from my end.

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:490: StartMarking();
On 2015/08/20 15:46:28, Michael Lippautz wrote:
> On 2015/08/20 13:33:37, Michael Starzinger wrote:
> > On 2015/08/20 13:06:29, Michael Lippautz wrote:
> > > Let's not reset the flags after kicking off incremental marking.
> > 
> > Acknowledged. Can we place a TODO in that regard here?
> 
> It's not a TODO. We should not reset them here, because a MarkSweep finalizing
> incremental marking should respect these flags.

Acknowledged. Ah, now I understand, this comment was about the removal of the
line below. Got it. Sorry!

https://codereview.chromium.org/1301183002/diff/40001/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/1301183002/diff/40001/test/cctest/test-debug....
test/cctest/test-debug.cc:7319: int CountNativeContexts();
On 2015/08/20 15:46:29, Michael Lippautz wrote:
> On 2015/08/20 13:33:37, Michael Starzinger wrote:
> > Would "int v8::internal::CountNativeContexts();" also work here?
> 
> Nope, not possible in C++ as far as I know. Fully qualified names only work
for
> entities that already have been declared.

Acknowledged.

https://codereview.chromium.org/1301183002/diff/80001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1301183002/diff/80001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2814: CHECK_EQ(true, heap->current_gc_flags() &
Heap::kReduceMemoryFootprintMask);
nit: Comparison of boolean with integer looks fishy. Can we use ...

CHECK_NE(0, heap->current_gc_flags() & Heap::kReduceMemoryFootprintMask);

https://codereview.chromium.org/1301183002/diff/80001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2817: CHECK_EQ(true, heap->current_gc_flags() &
Heap::kReduceMemoryFootprintMask);
nit: Likewise.

[Michael Lippautz, 2015-08-20 16:15:05]

https://codereview.chromium.org/1301183002/diff/80001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1301183002/diff/80001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2814: CHECK_EQ(true, heap->current_gc_flags() &
Heap::kReduceMemoryFootprintMask);
On 2015/08/20 15:52:03, Michael Starzinger wrote:
> nit: Comparison of boolean with integer looks fishy. Can we use ...
> 
> CHECK_NE(0, heap->current_gc_flags() & Heap::kReduceMemoryFootprintMask);

Done.

https://codereview.chromium.org/1301183002/diff/80001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2817: CHECK_EQ(true, heap->current_gc_flags() &
Heap::kReduceMemoryFootprintMask);
On 2015/08/20 15:52:03, Michael Starzinger wrote:
> nit: Likewise.

Done.

[Hannes Payer (out of office), 2015-08-20 16:28:30]

lgtm, nits attached

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.cc#newco...
src/heap/heap.cc:797: void Heap::CollectAllGarbage(int flags, const char*
gc_reason,
We should change int flags in a follow-up cl to an enum.

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:2291: // Currently set GC flags that are respected by all
components, e.g. Heap,
... all GC components.

https://codereview.chromium.org/1301183002/diff/100001/src/heap/mark-compact-...
File src/heap/mark-compact-inl.h (left):

https://codereview.chromium.org/1301183002/diff/100001/src/heap/mark-compact-...
src/heap/mark-compact-inl.h:21: DCHECK(!finalize_incremental_marking_ ||
!abort_incremental_marking_);
Bring this DCHECK back where the flags are set now.

[Michael Starzinger, 2015-08-20 16:43:05]

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.cc#newco...
src/heap/heap.cc:797: void Heap::CollectAllGarbage(int flags, const char*
gc_reason,
On 2015/08/20 16:28:29, Hannes Payer wrote:
> We should change int flags in a follow-up cl to an enum.

I think plain enums are yucky, IMHO "typedef base::Flags<Flag> Flags" is what
you want. But that is just, like, my opinion man. :)

[Michael Lippautz, 2015-08-20 17:02:19]

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.cc#newco...
src/heap/heap.cc:797: void Heap::CollectAllGarbage(int flags, const char*
gc_reason,
On 2015/08/20 16:28:29, Hannes Payer wrote:
> We should change int flags in a follow-up cl to an enum.

Acknowledged.

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1301183002/diff/100001/src/heap/heap.h#newcod...
src/heap/heap.h:2291: // Currently set GC flags that are respected by all
components, e.g. Heap,
On 2015/08/20 16:28:30, Hannes Payer wrote:
> ... all GC components.

Done.

https://codereview.chromium.org/1301183002/diff/100001/src/heap/mark-compact-...
File src/heap/mark-compact-inl.h (left):

https://codereview.chromium.org/1301183002/diff/100001/src/heap/mark-compact-...
src/heap/mark-compact-inl.h:21: DCHECK(!finalize_incremental_marking_ ||
!abort_incremental_marking_);
On 2015/08/20 16:28:30, Hannes Payer wrote:
> Bring this DCHECK back where the flags are set now.

Done.

[Benedikt Meurer, 2015-08-21 05:26:43]

LGTM.

One question: While we are changing the heap testing stuff, can we migrate the
tests to unittests using GoogleTest?

[Michael Lippautz, 2015-08-21 06:43:27]

On 2015/08/21 05:26:43, Benedikt Meurer wrote:
> LGTM.
> 
> One question: While we are changing the heap testing stuff, can we migrate the
> tests to unittests using GoogleTest?

Thought about it but didn't dare to touch and convert the tests (yet). I will
have a look at some point though.

[Michael Lippautz, 2015-08-21 06:43:31]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2015-08-21 06:43:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1301183002/#ps120001
(title: "Addressed Hannes' comments")

[commit-bot: I haz the power, 2015-08-21 06:43:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1301183002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1301183002/120001

[commit-bot: I haz the power, 2015-08-21 07:09:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-21 07:09:25]

Patchset 7 (id:??) landed as
https://crrev.com/a56f537381ddfb8f89394880e1f0fa63a17cd510
Cr-Commit-Position: refs/heads/master@{#30281}