heap: make array buffer maps disjoint

src/heap/heap.cc

Index: src/heap/heap.cc
diff --git a/src/heap/heap.cc b/src/heap/heap.cc
index 5d67854462dbaa5d3a6967a5696014509f9e8cf1..abea7f07f7ef5d71856466e1605f3a2143f89396 100644
--- a/src/heap/heap.cc
+++ b/src/heap/heap.cc
@@ -1736,61 +1736,13 @@ void Heap::ProcessNativeContexts(WeakObjectRetainer* retainer) {
 }
 
 
-void Heap::RegisterNewArrayBufferHelper(std::map<void*, size_t>& live_buffers,
-                                        void* data, size_t length) {
-  live_buffers[data] = length;
-}
-
-
-void Heap::UnregisterArrayBufferHelper(
-    std::map<void*, size_t>& live_buffers,
-    std::map<void*, size_t>& not_yet_discovered_buffers, void* data) {
-  DCHECK(live_buffers.count(data) > 0);
-  live_buffers.erase(data);
-  not_yet_discovered_buffers.erase(data);
-}
-
-
-void Heap::RegisterLiveArrayBufferHelper(
-    std::map<void*, size_t>& not_yet_discovered_buffers, void* data) {
-  not_yet_discovered_buffers.erase(data);
-}
-
-
-size_t Heap::FreeDeadArrayBuffersHelper(
-    Isolate* isolate, std::map<void*, size_t>& live_buffers,
-    std::map<void*, size_t>& not_yet_discovered_buffers) {
-  size_t freed_memory = 0;
-  for (auto buffer = not_yet_discovered_buffers.begin();
-       buffer != not_yet_discovered_buffers.end(); ++buffer) {
-    isolate->array_buffer_allocator()->Free(buffer->first, buffer->second);
-    freed_memory += buffer->second;
-    live_buffers.erase(buffer->first);
-  }
-  not_yet_discovered_buffers = live_buffers;
-  return freed_memory;
-}
-
-
-void Heap::TearDownArrayBuffersHelper(
-    Isolate* isolate, std::map<void*, size_t>& live_buffers,
-    std::map<void*, size_t>& not_yet_discovered_buffers) {
-  for (auto buffer = live_buffers.begin(); buffer != live_buffers.end();
-       ++buffer) {
-    isolate->array_buffer_allocator()->Free(buffer->first, buffer->second);
-  }
-  live_buffers.clear();
-  not_yet_discovered_buffers.clear();
-}
-
-
 void Heap::RegisterNewArrayBuffer(bool in_new_space, void* data,
                                   size_t length) {
   if (!data) return;
-  RegisterNewArrayBufferHelper(live_array_buffers_, data, length);
   if (in_new_space) {
-    RegisterNewArrayBufferHelper(live_array_buffers_for_scavenge_, data,
-                                 length);
+    live_array_buffers_for_scavenge_[data] = length;
+  } else {
+    live_array_buffers_[data] = length;
   }
   reinterpret_cast<v8::Isolate*>(isolate_)
       ->AdjustAmountOfExternalAllocatedMemory(length);
@@ -1799,46 +1751,72 @@ void Heap::RegisterNewArrayBuffer(bool in_new_space, void* data,
 
 void Heap::UnregisterArrayBuffer(bool in_new_space, void* data) {

[Michael Lippautz, 2015/08/28 14:25:30]

We can simplify the main part a lot:

std::map<void*, size_t>*live_buffers = 
    in_new_space ? &live_array_buffers_for_scavenge_ : &live_array_buffers_;
std::map<void*, size_t>* not_yet_discovered_buffers = 
    in_new_space ? &not_yet_discovered_buffers_for_scavenge_ :
&not_yet_discovered_buffers_;
DCHECK(live_array_buffers->count(data) > 0);
live_array_buffers->erase(data);
not_yet_discovered_array_buffers->erase(data);

[fedor.indutny, 2015/08/28 20:49:41]

Acknowledged.

   if (!data) return;
-  UnregisterArrayBufferHelper(live_array_buffers_,
-                              not_yet_discovered_array_buffers_, data);
+
+  std::map<void*, size_t>* live_buffers;
+  std::map<void*, size_t>* not_yet_discovered_buffers;
   if (in_new_space) {
-    UnregisterArrayBufferHelper(live_array_buffers_for_scavenge_,
-                                not_yet_discovered_array_buffers_for_scavenge_,
-                                data);
+    live_buffers = &live_array_buffers_for_scavenge_;
+    not_yet_discovered_buffers =
+        &not_yet_discovered_array_buffers_for_scavenge_;
+    not_yet_discovered_array_buffers_.erase(data);

[Michael Lippautz, 2015/08/28 14:25:30]

This  .erase() will not be part of the method as we keep the sets disjoint.

[fedor.indutny, 2015/08/28 20:49:41]

Acknowledged.

+  } else {
+    live_buffers = &live_array_buffers_;
+    not_yet_discovered_buffers = &not_yet_discovered_array_buffers_;
   }
+
+  DCHECK(live_buffers->count(data) > 0);
+  live_buffers->erase(data);
+  not_yet_discovered_buffers->erase(data);
 }
 
 
 void Heap::RegisterLiveArrayBuffer(bool from_scavenge, void* data) {
   // ArrayBuffer might be in the middle of being constructed.
   if (data == undefined_value()) return;
-  RegisterLiveArrayBufferHelper(
-      from_scavenge ? not_yet_discovered_array_buffers_for_scavenge_
-                    : not_yet_discovered_array_buffers_,
-      data);
+  if (from_scavenge) {

[fedor.indutny, 2015/08/28 20:49:41]

This one will need to become unconditional.

+    not_yet_discovered_array_buffers_for_scavenge_.erase(data);
+  } else {
+    not_yet_discovered_array_buffers_.erase(data);
+  }
 }
 
 
 void Heap::FreeDeadArrayBuffers(bool from_scavenge) {

[Michael Lippautz, 2015/08/28 14:25:30]

Since we keep the sets disjoint, we only need to visit XXX_scavenge_ for the new
space part and in the other case we visit all of them:

for buffer in not_yet_discovered_buffers_scavenge:
  isolate.free(buffer)
  freed_memory += …
  live_array_buffers_for_scavenge_.clear(buffer)
if !from_scavenge:
  for buffer in not_yet_discovered_buffers_:
    isolate.free(buffer)
    freed_memory += …
    live_array_buffers_.clear(buffer)


not_yet_discovered_buffers_for_scavenge_ = live_array_buffers_for_scavenge_
if !from_scavenge:
  not_yet_discovered_buffers_ = live_array_buffers_

[fedor.indutny, 2015/08/28 20:49:41]

Acknowledged.

+  std::map<void*, size_t> not_yet_discovered_buffers;
   if (from_scavenge) {
-    for (auto& buffer : not_yet_discovered_array_buffers_for_scavenge_) {
+    not_yet_discovered_buffers = not_yet_discovered_array_buffers_for_scavenge_;
+  } else {
+    not_yet_discovered_buffers = not_yet_discovered_array_buffers_;
+  }
+
+  size_t freed_memory = 0;
+  for (auto& buffer : not_yet_discovered_buffers) {
+    isolate()->array_buffer_allocator()->Free(buffer.first, buffer.second);
+    freed_memory += buffer.second;
+
+    // Scavenge can't happend during evacuation, so we only need to update
+    // live_array_buffers_for_scavenge_.
+    // not_yet_discovered_array_buffers_for_scanvenge_ will be reset before
+    // the next scavenge run in PrepareArrayBufferDiscoveryInNewSpace.
+    live_array_buffers_for_scavenge_.erase(buffer.first);
+
+    if (from_scavenge) {
       not_yet_discovered_array_buffers_.erase(buffer.first);
+    } else {
       live_array_buffers_.erase(buffer.first);
     }
+  }
+
+  if (from_scavenge) {
+    not_yet_discovered_array_buffers_for_scavenge_ =
+        live_array_buffers_for_scavenge_;
   } else {
-    for (auto& buffer : not_yet_discovered_array_buffers_) {
-      // Scavenge can't happend during evacuation, so we only need to update
-      // live_array_buffers_for_scavenge_.
-      // not_yet_discovered_array_buffers_for_scanvenge_ will be reset before
-      // the next scavenge run in PrepareArrayBufferDiscoveryInNewSpace.
-      live_array_buffers_for_scavenge_.erase(buffer.first);
-    }
+    not_yet_discovered_array_buffers_ = live_array_buffers_;
+    not_yet_discovered_array_buffers_.insert(
+        live_array_buffers_for_scavenge_.begin(),
+        live_array_buffers_for_scavenge_.end());
   }
-  size_t freed_memory = FreeDeadArrayBuffersHelper(
-      isolate_,
-      from_scavenge ? live_array_buffers_for_scavenge_ : live_array_buffers_,
-      from_scavenge ? not_yet_discovered_array_buffers_for_scavenge_
-                    : not_yet_discovered_array_buffers_);
+
   if (freed_memory) {
     reinterpret_cast<v8::Isolate*>(isolate_)
         ->AdjustAmountOfExternalAllocatedMemory(
@@ -1848,8 +1826,16 @@ void Heap::FreeDeadArrayBuffers(bool from_scavenge) {
 
 
 void Heap::TearDownArrayBuffers() {

[Michael Lippautz, 2015/08/28 14:25:30]

nit: While practically not relevant, we should still tear down gentle and record
the freed memory (like in FreeDeadArrayBuffers).

e.g.
  freed_memory += buffer.second;
throughout the loop

and 
  AdjustAmountOfExternalAllocatedMemory(freed_memory); 
in the end.

[fedor.indutny, 2015/08/28 20:49:41]

Acknowledged.

-  TearDownArrayBuffersHelper(isolate_, live_array_buffers_,
-                             not_yet_discovered_array_buffers_);
+  for (auto& buffer : live_array_buffers_) {
+    isolate()->array_buffer_allocator()->Free(buffer.first, buffer.second);
+  }
+  for (auto& buffer : live_array_buffers_for_scavenge_) {
+    isolate()->array_buffer_allocator()->Free(buffer.first, buffer.second);
+  }
+  live_array_buffers_.clear();
+  live_array_buffers_for_scavenge_.clear();
+  not_yet_discovered_array_buffers_.clear();
+  not_yet_discovered_array_buffers_for_scavenge_.clear();
 }
 
 
@@ -1867,7 +1853,7 @@ void Heap::PromoteArrayBuffer(Object* obj) {
   // ArrayBuffer might be in the middle of being constructed.
   if (data == undefined_value()) return;
   DCHECK(live_array_buffers_for_scavenge_.count(data) > 0);
-  DCHECK(live_array_buffers_.count(data) > 0);
+  live_array_buffers_[data] = live_array_buffers_for_scavenge_[data];
   live_array_buffers_for_scavenge_.erase(data);
   not_yet_discovered_array_buffers_for_scavenge_.erase(data);
 }