Use MemoryChunk-based allocation for deoptimization entry code

src/deoptimizer.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 26 matching lines @@
 #include "prettyprinter.h"
 
 
 namespace v8 {
 namespace internal {
 
 DeoptimizerData::DeoptimizerData() {
   eager_deoptimization_entry_code_entries_ = -1;
   lazy_deoptimization_entry_code_entries_ = -1;
   size_t deopt_table_size = Deoptimizer::GetMaxDeoptTableSize();
-  eager_deoptimization_entry_code_ = new VirtualMemory(deopt_table_size);
-  lazy_deoptimization_entry_code_ = new VirtualMemory(deopt_table_size);
+  eager_deoptimization_entry_code_ = new VirtualMemory();
+  lazy_deoptimization_entry_code_ = new VirtualMemory();
+  MemoryAllocator* allocator = Isolate::Current()->memory_allocator();
+  eager_deoptimization_entry_start_ = allocator->ReserveChunk(deopt_table_size,

[danno, 2012/12/28 11:58:37]

Please don't store into a member variable and change after the fact. Set a local
variable and store the member variable only once with the tweaked address, i.e.:
Address chunk = allocator->ReserveChunk(...);
eager_deoptimization_entry_start_ = chunk +
MemoryAllocator::CodePageAreaStartOffset();

[haitao.feng, 2012/12/28 15:04:54]

In this CL, I am using eager_deoptimization_entry_code_ and
lazy_deoptimization_entry_code_ to store the reserved address and reserved size.
The allocator->ReserveChunk is equal to the reserved address if reservation is
successful.

+                                          EXECUTABLE,
+                                          eager_deoptimization_entry_code_);
+  if (eager_deoptimization_entry_start_ == NULL) {
+    V8::FatalProcessOutOfMemory("Not enough memory for deoptimization table");
+  }
+  eager_deoptimization_entry_start_ +=
+      MemoryAllocator::CodePageAreaStartOffset();
+  lazy_deoptimization_entry_start_ = allocator->ReserveChunk(deopt_table_size,
+                                         EXECUTABLE,
+                                         lazy_deoptimization_entry_code_);
+  if (lazy_deoptimization_entry_start_ == NULL) {
+    V8::FatalProcessOutOfMemory("Not enough memory for deoptimization table");
+  }
+  lazy_deoptimization_entry_start_ +=
+      MemoryAllocator::CodePageAreaStartOffset();
+  eager_deoptimization_chunk_ = NULL;
+  lazy_deoptimization_chunk_ = NULL;
   current_ = NULL;
   deoptimizing_code_list_ = NULL;
 #ifdef ENABLE_DEBUGGER_SUPPORT
   deoptimized_frame_info_ = NULL;
 #endif
 }
 
 
 DeoptimizerData::~DeoptimizerData() {
   delete eager_deoptimization_entry_code_;

[danno, 2012/12/28 11:58:37]

These must be destructed _after_ the memory chunks, don't they, since they are
initialized before?

[haitao.feng, 2012/12/28 15:04:54]

When a trunk is returned, the trunk will take control of
eager_deoptimization_entry_code_ and lazy_deoptimization_entry_code_. The
address in eager_deoptimization_entry_code_ and lazy_deoptimization_entry_code_
will be NULL.

[danno, 2012/12/28 15:38:10]

I don't know what you mean by "trunk". The more I think about this, you
shouldn't be using a VirtualMemory to store this at all. If you need to need to
store the base and size, then store those explicitly rather than wrapping them
in a VirtualMemory object. You don't actually manage the deopt table's
VirtualMemory, so you shouldn't act like you do.

On 2012/12/28 15:04:54, haitao.feng wrote:

   eager_deoptimization_entry_code_ = NULL;
   delete lazy_deoptimization_entry_code_;
   lazy_deoptimization_entry_code_ = NULL;
+  if (eager_deoptimization_chunk_ != NULL) {
+    Isolate::Current()->memory_allocator()->Free(
+        eager_deoptimization_chunk_);
+    eager_deoptimization_chunk_ = NULL;
+  }
+  if (lazy_deoptimization_chunk_ != NULL) {
+    Isolate::Current()->memory_allocator()->Free(
+        lazy_deoptimization_chunk_);
+    lazy_deoptimization_chunk_ = NULL;
+  }
 
   DeoptimizingCodeListNode* current = deoptimizing_code_list_;
   while (current != NULL) {
     DeoptimizingCodeListNode* prev = current;
     current = current->next();
     delete prev;
   }
   deoptimizing_code_list_ = NULL;
 }
 
@@ skipping 538 matching lines @@
   output_ = NULL;
   ASSERT(!HEAP->allow_allocation(true));
 }
 
 
 Address Deoptimizer::GetDeoptimizationEntry(int id,
                                             BailoutType type,
                                             GetEntryMode mode) {
   ASSERT(id >= 0);
   if (id >= kMaxNumberOfEntries) return NULL;
-  VirtualMemory* base = NULL;
+  Address base = NULL;
   if (mode == ENSURE_ENTRY_CODE) {
     EnsureCodeForDeoptimizationEntry(type, id);
   } else {
     ASSERT(mode == CALCULATE_ENTRY_ADDRESS);
   }
   DeoptimizerData* data = Isolate::Current()->deoptimizer_data();
   if (type == EAGER) {
-    base = data->eager_deoptimization_entry_code_;
+    base = data->eager_deoptimization_entry_start_;
   } else {
-    base = data->lazy_deoptimization_entry_code_;
+    base = data->lazy_deoptimization_entry_start_;
   }
   return
-      static_cast<Address>(base->address()) + (id * table_entry_size_);
+      base + (id * table_entry_size_);
 }
 
 
 int Deoptimizer::GetDeoptimizationId(Address addr, BailoutType type) {
-  VirtualMemory* base = NULL;
+  Address base = NULL;
   DeoptimizerData* data = Isolate::Current()->deoptimizer_data();
   if (type == EAGER) {
-    base = data->eager_deoptimization_entry_code_;
+    base = data->eager_deoptimization_entry_start_;
   } else {
-    base = data->lazy_deoptimization_entry_code_;
+    base = data->lazy_deoptimization_entry_start_;
   }
-  Address base_casted = reinterpret_cast<Address>(base->address());
   if (base == NULL ||
-      addr < base->address() ||
-      addr >= base_casted + (kMaxNumberOfEntries * table_entry_size_)) {
+      addr < base ||
+      addr >= base + (kMaxNumberOfEntries * table_entry_size_)) {
     return kNotDeoptimizationEntry;
   }
   ASSERT_EQ(0,
-            static_cast<int>(addr - base_casted) % table_entry_size_);
-  return static_cast<int>(addr - base_casted) / table_entry_size_;
+            static_cast<int>(addr - base) % table_entry_size_);
+  return static_cast<int>(addr - base) / table_entry_size_;
 }
 
 
 int Deoptimizer::GetOutputInfo(DeoptimizationOutputData* data,
                                BailoutId id,
                                SharedFunctionInfo* shared) {
   // TODO(kasperl): For now, we do a simple linear search for the PC
   // offset associated with the given node id. This should probably be
   // changed to a binary search.
   int length = data->DeoptPoints();
@@ skipping 903 matching lines @@
   masm.set_emit_debug_code(false);
   GenerateDeoptimizationEntries(&masm, entry_count, type);
   CodeDesc desc;
   masm.GetCode(&desc);
 
   VirtualMemory* memory = type == EAGER
       ? data->eager_deoptimization_entry_code_
       : data->lazy_deoptimization_entry_code_;
   size_t table_size = Deoptimizer::GetMaxDeoptTableSize();
   ASSERT(static_cast<int>(table_size) >= desc.instr_size);
-  memory->Commit(memory->address(), table_size, true);
-  memcpy(memory->address(), desc.buffer, desc.instr_size);
-  CPU::FlushICache(memory->address(), desc.instr_size);
+
+  MemoryAllocator* allocator = Isolate::Current()->memory_allocator();
+  MemoryChunk** chunk = type == EAGER

[danno, 2012/12/28 11:58:37]

Can you please not use the conditional operator here to be tricky with the
assignment address? Just store the results of CommitChunk into chunk which is
just a MemoryChunk* and use an if statement afterwards to assign to either
data->eager_deoptimization_chunk_
 or data->lazy_deoptimization_chunk_.

+      ? &data->eager_deoptimization_chunk_
+      : &data->lazy_deoptimization_chunk_;
+  if (*chunk == NULL) {

[danno, 2012/12/28 11:58:37]

I don't think this is right. If the deopt table grows (which it does), this
routine gets called multiple times. In that case, you need to make sure that the
entire grown table is committed. Here you only commit once with the initial
table size. Or am I missing something?

[haitao.feng, 2012/12/28 15:04:54]

If I read the codes right, table_size is Deoptimizer::GetMaxDeoptTableSize. So
we need commit memory only once, but we do need to do memcpy when the deopt
table grows.

+    *chunk = allocator->CommitChunk(table_size, EXECUTABLE, memory, NULL);
+    if (*chunk == NULL) {
+      V8::FatalProcessOutOfMemory("Not enough memory for deoptimization table");
+    }
+  }
+  memcpy((*chunk)->area_start(), desc.buffer, desc.instr_size);
+  CPU::FlushICache((*chunk)->area_start(), desc.instr_size);
 
   if (type == EAGER) {
     data->eager_deoptimization_entry_code_entries_ = entry_count;
   } else {
     data->lazy_deoptimization_entry_code_entries_ = entry_count;
   }
 }
 
 
 void Deoptimizer::ReplaceCodeForRelatedFunctions(JSFunction* function,
@@ skipping 521 matching lines @@
 
 void DeoptimizedFrameInfo::Iterate(ObjectVisitor* v) {
   v->VisitPointer(BitCast<Object**>(&function_));
   v->VisitPointers(parameters_, parameters_ + parameters_count_);
   v->VisitPointers(expression_stack_, expression_stack_ + expression_count_);
 }
 
 #endif  // ENABLE_DEBUGGER_SUPPORT
 
 } }  // namespace v8::internal