[heap] Minor MC: Add marking

[heap] Minor MC: Add marking

Adds the marking logic to mark the young generation.

BUG=chromium:651354
Committed: https://crrev.com/7e5755cbc594b610fc26493a2bad547dd69138f3
Cr-Commit-Position: refs/heads/master@{#41104}

[Michael Lippautz, 2016-11-11 12:49:44]

Patchset #3 (id:40001) has been deleted

[Michael Lippautz, 2016-11-15 18:16:05]

Description was changed from

==========
[heap] Minor MC: Add marking and verification mode

BUG=
==========

to

==========
[heap] Minor MC: Add marking and verification mode

Adds the marking logic to mark the young generation.

Verification mode:
- gn args: v8_enable_verify_heap = true
- flag: --verify-minor-mc-marking

The marking stats are printed when called with --trace-gc-nvp and 
represent the time duration that would've been needed in a minor mc.

BUG=
==========

[Michael Lippautz, 2016-11-15 18:16:50]

Description was changed from

==========
[heap] Minor MC: Add marking and verification mode

Adds the marking logic to mark the young generation.

Verification mode:
- gn args: v8_enable_verify_heap = true
- flag: --verify-minor-mc-marking

The marking stats are printed when called with --trace-gc-nvp and 
represent the time duration that would've been needed in a minor mc.

BUG=
==========

to

==========
[heap] Minor MC: Add marking and verification mode

Adds the marking logic to mark the young generation.

Verification mode:
- gn args: v8_enable_verify_heap = true
- flag: --verify-minor-mc-marking

The marking stats are printed when called with --trace-gc-nvp and 
represent the time duration that would've been needed in a minor mc.

BUG=chromium:651354
==========

[Michael Lippautz, 2016-11-15 18:17:21]

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

[Michael Lippautz, 2016-11-15 18:18:24]

PTAL, this is now ready; Successfully ran Octane and the whole test suite using
the verifier.

[ulan, 2016-11-15 19:57:22]

Exciting stuff! Few comments, will review more carefully tomorrow.

https://codereview.chromium.org/2498583002/diff/220001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/2498583002/diff/220001/src/global-handles.cc#...
src/global-handles.cc:722: template <GlobalHandles::IterationMode mode>
mode is unused in this function.

https://codereview.chromium.org/2498583002/diff/220001/src/global-handles.cc#...
src/global-handles.cc:749: void GlobalHandles::ResetNewSpaceHandlesForTesting()
{
Would it be more robust to remember the state before changes and restore it
here, instead of undoing?

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

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.cc:2266: if (backing_store_committed_size_ == 0) return;
How can this happen?

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.cc:2365: // ToSpace.
DCHECK(object_in_new_space implies object_in_to_space) ?

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.cc:2381: return heap->InNewSpace(*p) &&
For consistency let's use either InNewSpace or InToSpace in all places.

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact.h
File src/heap/mark-compact.h (right):

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.h:78: void StopUsing(bool free_immediately = false);
nit: let's use enum instead of bool

[Michael Lippautz, 2016-11-16 08:59:37]

Thanks for the first round. Still trying to figure out a better way for the
global handles mess.

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

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.cc:2266: if (backing_store_committed_size_ == 0) return;
On 2016/11/15 19:57:21, ulan wrote:
> How can this happen?

I added a comment: Basically, two calls for StopUsing running concurrently,
i.e., one synchronously, and one in the task. The synchronous one would set the
in_use_ flag to false and immediately uncommit the buffer. The task observed the
deque is not in use anymore and tries to uncommit again.

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.cc:2365: // ToSpace.
On 2016/11/15 19:57:21, ulan wrote:
> DCHECK(object_in_new_space implies object_in_to_space) ?

Used InNewSpace with a DCHECK for ToSpace.

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.cc:2381: return heap->InNewSpace(*p) &&
On 2016/11/15 19:57:21, ulan wrote:
> For consistency let's use either InNewSpace or InToSpace in all places.

Done (added dcheck).

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact.h
File src/heap/mark-compact.h (right):

https://codereview.chromium.org/2498583002/diff/220001/src/heap/mark-compact....
src/heap/mark-compact.h:78: void StopUsing(bool free_immediately = false);
On 2016/11/15 19:57:21, ulan wrote:
> nit: let's use enum instead of bool

Done.

[Michael Lippautz, 2016-11-16 10:25:26]

PTAL. Restructured the global handles code and the entry points into
verification.

https://codereview.chromium.org/2498583002/diff/220001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/2498583002/diff/220001/src/global-handles.cc#...
src/global-handles.cc:722: template <GlobalHandles::IterationMode mode>
On 2016/11/15 19:57:21, ulan wrote:
> mode is unused in this function.

Done. Worked by accident because none of our tests check against dupes in
callbacks.

https://codereview.chromium.org/2498583002/diff/220001/src/global-handles.cc#...
src/global-handles.cc:749: void GlobalHandles::ResetNewSpaceHandlesForTesting()
{
On 2016/11/15 19:57:21, ulan wrote:
> Would it be more robust to remember the state before changes and restore it
> here, instead of undoing?

Yes! Done.

[ulan, 2016-11-16 10:46:55]

lgtm!

[Michael Lippautz, 2016-11-17 12:17:50]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 12:18:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hannes Payer (out of office), 2016-11-18 07:33:02]

Before I have a close look, one general question:
Do we need the verification code? In case we are going to keep the scavenger
code, I do not think it would add much value to add a verifier based on marking.
Since it adds quite some complexity and a couple of ifdefs I would rather not
add this code. WDYT?

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

https://codereview.chromium.org/2498583002/diff/300001/src/heap/mark-compact....
src/heap/mark-compact.cc:2256: void MarkingDeque::StopUsing(UncommitMode mode) {
Based on the recent latency numbers of uncommit, we should always do that
concurrently.

[Michael Lippautz, 2016-11-18 09:44:29]

Description was changed from

==========
[heap] Minor MC: Add marking and verification mode

Adds the marking logic to mark the young generation.

Verification mode:
- gn args: v8_enable_verify_heap = true
- flag: --verify-minor-mc-marking

The marking stats are printed when called with --trace-gc-nvp and 
represent the time duration that would've been needed in a minor mc.

BUG=chromium:651354
==========

to

==========
[heap] Minor MC: Add marking

Adds the marking logic to mark the young generation.

BUG=chromium:651354
==========

[Michael Lippautz, 2016-11-18 09:46:19]

PTAL. Removed the verifier code.

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

https://codereview.chromium.org/2498583002/diff/300001/src/heap/mark-compact....
src/heap/mark-compact.cc:2256: void MarkingDeque::StopUsing(UncommitMode mode) {
On 2016/11/18 07:33:02, Hannes Payer wrote:
> Based on the recent latency numbers of uncommit, we should always do that
> concurrently.

Yes, obviously. Let's leave this for later though as I don't want to add
concurrency optimizations while still trying to get correctness.

[Hannes Payer (out of office), 2016-11-18 10:07:11]

LGTM. Nice reuse of marking infrastructure.

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

https://codereview.chromium.org/2498583002/diff/300001/src/heap/mark-compact....
src/heap/mark-compact.cc:2256: void MarkingDeque::StopUsing(UncommitMode mode) {
On 2016/11/18 09:46:19, Michael Lippautz wrote:
> On 2016/11/18 07:33:02, Hannes Payer wrote:
> > Based on the recent latency numbers of uncommit, we should always do that
> > concurrently.
> 
> Yes, obviously. Let's leave this for later though as I don't want to add
> concurrency optimizations while still trying to get correctness.

As discussed offline. I would trust the concurrent uncommit implementation and
would just use it for simplicity.

[Michael Lippautz, 2016-11-18 11:33:32]

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

https://codereview.chromium.org/2498583002/diff/300001/src/heap/mark-compact....
src/heap/mark-compact.cc:2256: void MarkingDeque::StopUsing(UncommitMode mode) {
On 2016/11/18 10:07:11, Hannes Payer wrote:
> On 2016/11/18 09:46:19, Michael Lippautz wrote:
> > On 2016/11/18 07:33:02, Hannes Payer wrote:
> > > Based on the recent latency numbers of uncommit, we should always do that
> > > concurrently.
> > 
> > Yes, obviously. Let's leave this for later though as I don't want to add
> > concurrency optimizations while still trying to get correctness.
> 
> As discussed offline. I would trust the concurrent uncommit implementation and
> would just use it for simplicity.

Done.

[Michael Lippautz, 2016-11-18 11:33:39]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 11:33:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 12:15:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 12:15:43]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2016-11-18 12:32:46]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-11-18 12:32:47]

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/2498583002/#ps340001
(title: "Move to concurrent uncomitting of marking deque")

[commit-bot: I haz the power, 2016-11-18 12:32:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 12:55:58]

Description was changed from

==========
[heap] Minor MC: Add marking

Adds the marking logic to mark the young generation.

BUG=chromium:651354
==========

to

==========
[heap] Minor MC: Add marking

Adds the marking logic to mark the young generation.

BUG=chromium:651354
==========

[commit-bot: I haz the power, 2016-11-18 12:55:59]

Committed patchset #17 (id:340001)

[commit-bot: I haz the power, 2016-11-18 12:56:22]

Description was changed from

==========
[heap] Minor MC: Add marking

Adds the marking logic to mark the young generation.

BUG=chromium:651354
==========

to

==========
[heap] Minor MC: Add marking

Adds the marking logic to mark the young generation.

BUG=chromium:651354

Committed: https://crrev.com/7e5755cbc594b610fc26493a2bad547dd69138f3
Cr-Commit-Position: refs/heads/master@{#41104}
==========

[commit-bot: I haz the power, 2016-11-18 12:56:23]

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