GC: Refactor public incremental marking interface in heap

GC: Refactor incremental marking interface from heap

BUG=

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

[Michael Lippautz, 2015-08-04 12:57:15]

mlippautz@chromium.org changed reviewers:
+ hpayer@chromium.org, ulan@chromium.org

[Michael Lippautz, 2015-08-04 12:57:16]

The idea is to then call 
  Heap::StartIncrementalMarking and 
  Heap::AdvanceIncrementalMarking
from api.cc and not loose any callback/flag information.

It should also be general enough to call with different step actions.

WDYT?

[ulan, 2015-08-04 13:13:31]

lgtm

https://codereview.chromium.org/1273483002/diff/1/src/heap/incremental-marking.h
File src/heap/incremental-marking.h (right):

https://codereview.chromium.org/1273483002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.h:31: CompletionAction complete_action_ =
NO_GC_VIA_STACK_GUARD,
For readability of the call site, I would prefer passing flags explicitly in the
main constructor and making static constructor function with descriptive name
for specific flag combinations that are frequently used.

[Michael Lippautz, 2015-08-04 13:55:35]

https://codereview.chromium.org/1273483002/diff/1/src/heap/incremental-marking.h
File src/heap/incremental-marking.h (right):

https://codereview.chromium.org/1273483002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.h:31: CompletionAction complete_action_ =
NO_GC_VIA_STACK_GUARD,
On 2015/08/04 13:13:31, ulan wrote:
> For readability of the call site, I would prefer passing flags explicitly in
the
> main constructor and making static constructor function with descriptive name
> for specific flag combinations that are frequently used.

Done.

[ulan, 2015-08-04 14:33:06]

lgtm

[Hannes Payer (out of office), 2015-08-04 15:05:59]

https://codereview.chromium.org/1273483002/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1273483002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4806: GCIdleTimeHandler::HeapState heap_state =
ComputeHeapState();
You don't need the heap_state for that. Just call EstimateMarkingStepSize with
incremental marking speed. Also, document that in the API.

https://codereview.chromium.org/1273483002/diff/60001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1273483002/diff/60001/src/heap/heap.h#newcode855
src/heap/heap.h:855: void StartIncrementalMarking(int gc_flags,
Add comments to these functions.

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

https://codereview.chromium.org/1273483002/diff/60001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:485: (reason == nullptr) ? "unknown reason" :
reason);
can we always have a reason?

[Michael Lippautz, 2015-08-04 15:53:06]

https://codereview.chromium.org/1273483002/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1273483002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4806: GCIdleTimeHandler::HeapState heap_state =
ComputeHeapState();
On 2015/08/04 15:05:59, Hannes Payer wrote:
> You don't need the heap_state for that. Just call EstimateMarkingStepSize with
> incremental marking speed. Also, document that in the API.

Done.

https://codereview.chromium.org/1273483002/diff/60001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1273483002/diff/60001/src/heap/heap.h#newcode855
src/heap/heap.h:855: void StartIncrementalMarking(int gc_flags,
On 2015/08/04 15:05:59, Hannes Payer wrote:
> Add comments to these functions.

Done.

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

https://codereview.chromium.org/1273483002/diff/60001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:485: (reason == nullptr) ? "unknown reason" :
reason);
On 2015/08/04 15:05:59, Hannes Payer wrote:
> can we always have a reason?

Done.

[Hannes Payer (out of office), 2015-08-04 16:21:52]

lgtm, one nit

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

https://codereview.chromium.org/1273483002/diff/80001/src/heap/heap.cc#newcod...
src/heap/heap.cc:983: incremental_marking()->Start(kNoGCFlags,
kNoGCCallbackFlags, "full GC");
GC epilogue

[Michael Lippautz, 2015-08-04 16:26:19]

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

https://codereview.chromium.org/1273483002/diff/80001/src/heap/heap.cc#newcod...
src/heap/heap.cc:983: incremental_marking()->Start(kNoGCFlags,
kNoGCCallbackFlags, "full GC");
On 2015/08/04 16:21:52, Hannes Payer wrote:
> GC epilogue

Done.

[Michael Lippautz, 2015-08-04 16:26:22]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2015-08-04 16:26:22]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1273483002/#ps100001
(title: "Addressed nit")

[commit-bot: I haz the power, 2015-08-04 16:26:37]

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

[commit-bot: I haz the power, 2015-08-04 16:56:34]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-04 16:57:21]

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

[adamk, 2015-08-04 17:47:14]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1271823006/ by adamk@chromium.org.

The reason for reverting is: Fails on the MSAN builder:

http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20s...

Likely due to lack of initialization of IncrementalMarking::gc_callback_flags_..