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

src/heap/marking.h

 // Copyright 2016 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef V8_MARKING_H
 #define V8_MARKING_H
 
 #include "src/base/atomic-utils.h"
 #include "src/utils.h"
 
@@ skipping 39 matching lines @@
   base::Atomic32* cell_;
   base::Atomic32 mask_;
 
   friend class IncrementalMarking;
   friend class ConcurrentMarkingMarkbits;
   friend class Marking;
 };
 
 template <>
 inline bool MarkBit::Set<MarkBit::NON_ATOMIC>() {
-  *cell_ |= mask_;
-  return true;
+  base::Atomic32 old_value = *cell_;

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

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

[ulan, 2017/05/03 08:51:55]

Done.

+  *cell_ = old_value | mask_;
+  return (old_value & mask_) == 0;

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

Why did you change that? The non atomic transition will always succeed.

[ulan, 2017/05/03 08:51:55]

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.

 }
 
 template <>
 inline bool MarkBit::Set<MarkBit::ATOMIC>() {
   base::Atomic32 old_value;
   base::Atomic32 new_value;
   do {
     old_value = base::NoBarrier_Load(cell_);
     if (old_value & mask_) return false;
     new_value = old_value | mask_;
   } while (base::Release_CompareAndSwap(cell_, old_value, new_value) !=
            old_value);
   return true;
 }
 
 template <>
 inline bool MarkBit::Get<MarkBit::NON_ATOMIC>() {
   return (base::NoBarrier_Load(cell_) & mask_) != 0;
 }
 
 template <>
 inline bool MarkBit::Get<MarkBit::ATOMIC>() {
   return (base::Acquire_Load(cell_) & mask_) != 0;
 }
 
 template <>
 inline bool MarkBit::Clear<MarkBit::NON_ATOMIC>() {
-  *cell_ &= ~mask_;
-  return true;
+  base::Atomic32 old_value = *cell_;
+  *cell_ = old_value & ~mask_;
+  return (old_value & mask_) == mask_;

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

Same here, this should always be true.

[ulan, 2017/05/03 08:51:55]

Replied above.

 }
 
 template <>
 inline bool MarkBit::Clear<MarkBit::ATOMIC>() {
   base::Atomic32 old_value;
   base::Atomic32 new_value;
   do {
     old_value = base::NoBarrier_Load(cell_);
     if (!(old_value & mask_)) return false;
     new_value = old_value & ~mask_;
@@ skipping 304 matching lines @@
 
   template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
   INLINE(static bool BlackToGrey(MarkBit markbit)) {
     STATIC_ASSERT(mode == MarkBit::NON_ATOMIC);
     DCHECK(IsBlack(markbit));
     return markbit.Next().Clear<mode>();
   }
 
   template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
   INLINE(static bool WhiteToGrey(MarkBit markbit)) {
-    DCHECK(mode == MarkBit::ATOMIC || IsWhite(markbit));
     return markbit.Set<mode>();
   }
 
-  // Warning: this method is not safe in general in concurrent scenarios.
-  // If you know that nobody else will change the bits on the given location
-  // then you may use it.
   template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
-  INLINE(static void WhiteToBlack(MarkBit markbit)) {
-    DCHECK(mode == MarkBit::ATOMIC || IsWhite(markbit));
-    markbit.Set<mode>();
-    markbit.Next().Set<mode>();
+  INLINE(static bool WhiteToBlack(MarkBit markbit)) {
+    return markbit.Set<mode>() && markbit.Next().Set<mode>();
   }
 
   template <MarkBit::AccessMode mode = MarkBit::NON_ATOMIC>
   INLINE(static bool GreyToBlack(MarkBit markbit)) {
-    DCHECK(mode == MarkBit::ATOMIC || IsGrey(markbit));
-    return markbit.Next().Set<mode>();
+    return markbit.Get<mode>() && markbit.Next().Set<mode>();
   }
 
   enum ObjectColor {
     BLACK_OBJECT,
     WHITE_OBJECT,
     GREY_OBJECT,
     IMPOSSIBLE_COLOR
   };
 
   static const char* ColorName(ObjectColor color) {
@@ skipping 19 matching lines @@
   }
 
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(Marking);
 };
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_MARKING_H_