heap: make array buffer maps disjoint

src/heap/heap.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/heap/heap.h"
 
 #include "src/accessors.h"
 #include "src/api.h"
 #include "src/base/bits.h"
 #include "src/base/once.h"
@@ skipping 1718 matching lines @@
 }
 
 
 void Heap::ProcessNativeContexts(WeakObjectRetainer* retainer) {
   Object* head = VisitWeakList<Context>(this, native_contexts_list(), retainer);
   // Update the head of the list of contexts.
   set_native_contexts_list(head);
 }
 
 
 void Heap::RegisterNewArrayBufferHelper(std::map<void*, size_t>& live_buffers,

[Michael Lippautz, 2015/08/28 08:05:02]

Please get rid of this function and inline it into RegisterNewArrayBuffer.

[fedor.indutny, 2015/08/28 11:03:34]

Acknowledged.

                                         void* data, size_t length) {
   live_buffers[data] = length;
 }
 
 
 void Heap::UnregisterArrayBufferHelper(

[Michael Lippautz, 2015/08/28 08:05:02]

Ditto, please inline the call manually.

[fedor.indutny, 2015/08/28 11:03:34]

Acknowledged.

     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(

[Michael Lippautz, 2015/08/28 08:05:02]

Please get rid of this function and inline it into RegisterLiveArrayBuffer.

[fedor.indutny, 2015/08/28 11:03:34]

Acknowledged.

     std::map<void*, size_t>& not_yet_discovered_buffers, void* data) {
   not_yet_discovered_buffers.erase(data);
 }
 
 
 size_t Heap::FreeDeadArrayBuffersHelper(

[Michael Lippautz, 2015/08/28 08:05:02]

We should get rid of this one. See below.

[fedor.indutny, 2015/08/28 11:03:34]

Acknowledged.

-    Isolate* isolate, std::map<void*, size_t>& live_buffers,
+    Isolate* isolate, bool from_scavenge,
     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);
+    if (!from_scavenge) {
+      live_array_buffers_.erase(buffer->first);
+    }
+    live_array_buffers_for_scavenge_.erase(buffer->first);
   }
-  not_yet_discovered_buffers = live_buffers;
+  if (from_scavenge) {
+    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
+  } else {
+    not_yet_discovered_buffers = live_array_buffers_;
+    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_.begin(),
+                                      live_array_buffers_for_scavenge_.end());
+  }
   return freed_memory;
 }
 
 
 void Heap::TearDownArrayBuffersHelper(

[Michael Lippautz, 2015/08/28 08:05:02]

Also inline please.

[fedor.indutny, 2015/08/28 10:02:37]

This one is going to result in duplicated code, isn't it?

[fedor.indutny, 2015/08/28 11:03:34]

Acknowledged.

     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);
+  } else {
+    RegisterNewArrayBufferHelper(live_array_buffers_, data, length);
   }
   reinterpret_cast<v8::Isolate*>(isolate_)
       ->AdjustAmountOfExternalAllocatedMemory(length);
 }
 
 
 void Heap::UnregisterArrayBuffer(bool in_new_space, void* data) {
   if (!data) return;
-  UnregisterArrayBufferHelper(live_array_buffers_,
-                              not_yet_discovered_array_buffers_, data);
   if (in_new_space) {
     UnregisterArrayBufferHelper(live_array_buffers_for_scavenge_,
                                 not_yet_discovered_array_buffers_for_scavenge_,
                                 data);
+    not_yet_discovered_array_buffers_.erase(data);
+  } else {
+    UnregisterArrayBufferHelper(live_array_buffers_,
+                                not_yet_discovered_array_buffers_, 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);
 }
 
 
 void Heap::FreeDeadArrayBuffers(bool from_scavenge) {

[Michael Lippautz, 2015/08/28 08:05:02]

The logic here is way too complicated as the semantic is to maintain two
distinct sets for array buffers.

I inlined, reshuffled and consolidated the calls. I think we should get rid of
the helper method as the actual logic seems to be pretty simple. 

Please check the simplification. I am using .erase() with buffers (there exists
an overload that takes a map). The loops (over maps) get unfolded into a single
loop in the last step.


======== Step 0 (baseline; inlined FreeDeadArrayBuffersHelper call from patch)
======== 

FreeDeadArrayBuffers(from_scavenge):
  if from_scavenge:
   
not_yet_discovered_array_buffers_.erase(not_yet_discovered_array_buffers_for_scavenge_)
  else:
    live_array_buffers_for_scavenge_.erase(not_yet_discovered_array_buffers_)

  # inlined FreeDeadArrayBuffersHelper
  # parameters are unfolded here
  if from_scavenge:
    not_yet_discovered_buffers = not_yet_discovered_array_buffers_for_scavenge_
  else:
    not_yet_discovered_buffers = not_yet_discovered_array_buffers_

  for buffer in not_yet_discovered_buffers:
    isolate.free_array_buffer(buffer)
    if (!from_scavenge):
      live_array_buffers_.erase(buffer)
    live_array_buffers_for_scavenge_.erase(buffer)

  if (from_scavenge):
    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
  else:
    not_yet_discovered_buffers = live_array_buffers_;
    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge.begin(),
...end())


======== Step 1 (shuffle inlined stuff around) ======== 

FreeDeadArrayBuffers(from_scavenge):
  if from_scavenge:
   
not_yet_discovered_array_buffers_.erase(not_yet_discovered_array_buffers_for_scavenge_)
    not_yet_discovered_buffers = not_yet_discovered_array_buffers_for_scavenge_
    live_array_buffers_for_scavenge_.erase(not_yet_discovered_buffers)
  else:
    live_array_buffers_for_scavenge_.erase(not_yet_discovered_array_buffers_)
    not_yet_discovered_buffers = not_yet_discovered_array_buffers_
    live_array_buffers_for_scavenge_.erase(not_yet_discovered_buffers)
    live_array_buffers_.erase(not_yet_discovered_buffers)

  # rest of inlined FreeDeadArrayBuffersHelper

  for buffer in not_yet_discovered_buffers:
    isolate.free_array_buffer(buffer)

  if (from_scavenge):
    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
  else:
    not_yet_discovered_buffers = live_array_buffers_;
    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)


