[heap] Remove black pages and use black areas instead.

[heap] Remove black pages and use black areas instead.

BUG=630386
LOG=n

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

[Hannes Payer (out of office), 2016-07-18 15:07:32]

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

[commit-bot: I haz the power, 2016-07-18 15:07:37]

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-07-18 15:26:55]

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

[commit-bot: I haz the power, 2016-07-18 15:26:56]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9236)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)

[Hannes Payer (out of office), 2016-07-19 08:52:49]

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

[commit-bot: I haz the power, 2016-07-19 08:52:58]

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-07-19 09:35:21]

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

[Hannes Payer (out of office), 2016-07-19 09:35:22]

PTAL
black allocation will be enabled in a separate CL
try bots passed with black allocation on previous patchset

[ulan, 2016-07-19 13:23:32]

Looking good. Few nits:

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:603: heap()->old_space()->EmptyAllocationInfo();
Instead of dropping the linear allocation area, how about just setting the range
for the current top and limit?

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.h:227: MemoryChunk::IncrementLiveBytesFromGC(to,
size);
This side-effect of the function is unexpected. Why not do it on call-site?

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact-i...
File src/heap/mark-compact-inl.h (right):

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact-i...
src/heap/mark-compact-inl.h:163: MemoryChunk* chunk =
MemoryChunk::FromAddress(end);
DCHECK_EQ(chunk_, MemoryChunk::FromAddress(end));
and use chunk_ instead of chunk

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact-i...
src/heap/mark-compact-inl.h:172: }
Let's add a comment, something like: Clear all bits in the cell up to and
including the end.

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:115: current = next_object_must_be_here_or_later;
This is making verification less strict.

How about additionally checking that either 
1) all bits are set in [object ... current) or
2) only the header bits are set?

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact.h...
src/heap/mark-compact.h:226: DCHECK(new_cell_index > cell_index_);
*DCHECK_GT
DCHECK_LE(new_cell_index, last_cell_index_)

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/marking.h#newc...
src/heap/marking.h:109: void SetRange(uint32_t start_index, uint32_t end_index)
{
Let's add a comment describing that it sets bits in range [start_index,
end_index): end is not included.

https://codereview.chromium.org/2160613002/diff/40001/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/2160613002/diff/40001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1278: Free(current_top, old_linear_size);
Let's move it after clearing the range, so that we do not free marked area.

https://codereview.chromium.org/2160613002/diff/40001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1285: page->IncrementLiveBytes(static_cast<int>(current_top -
current_limit));
To avoid large unsigned to int conversions, let's do
-static_cast<int>(current_limit - current_top)

Alternatively, how about adding page->DecrementLiveBytes, so that the intention
is obvious from code?

[Hannes Payer (out of office), 2016-07-19 14:42:54]

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:603: heap()->old_space()->EmptyAllocationInfo();
On 2016/07/19 13:23:32, ulan wrote:
> Instead of dropping the linear allocation area, how about just setting the
range
> for the current top and limit? 

I did the lazy approach. We can also mark it directly black. Done.

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.h:227: MemoryChunk::IncrementLiveBytesFromGC(to,
size);
On 2016/07/19 13:23:32, ulan wrote:
> This side-effect of the function is unexpected. Why not do it on call-site?

As discussed offline. I will do this change in a separate CL. Together with
decreasing liveness and adding a higher-level marking operation that also takes
care of live bytes.

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact-i...
File src/heap/mark-compact-inl.h (right):

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact-i...
src/heap/mark-compact-inl.h:163: MemoryChunk* chunk =
MemoryChunk::FromAddress(end);
On 2016/07/19 13:23:32, ulan wrote:
> DCHECK_EQ(chunk_, MemoryChunk::FromAddress(end));
> and use chunk_ instead of chunk

Done.

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact-i...
src/heap/mark-compact-inl.h:172: }
On 2016/07/19 13:23:32, ulan wrote:
> Let's add a comment, something like: Clear all bits in the cell up to and
> including the end.

Done.

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:115: current = next_object_must_be_here_or_later;
On 2016/07/19 13:23:32, ulan wrote:
> This is making verification less strict.
> 
> How about additionally checking that either 
> 1) all bits are set in [object ... current) or
> 2) only the header bits are set?

Good idea! Done.

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/mark-compact.h...
src/heap/mark-compact.h:226: DCHECK(new_cell_index > cell_index_);
On 2016/07/19 13:23:32, ulan wrote:
> *DCHECK_GT
> DCHECK_LE(new_cell_index, last_cell_index_)

Done.

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

https://codereview.chromium.org/2160613002/diff/40001/src/heap/marking.h#newc...
src/heap/marking.h:109: void SetRange(uint32_t start_index, uint32_t end_index)
{
On 2016/07/19 13:23:32, ulan wrote:
> Let's add a comment describing that it sets bits in range [start_index,
> end_index): end is not included.

