commit 47fec6ce006ebb39babea0e41dbc6dd9e5585f8c
author Liam Appelbe <liama@google.com> Tue Dec 18 18:31:10 2018 +0000
committer commit-bot@chromium.org <commit-bot@chromium.org> Tue Dec 18 18:31:10 2018 +0000
tree e478048e5ecb5c8d78f9821c4dcd133232cb78b7
parent f94aa6e329b946d983755ce7abe69c8c6434aa12

Make capacity check and usage increase atomic.

The fiddly part of this is that the current flow is: check
capacity, allocate page, increase usage (but only if page
allocation was successful). So the idea of this CL is to
increase the usage optimistically, then decrease it if the
page allocation fails.

Closes https://github.com/dart-lang/sdk/issues/27413

Bug: https://github.com/dart-lang/sdk/issues/27413
Change-Id: I97ccf3c5a449dc377b8748a73ff4fcffe6d07f2b
Reviewed-on: https://dart-review.googlesource.com/c/86943
Commit-Queue: Liam Appelbe <liama@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>

runtime/vm/heap/pages.cc

diff --git a/runtime/vm/heap/pages.cc b/runtime/vm/heap/pages.cc
index b6335d5..495ddfc 100644
--- a/runtime/vm/heap/pages.cc
+++ b/runtime/vm/heap/pages.cc
@@ -289,6 +289,13 @@
 }
 
 HeapPage* PageSpace::AllocatePage(HeapPage::PageType type, bool link) {
+  {
+    MutexLocker ml(pages_lock_);
+    if (!CanIncreaseCapacityInWordsLocked(kPageSizeInWords)) {
+      return NULL;
+    }
+    IncreaseCapacityInWordsLocked(kPageSizeInWords);
+  }
   const bool is_exec = (type == HeapPage::kExecutable);
   const intptr_t kVmNameSize = 128;
   char vm_name[kVmNameSize];
@@ -297,6 +304,7 @@
   HeapPage* page = HeapPage::Allocate(kPageSizeInWords, type, vm_name);
   if (page == NULL) {
     RELEASE_ASSERT(!FLAG_abort_on_oom);
+    IncreaseCapacityInWords(-kPageSizeInWords);
     return NULL;
   }
 
@@ -329,25 +337,31 @@
     }
   }
 
-  IncreaseCapacityInWordsLocked(kPageSizeInWords);
   page->set_object_end(page->memory_->end());
   return page;
 }
 
 HeapPage* PageSpace::AllocateLargePage(intptr_t size, HeapPage::PageType type) {
-  const bool is_exec = (type == HeapPage::kExecutable);
   const intptr_t page_size_in_words = LargePageSizeInWordsFor(size);
+  {
+    MutexLocker ml(pages_lock_);
+    if (!CanIncreaseCapacityInWordsLocked(page_size_in_words)) {
+      return NULL;
+    }
+    IncreaseCapacityInWordsLocked(page_size_in_words);
+  }
+  const bool is_exec = (type == HeapPage::kExecutable);
   const intptr_t kVmNameSize = 128;
   char vm_name[kVmNameSize];
   Heap::RegionName(heap_, is_exec ? Heap::kCode : Heap::kOld, vm_name,
                    kVmNameSize);
   HeapPage* page = HeapPage::Allocate(page_size_in_words, type, vm_name);
   if (page == NULL) {
+    IncreaseCapacityInWords(-page_size_in_words);
     return NULL;
   }
   page->set_next(large_pages_);
   large_pages_ = page;
-  IncreaseCapacityInWords(page_size_in_words);
   // Only one object in this page (at least until String::MakeExternal or
   // Array::MakeFixedLength is called).
   page->set_object_end(page->object_start() + size);
@@ -441,9 +455,8 @@
   after_allocation.used_in_words += size >> kWordSizeLog2;
   // Can we grow by one page?
   after_allocation.capacity_in_words += kPageSizeInWords;
-  if ((growth_policy == kForceGrowth ||
-       !page_space_controller_.NeedsGarbageCollection(after_allocation)) &&
-      CanIncreaseCapacityInWords(kPageSizeInWords)) {
+  if (growth_policy == kForceGrowth ||
+      !page_space_controller_.NeedsGarbageCollection(after_allocation)) {
     HeapPage* page = AllocatePage(type);
     if (page == NULL) {
       return 0;
@@ -498,9 +511,8 @@
     SpaceUsage after_allocation = GetCurrentUsage();
     after_allocation.used_in_words += size >> kWordSizeLog2;
     after_allocation.capacity_in_words += page_size_in_words;
-    if ((growth_policy == kForceGrowth ||
-         !page_space_controller_.NeedsGarbageCollection(after_allocation)) &&
-        CanIncreaseCapacityInWords(page_size_in_words)) {
+    if (growth_policy == kForceGrowth ||
+        !page_space_controller_.NeedsGarbageCollection(after_allocation)) {
       HeapPage* page = AllocateLargePage(size, type);
       if (page != NULL) {
         result = page->object_start();

runtime/vm/heap/pages.h

diff --git a/runtime/vm/heap/pages.h b/runtime/vm/heap/pages.h
index 4f93e24..f030f40 100644
--- a/runtime/vm/heap/pages.h
+++ b/runtime/vm/heap/pages.h
@@ -453,16 +453,13 @@
 
   static intptr_t LargePageSizeInWordsFor(intptr_t size);
 
-  bool CanIncreaseCapacityInWords(intptr_t increase_in_words) {
+  bool CanIncreaseCapacityInWordsLocked(intptr_t increase_in_words) {
     if (max_capacity_in_words_ == 0) {
       // Unlimited.
       return true;
     }
-    // TODO(issue 27413): Make the check against capacity and the bump
-    // of capacity atomic so that CapacityInWords does not exceed
-    // max_capacity_in_words_.
     intptr_t free_capacity_in_words =
-        (max_capacity_in_words_ - CapacityInWords());
+        (max_capacity_in_words_ - usage_.capacity_in_words);
     return ((free_capacity_in_words > 0) &&
             (increase_in_words <= free_capacity_in_words));
   }