- Removed the kWatchedBit and the associated weak property handling

runtime/vm/gc_marker.cc

Index: runtime/vm/gc_marker.cc
diff --git a/runtime/vm/gc_marker.cc b/runtime/vm/gc_marker.cc
index 2c2643ec8295fa2778a28acc90f3af0c5ba7c41e..93c9e7cff8be847fc204568d2694826dc9e61498 100644
--- a/runtime/vm/gc_marker.cc
+++ b/runtime/vm/gc_marker.cc
@@ -4,10 +4,6 @@
 
 #include "vm/gc_marker.h"
 
-#include <map>
-#include <utility>
-#include <vector>
-
 #include "vm/allocation.h"
 #include "vm/dart_api_state.h"
 #include "vm/isolate.h"
@@ -25,64 +21,6 @@
 
 namespace dart {
 
-class DelaySet {
- private:
-  typedef std::multimap<RawObject*, RawWeakProperty*> Map;
-  typedef std::pair<RawObject*, RawWeakProperty*> MapEntry;
-
- public:
-  DelaySet() : mutex_(new Mutex()) {}
-  ~DelaySet() { delete mutex_; }
-
-  // After atomically setting the watched bit on a white key (see
-  // EnsureWatchedIfWhitewhich; this means the mark bit cannot be set
-  // without observing the watched bit), this method atomically
-  // inserts raw_weak if its key is *still* white, so that any future
-  // call to VisitValuesForKey is guaranteed to include its
-  // value. Returns true on success, and false if the key is no longer white.
-  bool InsertIfWhite(RawWeakProperty* raw_weak) {
-    MutexLocker ml(mutex_);
-    RawObject* raw_key = raw_weak->ptr()->key_;
-    if (raw_key->IsMarked()) return false;
-    // The key was white *after* acquiring the lock. Thus any future call to
-    // VisitValuesForKey is guaranteed to include the entry inserted below.
-    delay_set_.insert(std::make_pair(raw_key, raw_weak));
-    return true;
-  }
-
-  void ClearReferences() {
-    MutexLocker ml(mutex_);
-    for (Map::iterator it = delay_set_.begin(); it != delay_set_.end(); ++it) {
-      ASSERT(!it->first->IsMarked());
-      WeakProperty::Clear(it->second);
-    }
-  }
-
-  // Visit all values with a key equal to raw_obj, which must already be marked.
-  void VisitValuesForKey(RawObject* raw_obj, ObjectPointerVisitor* visitor) {
-    ASSERT(raw_obj->IsMarked());
-    // Extract the range into a temporary vector to iterate over it
-    // while delay_set_ may be modified.
-    std::vector<MapEntry> temp_copy;
-    {
-      MutexLocker ml(mutex_);
-      std::pair<Map::iterator, Map::iterator> ret =
-          delay_set_.equal_range(raw_obj);
-      temp_copy.insert(temp_copy.end(), ret.first, ret.second);
-      delay_set_.erase(ret.first, ret.second);
-    }
-    for (std::vector<MapEntry>::iterator it = temp_copy.begin();
-         it != temp_copy.end(); ++it) {
-      it->second->VisitPointers(visitor);
-    }
-  }
-
- private:
-  Map delay_set_;
-  Mutex* mutex_;
-};
-
-
 class SkippedCodeFunctions : public ZoneAllocated {
  public:
   SkippedCodeFunctions() {}
@@ -206,7 +144,6 @@ class MarkingVisitorBase : public ObjectPointerVisitor {
                  Heap* heap,
                  PageSpace* page_space,
                  MarkingStack* marking_stack,
-                 DelaySet* delay_set,
                  SkippedCodeFunctions* skipped_code_functions)
       : ObjectPointerVisitor(isolate),
         thread_(Thread::Current()),
@@ -216,7 +153,7 @@ class MarkingVisitorBase : public ObjectPointerVisitor {
         class_stats_size_(isolate->class_table()->NumCids()),
         page_space_(page_space),
         work_list_(marking_stack),
-        delay_set_(delay_set),
+        delayed_weak_properties_(NULL),
         visiting_old_object_(NULL),
         skipped_code_functions_(skipped_code_functions),
         marked_bytes_(0) {
@@ -248,15 +185,44 @@ class MarkingVisitorBase : public ObjectPointerVisitor {
       return false;
     }
     do {
-      VisitingOldObject(raw_obj);
-      const intptr_t class_id = raw_obj->GetClassId();
-      if (class_id != kWeakPropertyCid) {
-        marked_bytes_ += raw_obj->VisitPointers(this);
-      } else {
-        RawWeakProperty* raw_weak = reinterpret_cast<RawWeakProperty*>(raw_obj);
-        marked_bytes_ += raw_weak->Size();
-        ProcessWeakProperty(raw_weak);
+      do {
+        // First drain the marking stacks.
+        VisitingOldObject(raw_obj);
+        const intptr_t class_id = raw_obj->GetClassId();
+        if (class_id != kWeakPropertyCid) {
+          marked_bytes_ += raw_obj->VisitPointers(this);
+        } else {
+          RawWeakProperty* raw_weak =
+              reinterpret_cast<RawWeakProperty*>(raw_obj);
+          marked_bytes_ += ProcessWeakProperty(raw_weak);
+        }
+        raw_obj = work_list_.Pop();
+      } while (raw_obj != NULL);
+
+      // Marking stack is empty.
+      // Process all the pending weak properties in this visitor.
+      RawWeakProperty* cur_weak = delayed_weak_properties_;
+      delayed_weak_properties_ = NULL;
+      while (cur_weak != NULL) {
+        uword next_weak = cur_weak->ptr()->next_;
+        RawObject* raw_key = cur_weak->ptr()->key_;
+        // Reset the next pointer in the weak property.
+        cur_weak->ptr()->next_ = 0;
+        if (raw_key->IsMarked()) {
+          // The key is marked so we make sure to properly visit all pointers
+          // originating from this weak property.
+          VisitingOldObject(cur_weak);
+          cur_weak->VisitPointers(this);
+        } else {
+          // Requeue this weak property to be handled later.
+          EnqueueWeakProperty(cur_weak);
+        }
+        // Advance to next weak property in the queue.
+        cur_weak = reinterpret_cast<RawWeakProperty*>(next_weak);
       }
+
+      // Check whether any further work was pushed either by other markers or
+      // by the handling of weak properties.
       raw_obj = work_list_.Pop();
     } while (raw_obj != NULL);
     VisitingOldObject(NULL);
@@ -278,47 +244,49 @@ class MarkingVisitorBase : public ObjectPointerVisitor {
     skipped_code_functions_->Add(func);
   }
 
-  // If unmarked, sets the watch bit and returns true.
-  // If marked, does nothing and returns false.
-  static bool EnsureWatchedIfWhite(RawObject* obj) {
-    if (!sync) {
-      if (obj->IsMarked()) return false;
-      if (!obj->IsWatched()) obj->SetWatchedBitUnsynchronized();
-      return true;
-    }
-    uword tags = obj->ptr()->tags_;
-    uword old_tags;
-    do {
-      old_tags = tags;
-      if (RawObject::MarkBit::decode(tags)) return false;
-      if (RawObject::WatchedBit::decode(tags)) return true;
-      uword new_tags = RawObject::WatchedBit::update(true, old_tags);
-      tags = AtomicOperations::CompareAndSwapWord(
-          &obj->ptr()->tags_, old_tags, new_tags);
-    } while (tags != old_tags);
-    return true;
+  void EnqueueWeakProperty(RawWeakProperty* raw_weak) {
+    ASSERT(raw_weak->IsHeapObject());
+    ASSERT(raw_weak->IsOldObject());
+    ASSERT(raw_weak->IsWeakProperty());
+    ASSERT(raw_weak->IsMarked());
+    ASSERT(raw_weak->ptr()->next_ == 0);
+    raw_weak->ptr()->next_ = reinterpret_cast<uword>(delayed_weak_properties_);
+    delayed_weak_properties_ = raw_weak;
   }
 
-  void ProcessWeakProperty(RawWeakProperty* raw_weak) {
+  intptr_t ProcessWeakProperty(RawWeakProperty* raw_weak) {
     // The fate of the weak property is determined by its key.
     RawObject* raw_key = raw_weak->ptr()->key_;
     if (raw_key->IsHeapObject() &&
         raw_key->IsOldObject() &&
-        EnsureWatchedIfWhite(raw_key) &&
-        delay_set_->InsertIfWhite(raw_weak)) {
-      // Key was white.  Delayed the weak property.
-    } else {
-      // Key is gray or black.  Make the weak property black.
-      raw_weak->VisitPointers(this);
+        !raw_key->IsMarked()) {
+      // Key was white. Enqueue the weak property.
+      EnqueueWeakProperty(raw_weak);
+      return raw_weak->Size();
     }
+    // Key is gray or black. Make the weak property black.
+    return raw_weak->VisitPointers(this);
   }
 
   // Called when all marking is complete.
   void Finalize() {
     work_list_.Finalize();
+    // Detach code from functions.
     if (skipped_code_functions_ != NULL) {
       skipped_code_functions_->DetachCode();
     }
+    // Clear pending weak properties.
+    RawWeakProperty* cur_weak = delayed_weak_properties_;
+    delayed_weak_properties_ = NULL;
+    intptr_t weak_properties_cleared = 0;
+    while (cur_weak != NULL) {
+      uword next_weak = cur_weak->ptr()->next_;
+      cur_weak->ptr()->next_ = 0;
+      WeakProperty::Clear(cur_weak);

[siva, 2016/05/23 20:35:33]

Maybe set next_ to 0 inside Clear so reseting of all fields of RawWeakProperty
is on place?

+      weak_properties_cleared++;
+      // Advance to next weak property in the queue.
+      cur_weak = reinterpret_cast<RawWeakProperty*>(next_weak);
+    }
   }
 
   void VisitingOldObject(RawObject* obj) {
@@ -335,15 +303,10 @@ class MarkingVisitorBase : public ObjectPointerVisitor {
 
     // Push the marked object on the marking stack.
     ASSERT(raw_obj->IsMarked());
-    const bool is_watched = raw_obj->IsWatched();
     // We acquired the mark bit => no other task is modifying the header.
     // TODO(koda): For concurrent mutator, this needs synchronization. Consider
     // clearing these bits already in the CAS for the mark bit.
     raw_obj->ClearRememberedBitUnsynchronized();
-    raw_obj->ClearWatchedBitUnsynchronized();
-    if (is_watched) {
-      delay_set_->VisitValuesForKey(raw_obj, this);
-    }
     work_list_.Push(raw_obj);
   }
 
@@ -430,7 +393,7 @@ class MarkingVisitorBase : public ObjectPointerVisitor {
   GrowableArray<intptr_t> class_stats_size_;
   PageSpace* page_space_;
   MarkerWorkList work_list_;
-  DelaySet* delay_set_;
+  RawWeakProperty* delayed_weak_properties_;
   RawObject* visiting_old_object_;
   SkippedCodeFunctions* skipped_code_functions_;
   uintptr_t marked_bytes_;
@@ -574,7 +537,6 @@ class MarkTask : public ThreadPool::Task {
            Heap* heap,
            PageSpace* page_space,
            MarkingStack* marking_stack,
-           DelaySet* delay_set,
            ThreadBarrier* barrier,
            bool collect_code,
            intptr_t task_index,
@@ -585,7 +547,6 @@ class MarkTask : public ThreadPool::Task {
         heap_(heap),
         page_space_(page_space),
         marking_stack_(marking_stack),
-        delay_set_(delay_set),
         barrier_(barrier),
         collect_code_(collect_code),
         task_index_(task_index),
@@ -605,7 +566,7 @@ class MarkTask : public ThreadPool::Task {
       SkippedCodeFunctions* skipped_code_functions =
           collect_code_ ? new(zone) SkippedCodeFunctions() : NULL;
       SyncMarkingVisitor visitor(isolate_, heap_, page_space_, marking_stack_,
-                                 delay_set_, skipped_code_functions);
+                                 skipped_code_functions);
       // Phase 1: Iterate over roots and drain marking stack in tasks.
       marker_->IterateRoots(isolate_, &visitor, task_index_, num_tasks_);
       do {
@@ -653,7 +614,6 @@ class MarkTask : public ThreadPool::Task {
   Heap* heap_;
   PageSpace* page_space_;
   MarkingStack* marking_stack_;
-  DelaySet* delay_set_;
   ThreadBarrier* barrier_;
   bool collect_code_;
   const intptr_t task_index_;
@@ -695,7 +655,6 @@ void GCMarker::MarkObjects(Isolate* isolate,
     StackZone stack_zone(Thread::Current());
     Zone* zone = stack_zone.GetZone();
     MarkingStack marking_stack;
-    DelaySet delay_set;
     marked_bytes_ = 0;
     const int num_tasks = FLAG_marker_tasks;
     if (num_tasks == 0) {
@@ -703,7 +662,7 @@ void GCMarker::MarkObjects(Isolate* isolate,
       SkippedCodeFunctions* skipped_code_functions =
           collect_code ? new(zone) SkippedCodeFunctions() : NULL;
       UnsyncMarkingVisitor mark(isolate, heap_, page_space, &marking_stack,
-                                &delay_set, skipped_code_functions);
+                                skipped_code_functions);
       IterateRoots(isolate, &mark, 0, 1);
       mark.DrainMarkingStack();
       MarkingWeakVisitor mark_weak;
@@ -720,31 +679,21 @@ void GCMarker::MarkObjects(Isolate* isolate,
       for (intptr_t i = 0; i < num_tasks; ++i) {
         MarkTask* mark_task =
             new MarkTask(this, isolate, heap_, page_space, &marking_stack,
-                         &delay_set, &barrier, collect_code,
+                         &barrier, collect_code,
                          i, num_tasks, &num_busy);
         ThreadPool* pool = Dart::thread_pool();
         pool->Run(mark_task);
       }
       barrier.Sync();
 
-      // Phase 2: Weak processing and follow-up marking on main thread.
-      SkippedCodeFunctions* skipped_code_functions =
-          collect_code ? new(zone) SkippedCodeFunctions() : NULL;
-      SyncMarkingVisitor mark(isolate, heap_, page_space, &marking_stack,
-                              &delay_set, skipped_code_functions);
+      // Phase 2: Weak processing on main thread.
       MarkingWeakVisitor mark_weak;
       IterateWeakRoots(isolate, &mark_weak);
       barrier.Sync();
 
       // Phase 3: Finalize results from all markers (detach code, etc.).
-      if (FLAG_log_marker_tasks) {
-        THR_Print("Main thread marked %" Pd " bytes.\n",
-                  mark.marked_bytes());
-      }
-      FinalizeResultsFrom(&mark);
       barrier.Exit();
     }
-    delay_set.ClearReferences();
     ProcessWeakTables(page_space);
     ProcessObjectIdTable(isolate);
   }