Done.

https://codereview.chromium.org/2160613002/diff/40001/src/heap/spaces.cc
File src/heap/spaces.cc (right):

https://codereview.chromium.org/2160613002/diff/40001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1278: Free(current_top, old_linear_size);
On 2016/07/19 13:23:32, ulan wrote:
> Let's move it after clearing the range, so that we do not free marked area.

Done.

https://codereview.chromium.org/2160613002/diff/40001/src/heap/spaces.cc#newc...
src/heap/spaces.cc:1285: page->IncrementLiveBytes(static_cast<int>(current_top -
current_limit));
On 2016/07/19 13:23:32, ulan wrote:
> To avoid large unsigned to int conversions, let's do
> -static_cast<int>(current_limit - current_top)
> 
> Alternatively, how about adding page->DecrementLiveBytes, so that the
intention
> is obvious from code?

Done.

Good idea. I do that in the follow-up CL that is doing the symmetric
decrementing of live bytes.

[Hannes Payer (out of office), 2016-07-19 15:29:38]

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

[commit-bot: I haz the power, 2016-07-19 15:29:47]

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

[ulan, 2016-07-19 15:36:55]

https://codereview.chromium.org/2160613002/diff/60001/src/heap/mark-compact-i...
File src/heap/mark-compact-inl.h (right):

https://codereview.chromium.org/2160613002/diff/60001/src/heap/mark-compact-i...
src/heap/mark-compact-inl.h:195: if (object != nullptr) return object;
What happens on left/right trimming? Are we going to return the free space
filler as a live object?

[commit-bot: I haz the power, 2016-07-19 15:56:45]

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

[commit-bot: I haz the power, 2016-07-19 15:56:47]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2016-07-21 08:25:43]

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

[commit-bot: I haz the power, 2016-07-21 08:25:48]

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-07-21 08:28:12]

https://codereview.chromium.org/2160613002/diff/60001/src/heap/mark-compact-i...
File src/heap/mark-compact-inl.h (right):

https://codereview.chromium.org/2160613002/diff/60001/src/heap/mark-compact-i...
src/heap/mark-compact-inl.h:195: if (object != nullptr) return object;
On 2016/07/19 15:36:54, ulan wrote:
> What happens on left/right trimming? Are we going to return the free space
> filler as a live object?

They would survive right now. I changed it to clearing mark bits when writing
fillers.

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

https://codereview.chromium.org/2160613002/diff/80001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3075: if (incremental_marking()->black_allocation() &&
We could templetize this function since sometimes we know that we do not need to
clear mark bits, e.g. in the sweeper.

[ulan, 2016-07-21 08:41:57]

lgtm

[commit-bot: I haz the power, 2016-07-21 08:58:34]

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

[commit-bot: I haz the power, 2016-07-21 08:58:35]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9438)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Hannes Payer (out of office), 2016-07-21 17:10:50]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2016-07-21 17:10:58]

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-07-21 17:35:19]

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

[commit-bot: I haz the power, 2016-07-21 17:35:20]

Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9478)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Hannes Payer (out of office), 2016-07-21 19:07:44]

The CQ bit was checked by hpayer@chromium.org

[Hannes Payer (out of office), 2016-07-21 19:07:44]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org
Link to the patchset: https://codereview.chromium.org/2160613002/#ps100001
(title: "perform cast after writing map")

[commit-bot: I haz the power, 2016-07-21 19:07:53]

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-07-21 19:08:02]

The CQ bit was unchecked by hpayer@chromium.org

[Hannes Payer (out of office), 2016-07-21 19:46:50]

Description was changed from

==========
[heap] Remove black pages and use black areas instead.

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

to

==========
[heap] Remove black pages and use black areas instead.

BUG=630386
LOG=n
==========

[Hannes Payer (out of office), 2016-07-22 06:57:36]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2016-07-22 06:57:44]

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-07-22 06:59:30]

Description was changed from

==========
[heap] Remove black pages and use black areas instead.

BUG=630386
LOG=n
==========

to

==========
[heap] Remove black pages and use black areas instead.

BUG=630386
LOG=n
==========

[commit-bot: I haz the power, 2016-07-22 06:59:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-22 07:02:33]

Description was changed from

==========
[heap] Remove black pages and use black areas instead.

BUG=630386
LOG=n
==========

to

==========
[heap] Remove black pages and use black areas instead.

BUG=630386
LOG=n

Committed: https://crrev.com/b008a0d5a3db80a854cb93d9c94d67bf2d780f2c
Cr-Commit-Position: refs/heads/master@{#37967}
==========

[commit-bot: I haz the power, 2016-07-22 07:02:34]

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

[Yang, 2016-07-25 09:46:07]

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

The reason for reverting is: Suspected to cause crbug.com/630969.