[heap] Add atomics to mark bit operations.

[heap] Add atomics to mark bit operations.

BUG=chromium:694255
Review-Url: https://codereview.chromium.org/2492263002
Cr-Commit-Position: refs/heads/master@{#43679}
Committed: https://chromium.googlesource.com/v8/v8/+/b643bb771303d33d1ab80577cc58106415df8c0e

[Hannes Payer (out of office), 2016-11-24 13:45:29]

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

[commit-bot: I haz the power, 2016-11-24 13:45:40]

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-24 13:50:26]

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

[commit-bot: I haz the power, 2016-11-24 13:50:27]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16775)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13160)

[Hannes Payer (out of office), 2017-02-20 17:00:14]

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

[commit-bot: I haz the power, 2017-02-20 17:00:21]

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, 2017-02-20 17:03:57]

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

[commit-bot: I haz the power, 2017-02-20 17:03:58]

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

[Hannes Payer (out of office), 2017-02-21 08:57:52]

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

[commit-bot: I haz the power, 2017-02-21 08:57:54]

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, 2017-02-21 09:01:24]

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

[commit-bot: I haz the power, 2017-02-21 09:01:25]

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

[Hannes Payer (out of office), 2017-02-21 10:15:16]

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

[commit-bot: I haz the power, 2017-02-21 10:15:25]

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), 2017-02-21 10:17:15]

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

[commit-bot: I haz the power, 2017-02-21 10:17:25]

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, 2017-02-21 10:44:10]

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

[commit-bot: I haz the power, 2017-02-21 10:44:13]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2017-02-21 12:57:13]

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

[commit-bot: I haz the power, 2017-02-21 12:57:17]

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, 2017-02-21 13:22:53]

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

[commit-bot: I haz the power, 2017-02-21 13:22:54]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2017-03-06 12:40:30]

Description was changed from

==========
[heap] Benchmarking atomic ops.

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

to

==========
[heap] Add atomics to mark bit operations.

BUG=chromium:694255
==========

[Hannes Payer (out of office), 2017-03-06 13:35:07]

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

[Hannes Payer (out of office), 2017-03-06 13:35:08]

This CL only changes the Marking and ObjectMarking operaters to be ATOMIC or
NON_ATOMIC.

The range functions still need to be changed, separate CL.

[Michael Lippautz, 2017-03-06 13:52:54]

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

https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact....
src/heap/mark-compact.h:50: template <MarkBit::ModificationMode mod_mode =
MarkBit::NON_ATOMIC,
nit: Can you spell out the names, e.g. modication_mode, marking_mode.

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

https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new...
src/heap/marking.h:16: typedef uint32_t CellType;
nit: Can we have a static_assert that sizeof(CellType) == sizeof(Atomic32)?

https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new...
src/heap/marking.h:18: enum ModificationMode { ATOMIC, NON_ATOMIC };
You could make an 
  enum class ModicationMode
outside of MarkBit. We often had problems with enums nested in classes.

[ulan, 2017-03-06 14:51:33]

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

https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact....
src/heap/mark-compact.h:50: template <MarkBit::ModificationMode mod_mode =
MarkBit::NON_ATOMIC,
On 2017/03/06 13:52:54, Michael Lippautz wrote:
> nit: Can you spell out the names, e.g. modication_mode, marking_mode.

+1, how about calling the parameter AccessMode?

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

https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new...
src/heap/marking.h:321: return !mark_bit.Get<mode>() &&
mark_bit.Next().Get<mode>();
Atomic version can return false positive if between the first bit read and the
second bit read the object goes from white to black in concurrent thread.

Can we get rid of this function?

