[heap] Use atomic marking operations in incremental marking if

[heap] Use atomic marking operations in incremental marking if
concurrent marking is enabled.

This patch adds kAtomicity flag to IncrementalMarking that is set
depending on the concurrent marking compile time flag.

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

[ulan, 2017-05-02 19:20:35]

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

[ulan, 2017-05-02 19:20:35]

ptal

[Michael Lippautz, 2017-05-03 07:49:52]

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.cc:48:
DCHECK(!ObjectMarking::IsImpossible<kAtomicity>(obj, marking_state(obj)));
Is IsImpossible safe for concurrent marking? I remember that there were issues
with it.

[ulan, 2017-05-03 09:00:28]

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.cc:48:
DCHECK(!ObjectMarking::IsImpossible<kAtomicity>(obj, marking_state(obj)));
On 2017/05/03 07:49:51, Michael Lippautz wrote:
> Is IsImpossible safe for concurrent marking? I remember that there were issues
with it.
It is not safe indeed. I will remove bunch of dchecks that are invalidated by
concurrent marking in a separate CL.

[Hannes Payer (out of office), 2017-05-03 10:08:53]

lgtm

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.cc:48:
DCHECK(!ObjectMarking::IsImpossible<kAtomicity>(obj, marking_state(obj)));
On 2017/05/03 09:00:28, ulan wrote:
> On 2017/05/03 07:49:51, Michael Lippautz wrote:
> > Is IsImpossible safe for concurrent marking? I remember that there were
issues
> with it.
> It is not safe indeed. I will remove bunch of dchecks that are invalidated by
> concurrent marking in a separate CL.

Is the current implementation of the check not correct? Why?

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

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.h:165: static const MarkBit::AccessMode kAtomicity
= MarkBit::AccessMode::ATOMIC;
I am not sure if this is the right mechanism here. There may be cases where we
do not run concurrent marking but use only incremental instead. The decision
will happen at runtime. Are you planning to change that later and using this as
a preparation CL only?

[ulan, 2017-05-03 12:09:31]

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.cc:48:
DCHECK(!ObjectMarking::IsImpossible<kAtomicity>(obj, marking_state(obj)));
On 2017/05/03 10:08:53, Hannes Payer wrote:
> On 2017/05/03 09:00:28, ulan wrote:
> > On 2017/05/03 07:49:51, Michael Lippautz wrote:
> > > Is IsImpossible safe for concurrent marking? I remember that there were
> issues
> > with it.
> > It is not safe indeed. I will remove bunch of dchecks that are invalidated
by
> > concurrent marking in a separate CL.
> 
> Is the current implementation of the check not correct? Why?

In general IsImpossible can produce false positive with concurrent marking.
Scenario:
1. The object is white.
2. IsImpossible function reads the first bit as 0.
3. In the meantime the concurrent thread transitions the object from white to
black.
4. IsImpossible function reads the second bit as 1 and returns true.

But in the BaseRecordWrite, the object is not white. So IsImpossible should
work.

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

https://codereview.chromium.org/2857743002/diff/1/src/heap/incremental-markin...
src/heap/incremental-marking.h:165: static const MarkBit::AccessMode kAtomicity
= MarkBit::AccessMode::ATOMIC;
On 2017/05/03 10:08:53, Hannes Payer wrote:
> I am not sure if this is the right mechanism here. There may be cases where we
> do not run concurrent marking but use only incremental instead. The decision
> will happen at runtime. Are you planning to change that later and using this
as
> a preparation CL only?

As discussed offline, in future we will have two IncrementalMarking instances
and use virtual dispatch on Step.

[ulan, 2017-05-04 11:47:31]

The CQ bit was checked by ulan@chromium.org

[ulan, 2017-05-04 11:47:31]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/2857743002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-04 11:47:38]

CQ is trying da patch.

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

[Michael Lippautz, 2017-05-04 11:49:28]

lgtm

[commit-bot: I haz the power, 2017-05-04 12:37:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493898451483530,
"parent_rev": "5c433ec500e0de20737624bceb0ade69d67a9153", "commit_rev":
"1f2c3596e9c5dd86039560ff890ad68b4fdad0a1"}

[commit-bot: I haz the power, 2017-05-04 12:37:14]

Description was changed from

==========
[heap] Use atomic marking operations in incremental marking if
concurrent marking is enabled.

This patch adds kAtomicity flag to IncrementalMarking that is set
depending on the concurrent marking compile time flag.

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

to

==========
[heap] Use atomic marking operations in incremental marking if
concurrent marking is enabled.

This patch adds kAtomicity flag to IncrementalMarking that is set
depending on the concurrent marking compile time flag.

BUG=chromium:694255

Review-Url: https://codereview.chromium.org/2857743002
Cr-Commit-Position: refs/heads/master@{#45091}
Committed:
https://chromium.googlesource.com/v8/v8/+/1f2c3596e9c5dd86039560ff890ad68b4fd...
==========

[commit-bot: I haz the power, 2017-05-04 12:37:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/1f2c3596e9c5dd86039560ff890ad68b4fd...