======== Step 2 (re-orde / remove duplicates) ======== 

FreeDeadArrayBuffers(from_scavenge):
  if from_scavenge:
    tmp_not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
    not_yet_discovered_array_buffers_.erase((tmp_not_yet_discovered_buffers)
    live_array_buffers_for_scavenge_.erase(tmp_not_yet_discovered_buffers)
  else:
    tmp_not_yet_discovered_buffers = not_yet_discovered_array_buffers_
    live_array_buffers_for_scavenge_.erase(tmp_not_yet_discovered_buffers)
    live_array_buffers_.erase(tmp_not_yet_discovered_buffers)

  # rest of inlined FreeDeadArrayBuffersHelper

  for buffer in tmp_not_yet_discovered_buffers:
    isolate.free_array_buffer(buffer)

  if (from_scavenge):
    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
  else:
    not_yet_discovered_buffers = live_array_buffers_;
    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)


======== Step 3 (consolidate into a single loop) ======== 

FreeDeadArrayBuffers(from_scavenge):
  if from_scavenge:
    tmp_not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
  else:
    tmp_not_yet_discovered_buffers = not_yet_discovered_array_buffers_

  for buffer in tmp_not_yet_discovered_buffers:
    isolate.free_array_buffer(buffer)
    live_array_buffers_for_scavenge_.erase(buffer)
    if (from_scavenge):
      not_yet_discovered_array_buffers_.erase(buffer)
    else:
      live_array_buffers_.erase(tmp_not_yet_discovered_buffers)

  if (from_scavenge):
    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
  else:
    not_yet_discovered_buffers = live_array_buffers_;
    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)

[fedor.indutny, 2015/08/28 11:03:34]

Acknowledged.

   if (from_scavenge) {
     for (auto& buffer : not_yet_discovered_array_buffers_for_scavenge_) {
       not_yet_discovered_array_buffers_.erase(buffer.first);
-      live_array_buffers_.erase(buffer.first);
     }
   } 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);
     }
   }
   size_t freed_memory = FreeDeadArrayBuffersHelper(
       isolate_,
-      from_scavenge ? live_array_buffers_for_scavenge_ : live_array_buffers_,
+      from_scavenge,
       from_scavenge ? not_yet_discovered_array_buffers_for_scavenge_
                     : not_yet_discovered_array_buffers_);
   if (freed_memory) {
     reinterpret_cast<v8::Isolate*>(isolate_)
         ->AdjustAmountOfExternalAllocatedMemory(
             -static_cast<int64_t>(freed_memory));
   }
 }
 
 
 void Heap::TearDownArrayBuffers() {
   TearDownArrayBuffersHelper(isolate_, live_array_buffers_,
                              not_yet_discovered_array_buffers_);
+  TearDownArrayBuffersHelper(isolate_, live_array_buffers_for_scavenge_,
+                             not_yet_discovered_array_buffers_for_scavenge_);
 }
 
 
 void Heap::PrepareArrayBufferDiscoveryInNewSpace() {
   not_yet_discovered_array_buffers_for_scavenge_ =
       live_array_buffers_for_scavenge_;
 }
 
 
 void Heap::PromoteArrayBuffer(Object* obj) {
   JSArrayBuffer* buffer = JSArrayBuffer::cast(obj);
   if (buffer->is_external()) return;
   void* data = buffer->backing_store();
   if (!data) return;
   // 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);
 }
 
 
 void Heap::ProcessAllocationSites(WeakObjectRetainer* retainer) {
   Object* allocation_site_obj =
       VisitWeakList<AllocationSite>(this, allocation_sites_list(), retainer);
   set_allocation_sites_list(allocation_site_obj);
 }
@@ skipping 4894 matching lines @@
     *object_type = "CODE_TYPE";                                                \
     *object_sub_type = "CODE_AGE/" #name;                                      \
     return true;
     CODE_AGE_LIST_COMPLETE(COMPARE_AND_RETURN_NAME)
 #undef COMPARE_AND_RETURN_NAME
   }
   return false;
 }
 }  // namespace internal
 }  // namespace v8