Re-land "Concurrently unmap free pages."

src/heap/spaces.cc

Index: src/heap/spaces.cc
diff --git a/src/heap/spaces.cc b/src/heap/spaces.cc
index 1f8194562a1e9ed948d8d9caff464ee43622d4c1..c72c8b0b0933c48e68134aa40d655f0337f40aa6 100644
--- a/src/heap/spaces.cc
+++ b/src/heap/spaces.cc
@@ -163,6 +163,8 @@ bool CodeRange::GetNextAllocationBlock(size_t requested) {
     }
   }
 
+  base::LockGuard<base::Mutex> free_list_lock_guard(&free_list_mutex_);

[Michael Lippautz, 2015/08/24 13:43:12]

Please add a block to make clear which parts are to-be-locked.

[Hannes Payer (out of office), 2015/08/24 14:58:13]

I would have just locked the rest of the function, but we can do better. Here is
your scope. Done.

+
   // Sort and merge the free blocks on the free list and the allocation list.
   free_list_.AddAll(allocation_list_);
   allocation_list_.Clear();
@@ -229,6 +231,7 @@ bool CodeRange::UncommitRawMemory(Address start, size_t length) {
 
 void CodeRange::FreeRawMemory(Address address, size_t length) {
   DCHECK(IsAddressAligned(address, MemoryChunk::kAlignment));
+  base::LockGuard<base::Mutex> free_list_lock_guard(&free_list_mutex_);
   free_list_.Add(FreeBlock(address, length));
   code_range_->Uncommit(address, length);
 }
@@ -237,6 +240,7 @@ void CodeRange::FreeRawMemory(Address address, size_t length) {
 void CodeRange::TearDown() {
   delete code_range_;  // Frees all memory in the virtual memory range.
   code_range_ = NULL;
+  base::LockGuard<base::Mutex> free_list_lock_guard(&free_list_mutex_);
   free_list_.Free();
   allocation_list_.Free();
 }
@@ -264,7 +268,10 @@ bool CodeRange::ReserveBlock(const size_t requested_size, FreeBlock* block) {
 }
 
 
-void CodeRange::ReleaseBlock(const FreeBlock* block) { free_list_.Add(*block); }
+void CodeRange::ReleaseBlock(const FreeBlock* block) {
+  base::LockGuard<base::Mutex> free_list_lock_guard(&free_list_mutex_);
+  free_list_.Add(*block);
+}
 
 
 void CodeRange::ReserveEmergencyBlock() {
@@ -332,26 +339,30 @@ bool MemoryAllocator::CommitMemory(Address base, size_t size,
 }
 
 
-void MemoryAllocator::FreeMemory(base::VirtualMemory* reservation,
-                                 Executability executable) {
-  // TODO(gc) make code_range part of memory allocator?
+void MemoryAllocator::FreeNewSpaceMemory(Address addr,
+                                         base::VirtualMemory* reservation,
+                                         Executability executable) {
+  LOG(isolate_, DeleteEvent("InitialChunk", addr));

[Michael Lippautz, 2015/08/24 13:43:12]

"InitialChunk"?

[Hannes Payer (out of office), 2015/08/24 14:58:13]

This was the original string. We can change it to NewSpace.

+
   DCHECK(reservation->IsReserved());
   size_t size = reservation->size();

[Michael Lippautz, 2015/08/24 13:43:12]

const size_t size = ..

[Hannes Payer (out of office), 2015/08/24 14:58:13]

Done.

   DCHECK(size_ >= size);
   size_ -= size;
-
   isolate_->counters()->memory_allocated()->Decrement(static_cast<int>(size));
+  FreeMemory(reservation, NOT_EXECUTABLE);
+}
 
-  if (executable == EXECUTABLE) {
-    DCHECK(size_executable_ >= size);
-    size_executable_ -= size;
-  }
+
+void MemoryAllocator::FreeMemory(base::VirtualMemory* reservation,
+                                 Executability executable) {
+  // TODO(gc) make code_range part of memory allocator?
   // Code which is part of the code-range does not have its own VirtualMemory.
   DCHECK(isolate_->code_range() == NULL ||
          !isolate_->code_range()->contains(
              static_cast<Address>(reservation->address())));
   DCHECK(executable == NOT_EXECUTABLE || isolate_->code_range() == NULL ||
-         !isolate_->code_range()->valid() || size <= Page::kPageSize);
+         !isolate_->code_range()->valid() ||
+         reservation->size() <= Page::kPageSize);
 
   reservation->Release();
 }
@@ -360,15 +371,6 @@ void MemoryAllocator::FreeMemory(base::VirtualMemory* reservation,
 void MemoryAllocator::FreeMemory(Address base, size_t size,
                                  Executability executable) {
   // TODO(gc) make code_range part of memory allocator?
-  DCHECK(size_ >= size);
-  size_ -= size;
-
-  isolate_->counters()->memory_allocated()->Decrement(static_cast<int>(size));
-
-  if (executable == EXECUTABLE) {
-    DCHECK(size_executable_ >= size);
-    size_executable_ -= size;
-  }
   if (isolate_->code_range() != NULL &&
       isolate_->code_range()->contains(static_cast<Address>(base))) {
     DCHECK(executable == EXECUTABLE);
@@ -742,7 +744,7 @@ LargePage* MemoryAllocator::AllocateLargePage(intptr_t object_size,
 }
 
 
-void MemoryAllocator::Free(MemoryChunk* chunk) {
+void MemoryAllocator::PreFreeMemory(MemoryChunk* chunk) {

[Michael Lippautz, 2015/08/24 13:43:12]

Should we add a flag to {MemoryChunk} indicating that it has been PreFreed()?
(The flag could be checked in PerformFree as DCHECK.)

[Hannes Payer (out of office), 2015/08/24 14:58:13]

Good idea. Done.

   LOG(isolate_, DeleteEvent("MemoryChunk", chunk));
   if (chunk->owner() != NULL) {
     ObjectSpace space =
@@ -753,6 +755,25 @@ void MemoryAllocator::Free(MemoryChunk* chunk) {
   isolate_->heap()->RememberUnmappedPage(reinterpret_cast<Address>(chunk),
                                          chunk->IsEvacuationCandidate());
 
+  size_t size;
+  base::VirtualMemory* reservation = chunk->reserved_memory();
+  if (reservation->IsReserved()) {
+    size = reservation->size();
+  } else {
+    size = chunk->size();
+  }
+  DCHECK(size_ >= size);
+  size_ -= size;
+  isolate_->counters()->memory_allocated()->Decrement(static_cast<int>(size));
+
+  if (chunk->executable() == EXECUTABLE) {
+    DCHECK(size_executable_ >= size);
+    size_executable_ -= size;
+  }
+}
+
+
+void MemoryAllocator::PerformFreeMemory(MemoryChunk* chunk) {

[Michael Lippautz, 2015/08/24 13:43:12]

We could make this a static on {MemoryChunk}. 

{MemoryChunk} is sort of statically created (just cast to a MemoryChunk after
allocating it, so it would probably make sense to destroy it the same way.)

[Hannes Payer (out of office), 2015/08/24 14:58:13]

As discussed offline, this method should not be static since it calls deeper
into the memory allocator. However, it makes sense to perform the deallocation
of the previously alloced data structures in a method on the memory chunk.

   delete chunk->slots_buffer();
   delete chunk->skip_list();
   delete chunk->mutex();
@@ -766,6 +787,12 @@ void MemoryAllocator::Free(MemoryChunk* chunk) {
 }
 
 
+void MemoryAllocator::Free(MemoryChunk* chunk) {
+  PreFreeMemory(chunk);
+  PerformFreeMemory(chunk);
+}
+
+
 bool MemoryAllocator::CommitBlock(Address start, size_t size,
                                   Executability executable) {
   if (!CommitMemory(start, size, executable)) return false;
@@ -1284,11 +1311,9 @@ void NewSpace::TearDown() {
   to_space_.TearDown();
   from_space_.TearDown();
 
-  LOG(heap()->isolate(), DeleteEvent("InitialChunk", chunk_base_));
+  heap()->isolate()->memory_allocator()->FreeNewSpaceMemory(
+      chunk_base_, &reservation_, NOT_EXECUTABLE);
 
-  DCHECK(reservation_.IsReserved());
-  heap()->isolate()->memory_allocator()->FreeMemory(&reservation_,
-                                                    NOT_EXECUTABLE);
   chunk_base_ = NULL;
   chunk_size_ = 0;
 }