Process weak reference sets when a scavenge invokes the API callbacks.

runtime/vm/scavenger.cc

Index: runtime/vm/scavenger.cc
diff --git a/runtime/vm/scavenger.cc b/runtime/vm/scavenger.cc
index 29bb59b8aafe037466e61f1d59fbe0317a431172..148134ab269c7b5879b101bf7950ecd2561995d1 100644
--- a/runtime/vm/scavenger.cc
+++ b/runtime/vm/scavenger.cc
@@ -240,6 +240,80 @@ void Scavenger::IterateRoots(Isolate* isolate,
 }
 
 
+bool Scavenger::IsUnreachable(RawObject** p) {

[Ivan Posva, 2012/04/06 21:54:06]

Can you change this to Scavenger::IsReachable() to make this a bit more readable
and to avoid the double negative where this is used?

[cshapiro, 2012/04/06 22:24:31]

Yes, will do.

[cshapiro, 2012/04/10 02:40:31]

While this is very rational adjustment when considered in isolation, it has been
so long since I made the change that I lost the context for the naming.

The unreachability test was supposed to be shared with the handle visiting code.
 The use of Unreachable as opposed to Reachable has some consistency given the
two uses.  Anyway, I have made the fix in VisitHandle and so this looks a bit
less strange (no more isolated double negative).  If you still think this looks
backward, I can submit a clean-up that flips the naming here and in the marker.

+  RawObject* raw_obj = *p;
+  if (!raw_obj->IsHeapObject()) {

[Ivan Posva, 2012/04/06 21:54:06]

This is surprising to me to say the least. Having a Smi in the weak reference
set keeps all of the references alive? Should a Smi even be allowed here and if
it is, shouldn't it be the other way around as the object 1 will keep the set
alive, while the result of 1 * 1.0 will not.

[cshapiro, 2012/04/06 22:24:31]

I am not sure why this is surprising.  Smi instances are an instance of just
another subclass of Integer.  This code ensures that these objects are always
considered reachable just like 

1)         0xFFFFFFFF, a Mint on 32-bit, a Smi on 64-bit 
2) 0x7FFFFFFFFFFFFFFF, a Mint on 64-bit, a Bigint on 32-bit  
3) 0xFFFFFFFFFFFFFFFF, a Bigint

would be.

[cshapiro, 2012/04/10 02:40:31]

I thought about this over the weekend.  Numbers are immutable and this is often
an opportunity for time and space optimizations.  Looking at it another way,
relying on values produced by different expressions being not equal is not
something that we are likely to make guarantees about.

Consider the following situations: "1.0 == 1.0" is always true, "1.0 === 1.0" is
true today under a broad range of circumstances, and "1.0 === (1 * 1.0)" is not
presently true but may become true in the future.

Reflecting on your example, Smi 1 does keep a set alive and 1.0 might as well,
depending on its provenance.

I am not sure there is an ideal resolution here.  This might be best to revisit
in a subsequent change as other parts of the code such as the handle processing
and the marker treats Smi objects as always live.

+    return false;
+  }
+  if (!raw_obj->IsNewObject()) {

[siva, 2012/04/06 17:12:25]

Wouldn't just this check be sufficient, it also implicitly implies something
passing this is a heap object.

[cshapiro, 2012/04/06 22:24:31]

Objects that become unreferenced after a new space collection are stored in the
from space part of new space.  Looking at this another way, at this point in
time, all live objects are in to space.

What this test tells us is that the object is in a part of the heap that was not
condemned by this collection and the object is guaranteed live.

What we want to know here is that the object did not survive the from-space
to-space flip.  We find that out on line 252.

+    return false;
+  }
+  uword raw_addr = RawObject::ToAddr(raw_obj);
+  if (!from_->Contains(raw_addr)) {
+    return false;
+  }
+  uword header = *reinterpret_cast<uword*>(raw_addr);
+  if (IsForwarding(header)) {
+    uword new_addr = ForwardedAddr(header);
+    *p = RawObject::FromAddr(new_addr);
+    return false;
+  }
+  return true;
+}
+
+
+void Scavenger::IterateWeakReferences(Isolate* isolate,
+                                      ObjectPointerVisitor* visitor) {
+  ApiState* state = isolate->api_state();
+  ASSERT(state != NULL);
+  while (true) {
+    WeakReference* queue = state->delayed_weak_references();
+    if (queue == NULL) {
+      // The delay queue is empty therefore no clean-up is required.
+      return;
+    }
+    state->set_delayed_weak_references(NULL);
+    while (queue != NULL) {
+      WeakReference* reference = WeakReference::Pop(&queue);
+      ASSERT(reference != NULL);
+      bool is_unreachable = true;
+      // Test each key object for reachability.  If a key object is
+      // reachable, all value objects should be marked.
+      for (intptr_t k = 0; k < reference->num_keys(); ++k) {
+        if (!IsUnreachable(reference->get_key(k))) {
+          for (intptr_t v = 0; v < reference->num_values(); ++v) {
+            visitor->VisitPointer(reference->get_value(v));
+          }
+          is_unreachable = false;
+          delete reference;
+          break;
+        }
+      }
+      // If all key objects are unreachable put the reference on a
+      // delay queue.  This reference will be revisited if another
+      // reference is marked.
+      if (is_unreachable) {
+        state->DelayWeakReference(reference);
+      }
+    }
+    if ((FirstObjectStart() < top_) || PromotedStackHasMore()) {
+      ProcessToSpace(visitor);
+    } else {
+      // Break out of the loop if there has been no forward process.
+      break;
+    }
+  }
+  // Deallocate any unmarked references on the delay queue.
+  if (state->delayed_weak_references() != NULL) {
+    WeakReference* queue = state->delayed_weak_references();
+    state->set_delayed_weak_references(NULL);
+    while (queue != NULL) {
+      delete WeakReference::Pop(&queue);
+    }
+  }
+}
+
+
 void Scavenger::IterateWeakRoots(Isolate* isolate,
                                  HandleVisitor* visitor,
                                  bool visit_prologue_weak_persistent_handles) {
@@ -303,6 +377,7 @@ void Scavenger::Scavenge(bool invoke_api_callbacks) {
   Prologue(isolate, invoke_api_callbacks);
   IterateRoots(isolate, &visitor, !invoke_api_callbacks);
   ProcessToSpace(&visitor);
+  IterateWeakReferences(isolate, &visitor);
   ScavengerWeakVisitor weak_visitor(this);
   IterateWeakRoots(isolate, &weak_visitor, invoke_api_callbacks);
   Epilogue(isolate, invoke_api_callbacks);