Extend Handle API with MarkIndependent.

src/heap.cc

Index: src/heap.cc
diff --git a/src/heap.cc b/src/heap.cc
index a08e3a3d9f60f4eb4ba33799b826f023f14a12f2..0c5eecc97d405437d8568fe955ecaeb57c799e65 100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -771,8 +771,7 @@ bool Heap::PerformGarbageCollection(GarbageCollector collector,
 
   isolate_->counters()->objs_since_last_young()->Set(0);
 
-  if (collector == MARK_COMPACTOR) {
-    DisableAssertNoAllocation allow_allocation;
+  { DisableAssertNoAllocation allow_allocation;
     GCTracer::Scope scope(tracer, GCTracer::Scope::EXTERNAL);
     next_gc_likely_to_collect_more =
         isolate_->global_handles()->PostGarbageCollectionProcessing();

[Vitaly Repeshko, 2011/05/16 13:42:11]

This can invoke callbacks for pending dependent handles left over from previous
major GCs. (Because some of them can trigger GCs.) Given that the
prologue/epilogue callbacks won't be called it might be a problem.

[Vyacheslav Egorov (Chromium), 2011/05/16 14:15:46]

Yes it can and I am failing to see how it is a problem.

Once the handle enters a PENDING state his callback _will_ be called no matter
what happens to object in subsequent collections. The object e.g. can become
strongly reachable, but this would not affect anything.

If you know places in bindings where this can be a problem --- we should fix
them.

[Vitaly Repeshko, 2011/05/16 14:31:22]

If I'm right about the prologue/epilogue callbacks not being called, it could be
a problem because weak callbacks used to be called between the prologue/epilogue
pair.

[Vyacheslav Egorov (Chromium), 2011/05/16 14:55:34]

We should not mark those handles that depend on prologue/epilogue callbacks as
independent. The whole idea of independent handle is that it, well, does not
depend on anything.

[Vitaly Repeshko, 2011/05/16 15:04:03]

Right, but please not that there can be _dependent_ callbacks still pending.
Those can rely on the prologue/epilogue callbacks.

[Vyacheslav Egorov (Chromium), 2011/05/16 15:19:58]

Argh. This is true. If those callbacks rely on being called inside "brackets" of
callbacks they will blow up.

Ok, for the sake of keeping old code up and running I will not invoke any
callbacks for dependent handles in scavenge.

@@ -935,6 +934,12 @@ void Heap::CheckNewSpaceExpansionCriteria() {
 }
 
 
+static bool IsUnscavengedHeapObject(Heap* heap, Object** p) {
+  return heap->InNewSpace(*p) &&
+      !HeapObject::cast(*p)->map_word().IsForwardingAddress();
+}
+
+
 void Heap::Scavenge() {
 #ifdef DEBUG
   if (FLAG_enable_slow_asserts) VerifyNonPointerSpacePointers();
@@ -1029,6 +1034,11 @@ void Heap::Scavenge() {
   scavenge_visitor.VisitPointer(BitCast<Object**>(&global_contexts_list_));
 
   new_space_front = DoScavenge(&scavenge_visitor, new_space_front);
+  isolate_->global_handles()->IdentifyWeakIndependentHandles(

[Vitaly Repeshko, 2011/05/16 13:42:11]

Scanning the linked list of handles was really slow in some benchmarks. We
should merge these two passes because the scavenges should be as fast as
possible.

[Vyacheslav Egorov (Chromium), 2011/05/16 14:15:46]

If there are two weak handles that point to the same object them both should
have their callbacks called.

Thus merging these two passes together would require additional mark somewhere
on the object to distinguish objects evacuated by normal scavenging and objects
evacuated by scavenging of weak independent handles. We probably can use the
second bit from map word but this would complicate things.

[Vitaly Repeshko, 2011/05/16 14:31:22]

Ahh, this is annoying. Can we at least inline the "identify" callback?

[Vyacheslav Egorov (Chromium), 2011/05/16 14:55:34]

To inline identify callback we need to move IdentifyWeakIndependentHandles into
global-handles-inl.h.

I can do that but I really think it's premature optimization. I have not seen
any significant degradation on a test that had dozens of thousands of
independent handles created and destroyed every second.

Alternatively we can have separate lists for independent and dependent handles
so we will not pay anything for dependent handles when we don't need to iterate
over them.

[Vitaly Repeshko, 2011/05/16 21:40:37]

I do remember seeing scavenge time increase when I experimented with a similar
approach for DOM nodes. But it was more than a year ago.

I've got an idea on how to merge the passes: we can detect when an object was
evacuated by comparing its forwarded address with a saved new space top.

Feel free to postpone doing this (if it works at all) or other optimizations.

[Vyacheslav Egorov (Chromium), 2011/05/17 11:55:31]

Very neat! But unfortunately will not work for promoted objects as they have no
imposed linear order. 

I will postpone optimization.

+      &IsUnscavengedHeapObject);
+  isolate_->global_handles()->IterateWeakIndependentRoots(&scavenge_visitor);
+  new_space_front = DoScavenge(&scavenge_visitor, new_space_front);
+
 
   UpdateNewSpaceReferencesInExternalStringTable(
       &UpdateNewSpaceReferenceInExternalStringTableEntry);
@@ -4492,7 +4502,8 @@ void Heap::IterateRoots(ObjectVisitor* v, VisitMode mode) {
 void Heap::IterateWeakRoots(ObjectVisitor* v, VisitMode mode) {
   v->VisitPointer(reinterpret_cast<Object**>(&roots_[kSymbolTableRootIndex]));
   v->Synchronize("symbol_table");
-  if (mode != VISIT_ALL_IN_SCAVENGE) {
+  if (mode != VISIT_ALL_IN_SCAVENGE &&
+      mode != VISIT_ALL_IN_SWEEP_NEWSPACE) {
     // Scavenge collections have special processing for this.
     external_string_table_.Iterate(v);
   }
@@ -4528,16 +4539,26 @@ void Heap::IterateStrongRoots(ObjectVisitor* v, VisitMode mode) {
   // Iterate over the builtin code objects and code stubs in the
   // heap. Note that it is not necessary to iterate over code objects
   // on scavenge collections.
-  if (mode != VISIT_ALL_IN_SCAVENGE) {
+  if (mode != VISIT_ALL_IN_SCAVENGE &&
+      mode != VISIT_ALL_IN_SWEEP_NEWSPACE) {
     isolate_->builtins()->IterateBuiltins(v);
   }
   v->Synchronize("builtins");
 
   // Iterate over global handles.
-  if (mode == VISIT_ONLY_STRONG) {
-    isolate_->global_handles()->IterateStrongRoots(v);
-  } else {
-    isolate_->global_handles()->IterateAllRoots(v);
+  switch (mode) {
+    case VISIT_ONLY_STRONG:
+      isolate_->global_handles()->IterateStrongRoots(v);
+      break;
+    case VISIT_ALL_IN_SCAVENGE:
+      isolate_->global_handles()->IterateStrongAndDependentRoots(v);
+      break;
+    case VISIT_ALL_IN_SWEEP_NEWSPACE:
+    case VISIT_ALL:
+      isolate_->global_handles()->IterateAllRoots(v);
+      break;
+    default:

[Vitaly Repeshko, 2011/05/16 13:42:11]

Gcc should be able to statically catch an unhanded case error if default is
omitted.

[Vyacheslav Egorov (Chromium), 2011/05/16 14:15:46]

Ok.

+      UNREACHABLE();
   }
   v->Synchronize("globalhandles");