[heap] Make non-atomic markbit operations consistent with atomic ones.

[heap] Make non-atomic markbit operations consistent with atomic ones.

Now non-atomic color transition operations return a boolean indicating
whether the transition succeeded or not.

This allows to replace color check and transition operations with a
single transition operation. For example:

if (IsWhite(object)) {
  WhiteToBlack(object);
  Foo();
}

becomes

if (WhiteToBlack(object)) {
  Foo();
}

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

[ulan, 2017-05-02 18:20:29]

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

[ulan, 2017-05-02 18:20:31]

ptal

https://codereview.chromium.org/2857713002/diff/1/src/heap/heap.cc
File src/heap/heap.cc (left):

https://codereview.chromium.org/2857713002/diff/1/src/heap/heap.cc#oldcode4290
src/heap/heap.cc:4290: incremental_marking()->MarkGrey(object);
MarkGrey is defined as WhiteToGreyAndPush. The latter has more descriptive name.

[Michael Lippautz, 2017-05-03 07:48:39]

lgtm

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

https://codereview.chromium.org/2857713002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.h:195: bool WhiteToGreyAndPush(HeapObject* obj);
nit: Please add a comment indicating what the return value means, i.e., that
it's about the white to grey and not the push.

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

https://codereview.chromium.org/2857713002/diff/40001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:2156: string_table->IteratePrefix(visitor);
:)

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

https://codereview.chromium.org/2857713002/diff/40001/src/heap/marking.h#newc...
src/heap/marking.h:60: base::Atomic32 old_value = *cell_;
Please add a comment on the class indicating that the return values indicate
whether the transition cleared<->set was a success and not the fact that the bit
is set/cleared

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

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

https://codereview.chromium.org/2857713002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:878: USE(ignored);
Do you want to DCHECK ignored?

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

https://codereview.chromium.org/2857713002/diff/40001/src/heap/marking.h#newc...
src/heap/marking.h:62: return (old_value & mask_) == 0;
Why did you change that? The non atomic transition will always succeed.

https://codereview.chromium.org/2857713002/diff/40001/src/heap/marking.h#newc...
src/heap/marking.h:92: return (old_value & mask_) == mask_;
Same here, this should always be true.

[ulan, 2017-05-03 08:51:56]

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

https://codereview.chromium.org/2857713002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.h:195: bool WhiteToGreyAndPush(HeapObject* obj);
On 2017/05/03 07:48:39, Michael Lippautz wrote:
> nit: Please add a comment indicating what the return value means, i.e., that
> it's about the white to grey and not the push.

Done.

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

https://codereview.chromium.org/2857713002/diff/40001/src/heap/marking.h#newc...
src/heap/marking.h:60: base::Atomic32 old_value = *cell_;
On 2017/05/03 07:48:39, Michael Lippautz wrote:
> Please add a comment on the class indicating that the return values indicate
> whether the transition cleared<->set was a success and not the fact that the
bit
> is set/cleared

Done.

https://codereview.chromium.org/2857713002/diff/40001/src/heap/marking.h#newc...
src/heap/marking.h:62: return (old_value & mask_) == 0;
On 2017/05/03 08:29:05, Hannes Payer wrote:
> Why did you change that? The non atomic transition will always succeed.
The success means that the bit was 0 and is changed to 1 by the Set like the
ATOMIC case. I added a comment in the class.

Set<NON_ATOMIC> and Set<ATOMIC> should differ only in the atomicity of the
access and should have the same semantic for single threaded execution.
Otherwise, we need to code paths in incremental marker depending on atomicity.

https://codereview.chromium.org/2857713002/diff/40001/src/heap/marking.h#newc...
src/heap/marking.h:92: return (old_value & mask_) == mask_;
On 2017/05/03 08:29:05, Hannes Payer wrote:
> Same here, this should always be true.

Replied above.

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

lgtm

[ulan, 2017-05-03 12:04:08]

The CQ bit was checked by ulan@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/2857713002/#ps100001
(title: "make comment on class")

[commit-bot: I haz the power, 2017-05-03 12:04:17]

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-05-03 12:29:46]

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

[commit-bot: I haz the power, 2017-05-03 12:29:47]

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/25374)
  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...)

[ulan, 2017-05-03 19:40:10]

Hannes, Michael: I added special handling for overlapping markbits in
IncrementalMarking::TransferMark.

[ulan, 2017-05-03 19:41:02]

The CQ bit was checked by ulan@chromium.org

[ulan, 2017-05-03 19:41:03]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/2857713002/#ps140001
(title: "more fixes to TranserMark")

[commit-bot: I haz the power, 2017-05-03 19:41:14]

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-05-03 19:43:03]

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

[commit-bot: I haz the power, 2017-05-03 19:43:04]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/40395)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/26994)

[ulan, 2017-05-03 19:58:09]

The CQ bit was checked by ulan@chromium.org

[ulan, 2017-05-03 19:58:09]

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

[commit-bot: I haz the power, 2017-05-03 19:58:14]

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-05-03 20:29:47]

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

[commit-bot: I haz the power, 2017-05-03 20:29:48]

Try jobs failed on following builders:
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_linux_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng_...)

[ulan, 2017-05-04 11:15:02]

The CQ bit was checked by ulan@chromium.org

[ulan, 2017-05-04 11:15:02]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/2857713002/#ps200001
(title: "fix overlap check")

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

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-05-04 11:44:15]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493896502233250,
"parent_rev": "662aa425bac00f468d2b2e91a149b5f35e2a4ad3", "commit_rev":
"dd37366fb56a57be5ef36ed0c2b3c7893cd6c6d8"}

[commit-bot: I haz the power, 2017-05-04 11:44:22]

Description was changed from

==========
[heap] Make non-atomic markbit operations consistent with atomic ones.

Now non-atomic color transition operations return a boolean indicating
whether the transition succeeded or not.

This allows to replace color check and transition operations with a
single transition operation. For example:

if (IsWhite(object)) {
  WhiteToBlack(object);
  Foo();
}

becomes

if (WhiteToBlack(object)) {
  Foo();
}

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

to

==========
[heap] Make non-atomic markbit operations consistent with atomic ones.

Now non-atomic color transition operations return a boolean indicating
whether the transition succeeded or not.

This allows to replace color check and transition operations with a
single transition operation. For example:

if (IsWhite(object)) {
  WhiteToBlack(object);
  Foo();
}

becomes

if (WhiteToBlack(object)) {
  Foo();
}

BUG=chromium:694255

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

[commit-bot: I haz the power, 2017-05-04 11:44:23]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/v8/v8/+/dd37366fb56a57be5ef36ed0c2b3c7893cd...

[Michael Achenbach, 2017-05-04 20:49:58]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2017-05-04 20:49:59]

Looks like this blocks the roll:
https://codereview.chromium.org/2857423002/

[Michael Achenbach, 2017-05-05 06:37:08]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2859093004/ by machenbach@chromium.org.

The reason for reverting is: Seems to block the roll:
https://codereview.chromium.org/2857423002/.

[Michael Achenbach, 2017-05-05 08:23:05]

Please add to CL description on reland:

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

[ulan, 2017-05-05 08:33:11]

On 2017/05/05 08:23:05, Michael Achenbach wrote:
> Please add to CL description on reland:
> 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

Thank you for looking into it, Michael!