Reimplemented zone memory tracking to avoid race conditions that were causing crashes in the previo…

runtime/vm/zone.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 RUNTIME_VM_ZONE_H_
 #define RUNTIME_VM_ZONE_H_
 
 #include "platform/utils.h"
 #include "vm/allocation.h"
 #include "vm/handles.h"
@@ skipping 46 matching lines @@
   // TODO(zra): Remove these calls and replace them with calls to OS::SCreate
   // and OS::VSCreate.
   // These calls are deprecated. Do not add further calls to these functions.
   // instead use OS::SCreate and OS::VSCreate.
   // Make a zone-allocated string based on printf format and args.
   char* PrintToString(const char* format, ...) PRINTF_ATTRIBUTE(2, 3);
   char* VPrint(const char* format, va_list args);
 
   // Compute the total size of this zone. This includes wasted space that is
   // due to internal fragmentation in the segments.
-  intptr_t SizeInBytes() const;
+  uintptr_t SizeInBytes() const;
 
   // Computes the amount of space used in the zone.
   intptr_t CapacityInBytes() const;
 
+  // Computes the amount of space remaining in the zone.
+  intptr_t FreeCapacityInBytes() const;
+
   // Structure for managing handles allocation.
   VMHandles* handles() { return &handles_; }
 
   void VisitObjectPointers(ObjectPointerVisitor* visitor);
 
   Zone* previous() const { return previous_; }
 
-#ifndef PRODUCT
-  void PrintJSON(JSONStream* stream) const;
-#endif
-
  private:
   Zone();
   ~Zone();  // Delete all memory associated with the zone.
 
   // All pointers returned from AllocateUnsafe() and New() have this alignment.
   static const intptr_t kAlignment = kDoubleSize;
 
   // Default initial chunk size.
   static const intptr_t kInitialChunkSize = 1 * KB;
 
@@ skipping 80 matching lines @@
 class StackZone : public StackResource {
  public:
   // Create an empty zone and set is at the current zone for the Thread.
   explicit StackZone(Thread* thread);
 
   // Delete all memory associated with the zone.
   ~StackZone();
 
   // Compute the total size of this zone. This includes wasted space that is
   // due to internal fragmentation in the segments.
-  intptr_t SizeInBytes() const { return zone_.SizeInBytes(); }
+  uintptr_t SizeInBytes() const { return zone_.SizeInBytes(); }
 
   // Computes the used space in the zone.
   intptr_t CapacityInBytes() const { return zone_.CapacityInBytes(); }
 
   Zone* GetZone() { return &zone_; }
 
  private:
   Zone zone_;
 
   template <typename T>
   friend class GrowableArray;
   template <typename T>
   friend class ZoneGrowableArray;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(StackZone);
 };
 
 inline uword Zone::AllocUnsafe(intptr_t size) {
   ASSERT(size >= 0);
-
   // Round up the requested size to fit the alignment.
   if (size > (kIntptrMax - kAlignment)) {
     FATAL1("Zone::Alloc: 'size' is too large: size=%" Pd "", size);
   }
   size = Utils::RoundUp(size, kAlignment);
 
   // Check if the requested size is available without expanding.
   uword result;
   intptr_t free_size = (limit_ - position_);
   if (free_size >= size) {
     result = position_;
     position_ += size;
+    Thread* current_thread = Thread::Current();

[siva, 2017/03/22 17:50:55]

This is an overhead on every zone allocation path, have you measured the impact
of doing this on the VM performance?

Is it critical to measure the actual memory usage Vs the current capacity that
has been allocated? We have already allocated that memory from the system how
does it matter whether we used it or not as far as our footprint goes that
memory has been allocated.

[bkonyi, 2017/03/22 18:01:48]

No, I haven't run a performance test yet. Now that you mention it, I agree that
the zone usage statistic isn't very useful compared to the zone capacity value,
unless we're concerned that there's potential issues in how/when the zones
decide to expand / handle reallocations. Thoughts?

+    if (current_thread != NULL) {

[Cutch, 2017/03/22 17:21:49]

Refactor this code so there are two helper functions:

IncrementThreadMemoryUsage(size) {
Thread* current_thread = Thread::Current();
if (current_thread == null) { return; }
current_thread->IncrementMemoryUsage(size);
}

Same for DecrementMemoryUsage

Use these functions instead of sprinkling all these (current_thread != NULL)
checks all over.

[bkonyi, 2017/03/22 18:01:48]

Done.

+      current_thread->IncrementMemoryUsage(size);
+    }
   } else {
     result = AllocateExpand(size);
   }
 
   // Check that the result has the proper alignment and return it.
   ASSERT(Utils::IsAligned(result, kAlignment));
   return result;
 }
 
 
@@ skipping 11 matching lines @@
 inline ElementType* Zone::Alloc(intptr_t len) {
   CheckLength<ElementType>(len);
   return reinterpret_cast<ElementType*>(AllocUnsafe(len * sizeof(ElementType)));
 }
 
 template <class ElementType>
 inline ElementType* Zone::Realloc(ElementType* old_data,
                                   intptr_t old_len,
                                   intptr_t new_len) {
   CheckLength<ElementType>(new_len);
+  Thread* current_thread = Thread::Current();

[Cutch, 2017/03/22 17:21:50]

move this Thread::Current() call so it only happens if you're going to use
current_thread.

[bkonyi, 2017/03/22 18:01:48]

Done.

   const intptr_t kElementSize = sizeof(ElementType);
   uword old_end = reinterpret_cast<uword>(old_data) + (old_len * kElementSize);
   // Resize existing allocation if nothing was allocated in between...
   if (Utils::RoundUp(old_end, kAlignment) == position_) {
     uword new_end =
         reinterpret_cast<uword>(old_data) + (new_len * kElementSize);
     // ...and there is sufficient space.
     if (new_end <= limit_) {
+      ASSERT(new_len >= old_len);
+      uword previous = position_;
       position_ = Utils::RoundUp(new_end, kAlignment);
+      if (current_thread != NULL && previous != position_) {

[Cutch, 2017/03/22 17:21:50]

Wrap each of these != expressions in parens.

[bkonyi, 2017/03/22 18:01:48]

Acknowledged.

+        current_thread->IncrementMemoryUsage((new_len - old_len) *
+                                             kElementSize);
+      }
       return old_data;
     }
   }
   if (new_len <= old_len) {
     return old_data;
   }
   ElementType* new_data = Alloc<ElementType>(new_len);
   if (old_data != 0) {
     memmove(reinterpret_cast<void*>(new_data),
             reinterpret_cast<void*>(old_data), old_len * kElementSize);
   }
   return new_data;
 }
 
 }  // namespace dart
 
 #endif  // RUNTIME_VM_ZONE_H_