Fix external size accounting for prologue weak persistent handles.

runtime/vm/dart_api_state.h

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #ifndef VM_DART_API_STATE_H_
 #define VM_DART_API_STATE_H_
 
 #include "include/dart_api.h"
 
 #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"
 #include "vm/handles.h"
 #include "vm/object.h"
 #include "vm/os.h"
 #include "vm/raw_object.h"
 #include "vm/visitor.h"
 
 #include "vm/handles_impl.h"
@@ skipping 186 matching lines @@
     uword addr = reinterpret_cast<uword>(this);
     return reinterpret_cast<Dart_WeakPersistentHandle>(
         addr | kPrologueWeakPersistentTag);
   }
   Dart_WeakPersistentHandle apiHandle() {
     return reinterpret_cast<Dart_WeakPersistentHandle>(this);
   }
 
   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.
   void UpdateUnreachable(Isolate* isolate, bool is_prologue_weak) {
     EnsureFreeExternal(isolate);
     Finalize(isolate, this, is_prologue_weak);
   }
 
   // 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;
-  }
-
-  // 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;
+    isolate->heap()->FreeExternal(external_size(), SpaceForExternal());
+    set_external_size(0);
   }
 
   static bool IsPrologueWeakPersistentHandle(Dart_WeakPersistentHandle handle) {
     uword addr = reinterpret_cast<uword>(handle);
     return (addr & kWeakPersistentTagMask) == kPrologueWeakPersistentTag;
   }
   static FinalizablePersistentHandle* Cast(Dart_WeakPersistentHandle handle);
 
  private:
   enum {
     kWeakPersistentTag = 0,
     kPrologueWeakPersistentTag = 1,
     kWeakPersistentTagSize = 1,
     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() { }
 
   static void Finalize(Isolate* isolate,
                        FinalizablePersistentHandle* handle,
                        bool is_prologue_weak) {
     Dart_WeakPersistentHandleFinalizer callback = handle->callback();
     if (callback != NULL) {
       void* peer = handle->peer();
       handle->Clear();
@@ skipping 16 matching lines @@
     ASSERT(!raw_->IsHeapObject());
   }
   void FreeHandle(FinalizablePersistentHandle* free_list) {
     Clear();
     SetNext(free_list);
   }
 
   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);
 };
 
 
 // Local handles repository structure.
 static const int kLocalHandleSizeInWords = sizeof(LocalHandle) / kWordSize;
 static const int kLocalHandlesPerChunk = 64;
 static const int kOffsetOfRawPtrInLocalHandle = 0;
@@ skipping 535 matching lines @@
           ApiNativeScope::Current()->zone()) {}
   ApiGrowableArray()
       : BaseGrowableArray<T, ValueObject>(
           ApiNativeScope::Current()->zone()) {}
 };
 
 
 }  // namespace dart
 
 #endif  // VM_DART_API_STATE_H_