[oilpan] MarkingVisitor cleanup

- Avoid nullptr checks when the visitor knows that it should be non-null
- Avoid duplicate methods: MarkHeader and MarkHeaderNoTracing are enough
  represent all use cases
- Style cleanups

Bug: chromium:757440
Change-Id: I8f4ed806670a1d4e9967a16596b520cb8133cfc3
Reviewed-on: https://chromium-review.googlesource.com/959963Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542799}

third_party/WebKit/Source/platform/heap/HeapAllocator.h

@@ -174,11 +174,6 @@ class PLATFORM_EXPORT HeapAllocator {
     return ThreadHeap::IsHeapObjectAlive(object);
   }
 
-  template <typename VisitorDispatcher>
-  static void MarkNoTracing(VisitorDispatcher visitor, const void* t) {
-    visitor->MarkNoTracing(t);
-  }
-
   template <typename VisitorDispatcher, typename T, typename Traits>
   static void Trace(VisitorDispatcher visitor, T& t) {
     TraceCollectionIfEnabled<Traits::kWeakHandlingFlag, T, Traits>::Trace(

third_party/WebKit/Source/platform/heap/HeapPage.h

@@ -205,6 +205,7 @@ class PLATFORM_EXPORT HeapObjectHeader {
   bool IsMarked() const;
   void Mark();
   void Unmark();
+  bool TryMark();
 
   // The payload starts directly after the HeapObjectHeader, and the payload
   // size does not include the sizeof(HeapObjectHeader).
@@ -1102,6 +1103,14 @@ NO_SANITIZE_ADDRESS inline void HeapObjectHeader::Unmark() {
   encoded_ &= ~kHeaderMarkBitMask;
 }
 
+NO_SANITIZE_ADDRESS inline bool HeapObjectHeader::TryMark() {
+  CheckHeader();
+  if (encoded_ & kHeaderMarkBitMask)
+    return false;
+  encoded_ |= kHeaderMarkBitMask;
+  return true;
+}
+
 NO_SANITIZE_ADDRESS inline bool BasePage::IsValid() const {
   return GetMagic() == magic_;
 }

third_party/WebKit/Source/platform/heap/MarkingVisitor.cpp

@@ -27,21 +27,20 @@ MarkingVisitor::MarkingVisitor(ThreadState* state, MarkingMode marking_mode)
 MarkingVisitor::~MarkingVisitor() = default;
 
 void MarkingVisitor::MarkNoTracingCallback(Visitor* visitor, void* object) {
-  // TODO(mlippautz): Remove cast;
-  reinterpret_cast<MarkingVisitor*>(visitor)->MarkNoTracing(object);
+  reinterpret_cast<MarkingVisitor*>(visitor)->MarkHeaderNoTracing(
+      HeapObjectHeader::FromPayload(object));
 }
 
 void MarkingVisitor::RegisterWeakCallback(void* closure,
                                           WeakCallback callback) {
-  DCHECK(GetMarkingMode() != kWeakProcessing);
   // We don't want to run weak processings when taking a snapshot.
-  if (GetMarkingMode() == kSnapshotMarking)
+  if (marking_mode_ == kSnapshotMarking)
     return;
   Heap().PushWeakCallback(closure, callback);
 }
 
 void MarkingVisitor::RegisterBackingStoreReference(void* slot) {
-  if (GetMarkingMode() != kGlobalMarkingWithCompaction)
+  if (marking_mode_ != kGlobalMarkingWithCompaction)
     return;
   Heap().RegisterMovingObjectReference(
       reinterpret_cast<MovableReference*>(slot));
@@ -50,7 +49,7 @@ void MarkingVisitor::RegisterBackingStoreReference(void* slot) {
 void MarkingVisitor::RegisterBackingStoreCallback(void* backing_store,
                                                   MovingObjectCallback callback,
                                                   void* callback_data) {
-  if (GetMarkingMode() != kGlobalMarkingWithCompaction)
+  if (marking_mode_ != kGlobalMarkingWithCompaction)
     return;
   Heap().RegisterMovingObjectCallback(
       reinterpret_cast<MovableReference>(backing_store), callback,
@@ -61,7 +60,6 @@ bool MarkingVisitor::RegisterWeakTable(
     const void* closure,
     EphemeronCallback iteration_callback,
     EphemeronCallback iteration_done_callback) {
-  DCHECK(GetMarkingMode() != kWeakProcessing);
   Heap().RegisterWeakTable(const_cast<void*>(closure), iteration_callback,
                            iteration_done_callback);
   return true;

third_party/WebKit/Source/platform/heap/MarkingVisitor.h

@@ -21,9 +21,6 @@ class PLATFORM_EXPORT MarkingVisitor final : public Visitor {
     // This visitor just marks objects and ignores weak processing.
     // This is used for GCType=TakeSnapshot.
     kSnapshotMarking,
-    // This visitor is used to trace objects during weak processing.
-    // This visitor is allowed to trace only already marked objects.
-    kWeakProcessing,
     // Perform global marking along with preparing for additional sweep
     // compaction of heap arenas afterwards. Compared to the GlobalMarking
     // visitor, this visitor will also register references to objects
@@ -38,33 +35,20 @@ class PLATFORM_EXPORT MarkingVisitor final : public Visitor {
   MarkingVisitor(ThreadState*, MarkingMode);
   virtual ~MarkingVisitor();
 
-  inline MarkingMode GetMarkingMode() const { return marking_mode_; }
-
   // Marking implementation.
 
-  // This method marks an object and adds it to the set of objects that should
-  // have their trace method called. Since not all objects have vtables we have
-  // to have the callback as an explicit argument, but we can use the templated
-  // one-argument mark method above to automatically provide the callback
-  // function.
-  inline void Mark(const void* object_pointer, TraceCallback);
-
-  // Used to mark objects during conservative scanning.
+  // Marks an object and adds a tracing callback for processing of the object.
   inline void MarkHeader(HeapObjectHeader*, TraceCallback);
-  inline void MarkHeaderNoTracing(HeapObjectHeader*);
 
-  // Marks the header of an object. Is used for eagerly tracing of objects.
-  inline bool EnsureMarked(const void* pointer);
-
-  // Used for eagerly marking objects and for delayed marking of backing stores
-  // when the actual payload is processed differently, e.g., by weak handling.
-  inline void MarkNoTracing(const void* pointer) {
-    Mark(pointer, reinterpret_cast<TraceCallback>(0));
-  }
+  // Try to mark an object without tracing. Returns true when the object was not
+  // marked upon calling.
+  inline bool MarkHeaderNoTracing(HeapObjectHeader*);
 
   // Implementation of the visitor interface. See above for descriptions.
 
   void Visit(void* object, TraceDescriptor desc) final {
+    DCHECK(object);
+    DCHECK(desc.base_object_payload);
     // Default mark method of the trait just calls the two-argument mark
     // method on the visitor. The second argument is the static trace method
     // of the trait, which by default calls the instance method
@@ -87,13 +71,15 @@ class PLATFORM_EXPORT MarkingVisitor final : public Visitor {
       // that lead to many recursions.
       DCHECK(Heap().GetStackFrameDepth().IsAcceptableStackUse());
       if (LIKELY(Heap().GetStackFrameDepth().IsSafeToRecurse())) {
-        if (EnsureMarked(desc.base_object_payload)) {
+        if (MarkHeaderNoTracing(
+                HeapObjectHeader::FromPayload(desc.base_object_payload))) {
           desc.callback(this, desc.base_object_payload);
         }
         return;
       }
     }
-    Mark(desc.base_object_payload, desc.callback);
+    MarkHeader(HeapObjectHeader::FromPayload(desc.base_object_payload),
+               desc.callback);
   }
 
   void VisitBackingStoreStrongly(void* object,
@@ -112,7 +98,6 @@ class PLATFORM_EXPORT MarkingVisitor final : public Visitor {
   void VisitBackingStoreWeakly(void* object,
                                void** object_slot,
                                TraceDescriptor desc) final {
-    DCHECK(GetMarkingMode() != kWeakProcessing);
     RegisterBackingStoreReference(object_slot);
     Heap().PushPostMarkingCallback(object, &MarkNoTracingCallback);
   }
@@ -136,54 +121,25 @@ class PLATFORM_EXPORT MarkingVisitor final : public Visitor {
   const MarkingMode marking_mode_;
 };
 
-inline void MarkingVisitor::MarkHeader(HeapObjectHeader* header,
-                                       TraceCallback callback) {
+inline bool MarkingVisitor::MarkHeaderNoTracing(HeapObjectHeader* header) {
   DCHECK(header);
-  if (header->IsMarked())
-    return;
-
-  DCHECK(ThreadState::Current()->IsInGC() ||
-         ThreadState::Current()->IsIncrementalMarking());
-  DCHECK(GetMarkingMode() != kWeakProcessing);
-
-  const void* object_pointer = header->Payload();
+  DCHECK(State()->IsInGC() || State()->IsIncrementalMarking());
   // A GC should only mark the objects that belong in its heap.
-  DCHECK(&PageFromObject(object_pointer)->Arena()->GetThreadState()->Heap() ==
-         &Heap());
+  DCHECK_EQ(State(),
+            PageFromObject(header->Payload())->Arena()->GetThreadState());
 
-  header->Mark();
-
-  if (callback)
-    Heap().PushTraceCallback(const_cast<void*>(object_pointer), callback);
+  return header->TryMark();
 }
 
-inline void MarkingVisitor::Mark(const void* object_pointer,
-                                 TraceCallback callback) {
-  if (!object_pointer)
-    return;
-  HeapObjectHeader* header = HeapObjectHeader::FromPayload(object_pointer);
-  MarkHeader(header, callback);
-}
-
-inline void MarkingVisitor::MarkHeaderNoTracing(HeapObjectHeader* header) {
-  MarkHeader(header, reinterpret_cast<TraceCallback>(0));
-}
-
-inline bool MarkingVisitor::EnsureMarked(const void* object_pointer) {
-  if (!object_pointer)
-    return false;
+inline void MarkingVisitor::MarkHeader(HeapObjectHeader* header,
+                                       TraceCallback callback) {
+  DCHECK(header);
+  DCHECK(callback);
 
-  HeapObjectHeader* header = HeapObjectHeader::FromPayload(object_pointer);
-  if (header->IsMarked())
-    return false;
-#if DCHECK_IS_ON()
-  MarkNoTracing(object_pointer);
-#else
-  // Inline what the above markNoTracing() call expands to,
-  // so as to make sure that we do get all the benefits (asserts excepted.)
-  header->Mark();
-#endif
-  return true;
+  if (MarkHeaderNoTracing(header)) {
+    Heap().PushTraceCallback(reinterpret_cast<void*>(header->Payload()),
+                             callback);
+  }
 }
 
 }  // namespace blink

third_party/WebKit/Source/platform/heap/Visitor.h

@@ -73,8 +73,8 @@ class PLATFORM_EXPORT Visitor {
   explicit Visitor(ThreadState* state) : state_(state) {}
   virtual ~Visitor() = default;
 
-  inline ThreadState* GetState() const { return state_; }
-  inline ThreadHeap& Heap() const { return GetState()->Heap(); }
+  inline ThreadState* State() const { return state_; }
+  inline ThreadHeap& Heap() const { return state_->Heap(); }
 
   // Static visitor implementation forwarding to dynamic interface.