Fix external size accounting for prologue weak persistent handles.

runtime/vm/dart_api_state.h

Index: runtime/vm/dart_api_state.h
===================================================================
--- runtime/vm/dart_api_state.h	(revision 33504)
+++ runtime/vm/dart_api_state.h	(working copy)
@@ -9,6 +9,7 @@
 
 #include "platform/thread.h"
 #include "platform/utils.h"
+#include "vm/bitfield.h"
 #include "vm/dart_api_impl.h"
 #include "vm/flags.h"
 #include "vm/growable_array.h"
@@ -215,10 +216,11 @@
 
   void SetExternalSize(intptr_t size, Isolate* isolate) {
     ASSERT(size >= 0);
-    external_size_ = Utils::RoundUp(size, kObjectAlignment);
+    set_external_size(Utils::RoundUp(size, kObjectAlignment));
+    UpdateExternalNewSpaceBit();
     // TODO(koda): On repeated/large external allocations for existing objects,
     // without any intervening normal allocation, GC will not trigger.
-    isolate->heap()->AllocateExternal(external_size_, SpaceForExternal());
+    isolate->heap()->AllocateExternal(external_size(), SpaceForExternal());
   }
 
   // Called when the referent becomes unreachable.
@@ -228,25 +230,20 @@
   }
 
   // Called when the referent has moved, potentially between generations.
-  void UpdateRelocated(Heap::Space before, Isolate* isolate) {
-    isolate->heap()->FreeExternal(external_size_, before);
-    isolate->heap()->AllocateExternal(external_size_, SpaceForExternal());
+  void UpdateRelocated(Isolate* isolate) {
+    Heap::Space after = SpaceForExternal();
+    Heap::Space before = WasPromotedSinceLastCall() ? Heap::kNew : after;

[siva, 2014/03/11 16:35:01]

Since we only promote new to old why can't this check just be a simple
if (IsSetNewSpaceBit() && (SpaceForExternal() == Heap::kOld)) {
  FreeExternal(external_size(), Heap::kNew);
  AllocateExternal(external_size(), Heap::kOld);
  ResetExternalNewSpaceBit();
}

[koda, 2014/03/11 17:35:45]

Done.

My reasoning was to localize the assumption that we have exactly two generations
and make the code as general as possible, but in retrospect, that might be more
trouble than it's worth.

+    isolate->heap()->FreeExternal(external_size(), before);
+    isolate->heap()->AllocateExternal(external_size(), after);

[siva, 2014/03/11 16:35:01]

if before == after it not necessary to do this Free/Allocate stuff right?

[koda, 2014/03/11 17:35:45]

Done.

   }
 
   // Idempotent. Called when the handle is explicitly deleted or the
   // referent becomes unreachable.
   void EnsureFreeExternal(Isolate* isolate) {
-    isolate->heap()->FreeExternal(external_size_, SpaceForExternal());
-    external_size_ = 0;
+    isolate->heap()->FreeExternal(external_size(), SpaceForExternal());
+    set_external_size(0);
   }
 
-  // Returns the space to charge for the external size.
-  Heap::Space SpaceForExternal() const {
-    // Non-heap and VM-heap objects count as old space here.
-    return (raw_->IsHeapObject() && raw_->IsNewObject()) ?
-           Heap::kNew : Heap::kOld;
-  }
-
   static bool IsPrologueWeakPersistentHandle(Dart_WeakPersistentHandle handle) {
     uword addr = reinterpret_cast<uword>(handle);
     return (addr & kWeakPersistentTagMask) == kPrologueWeakPersistentTag;
@@ -261,12 +258,20 @@
     kWeakPersistentTagMask = 1,
   };
 
+  // This part of external_data_ is the number of externally allocated bytes.
+  // TODO(koda): Measure size in words instead.
+  class ExternalSizeBits : public BitField<intptr_t, 1, kBitsPerWord - 1> {};
+  // This bit of external_data_ is true if the referent was created in new
+  // space and WasPromotedSinceLastCall has not yet detected any promotion.
+  class ExternalNewSpaceBit : public BitField<bool, 0, 1> {};
+  // TODO(koda): Use bitfield also for the prologue tag.
+
   friend class FinalizablePersistentHandles;
 
   FinalizablePersistentHandle()
       : raw_(NULL),
         peer_(NULL),
-        external_size_(0),
+        external_data_(0),
         callback_(NULL) { }
   ~FinalizablePersistentHandle() { }
 
@@ -303,13 +308,43 @@
   void Clear() {
     raw_ = Object::null();
     peer_ = NULL;
-    external_size_ = 0;
+    external_data_ = 0;
     callback_ = NULL;
   }
 
+  intptr_t external_size() const {
+    return ExternalSizeBits::decode(external_data_);
+  }
+
+  void set_external_size(intptr_t size) {
+    external_data_ = ExternalSizeBits::update(size, external_data_);
+  }
+
+  void UpdateExternalNewSpaceBit() {
+    bool value = (SpaceForExternal() == Heap::kNew);
+    external_data_ = ExternalNewSpaceBit::update(value, external_data_);
+  }

[siva, 2014/03/11 16:35:01]

The use of UpdateExternalNewSpaceBit to both set and reset the bit makes it hard
to read, why not have explicit:

SetExternalNewSpaceBit() { ... }
ResetExternalNewSpaceBit() { ... }

SetExternalNewSpaceBit is called in SetExternalSize() and
ResetExternalNewSpaceBit is called in WasPromotedSizeLastCall();

[koda, 2014/03/11 17:35:45]

Done.

+
+  bool WasPromotedSinceLastCall() {
+    if (ExternalNewSpaceBit::decode(external_data_) &&
+        SpaceForExternal() == Heap::kOld) {
+      UpdateExternalNewSpaceBit();
+      return true;
+    } else {
+      return false;
+    }
+  }
+
+  // Returns the space to charge for the external size.
+  Heap::Space SpaceForExternal() const {
+    // Non-heap and VM-heap objects count as old space here.
+    return (raw_->IsHeapObject() && raw_->IsNewObject()) ?
+           Heap::kNew : Heap::kOld;
+  }
+
   RawObject* raw_;
   void* peer_;
-  intptr_t external_size_;
+  uword external_data_;
   Dart_WeakPersistentHandleFinalizer callback_;
   DISALLOW_ALLOCATION();  // Allocated through AllocateHandle methods.
   DISALLOW_COPY_AND_ASSIGN(FinalizablePersistentHandle);