https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new...
src/heap/marking.h:354: INLINE(static bool MarkWhite(MarkBit markbit)) {
All functions that move the color in black -> grey -> white direction are not
safe for concurrent usage (e.g. concurrent MarkWhite() and GreyToBlack() on a
grey color can make impossible color).

Should we disallow atomic versions of MarkWhite and BlackToGrey?

[Hannes Payer (out of office), 2017-03-08 14:12:57]

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

https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact....
src/heap/mark-compact.h:50: template <MarkBit::ModificationMode mod_mode =
MarkBit::NON_ATOMIC,
On 2017/03/06 13:52:54, Michael Lippautz wrote:
> nit: Can you spell out the names, e.g. modication_mode, marking_mode.

Done.

https://codereview.chromium.org/2492263002/diff/170001/src/heap/mark-compact....
src/heap/mark-compact.h:50: template <MarkBit::ModificationMode mod_mode =
MarkBit::NON_ATOMIC,
On 2017/03/06 13:52:54, Michael Lippautz wrote:
> nit: Can you spell out the names, e.g. modication_mode, marking_mode.

Done.

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

https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new...
src/heap/marking.h:16: typedef uint32_t CellType;
On 2017/03/06 13:52:54, Michael Lippautz wrote:
> nit: Can we have a static_assert that sizeof(CellType) == sizeof(Atomic32)?

Done.

https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new...
src/heap/marking.h:18: enum ModificationMode { ATOMIC, NON_ATOMIC };
On 2017/03/06 13:52:54, Michael Lippautz wrote:
> You could make an 
>   enum class ModicationMode
> outside of MarkBit. We often had problems with enums nested in classes.

I kind of like it to keep it in the MarkBit class for that particular case. I do
not see any problems with that.

https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new...
src/heap/marking.h:321: return !mark_bit.Get<mode>() &&
mark_bit.Next().Get<mode>();
On 2017/03/06 14:51:33, ulan wrote:
> Atomic version can return false positive if between the first bit read and the
> second bit read the object goes from white to black in concurrent thread.
> 
> Can we get rid of this function?

This function is still useful. I made it safe for concurrent usage.

https://codereview.chromium.org/2492263002/diff/170001/src/heap/marking.h#new...
src/heap/marking.h:354: INLINE(static bool MarkWhite(MarkBit markbit)) {
On 2017/03/06 14:51:33, ulan wrote:
> All functions that move the color in black -> grey -> white direction are not
> safe for concurrent usage (e.g. concurrent MarkWhite() and GreyToBlack() on a
> grey color can make impossible color).
> 
> Should we disallow atomic versions of MarkWhite and BlackToGrey?

Done.

Why is BlackToGrey a problem?

[ulan, 2017-03-08 14:42:01]

lgtm

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

https://codereview.chromium.org/2492263002/diff/210001/src/heap/marking.h#new...
src/heap/marking.h:384: DCHECK(mode == MarkBit::ATOMIC || IsBlack(markbit));
STATIC_ASSERT(mode == MarkBit::NON_ATOMIC)

[Hannes Payer (out of office), 2017-03-08 15:55:00]

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

https://codereview.chromium.org/2492263002/diff/210001/src/heap/marking.h#new...
src/heap/marking.h:384: DCHECK(mode == MarkBit::ATOMIC || IsBlack(markbit));
On 2017/03/08 14:42:01, ulan wrote:
> STATIC_ASSERT(mode == MarkBit::NON_ATOMIC)

Done.

[Hannes Payer (out of office), 2017-03-08 15:57:27]

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

[commit-bot: I haz the power, 2017-03-08 15:57:35]

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, 2017-03-08 16:23:19]

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

[commit-bot: I haz the power, 2017-03-08 16:23:20]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2017-03-08 16:29:29]

The CQ bit was checked by hpayer@chromium.org

[Hannes Payer (out of office), 2017-03-08 16:29:30]

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/2492263002/#ps230001
(title: "comment")

[commit-bot: I haz the power, 2017-03-08 16:29:39]

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, 2017-03-08 16:31:19]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1488990569714430,
"parent_rev": "e7be85daf69bbf9adef4841eae2839e4075f40f7", "commit_rev":
"b643bb771303d33d1ab80577cc58106415df8c0e"}

[commit-bot: I haz the power, 2017-03-08 16:31:23]

Description was changed from

==========
[heap] Add atomics to mark bit operations.

BUG=chromium:694255
==========

to

==========
[heap] Add atomics to mark bit operations.

BUG=chromium:694255

Review-Url: https://codereview.chromium.org/2492263002
Cr-Commit-Position: refs/heads/master@{#43679}
Committed:
https://chromium.googlesource.com/v8/v8/+/b643bb771303d33d1ab80577cc58106415d...
==========

[commit-bot: I haz the power, 2017-03-08 16:31:24]

Committed patchset #13 (id:230001) as
https://chromium.googlesource.com/v8/v8/+/b643bb771303d33d1ab80577cc58106415d...