Refactor some deoptimization code.

runtime/vm/code_generator.cc

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/code_generator.h"
 
 #include "vm/assembler.h"
 #include "vm/ast.h"
 #include "vm/bigint_operations.h"
 #include "vm/code_patcher.h"
@@ skipping 1476 matching lines @@
       const intptr_t owner_cid = Class::Handle(Function::Handle(
           optimized_code.function()).Owner()).id();
       if (ContainsCid(classes, owner_cid)) {
         DeoptimizeAt(optimized_code, frame->pc());
       }
     }
   }
 }
 
 
-// Copy saved registers into the isolate buffer.
-static void CopySavedRegisters(uword saved_registers_address) {
+static void CopySavedRegisters(uword saved_registers_address,
+                               fpu_register_t** fpu_registers,
+                               intptr_t** cpu_registers) {
   ASSERT(sizeof(fpu_register_t) == kFpuRegisterSize);
   fpu_register_t* fpu_registers_copy =
       new fpu_register_t[kNumberOfFpuRegisters];
   ASSERT(fpu_registers_copy != NULL);
   for (intptr_t i = 0; i < kNumberOfFpuRegisters; i++) {
     fpu_registers_copy[i] =
         *reinterpret_cast<fpu_register_t*>(saved_registers_address);
     saved_registers_address += kFpuRegisterSize;
   }
-  Isolate::Current()->set_deopt_fpu_registers_copy(fpu_registers_copy);
+  *fpu_registers = fpu_registers_copy;
 
   ASSERT(sizeof(intptr_t) == kWordSize);
   intptr_t* cpu_registers_copy = new intptr_t[kNumberOfCpuRegisters];
   ASSERT(cpu_registers_copy != NULL);
   for (intptr_t i = 0; i < kNumberOfCpuRegisters; i++) {
     cpu_registers_copy[i] =
         *reinterpret_cast<intptr_t*>(saved_registers_address);
     saved_registers_address += kWordSize;
   }
-  Isolate::Current()->set_deopt_cpu_registers_copy(cpu_registers_copy);
+  *cpu_registers = cpu_registers_copy;
 }
 
 
-// Copy optimized frame into the isolate buffer.
+// Copy optimized frame into the DeoptContext..
 // The first incoming argument is stored at the last entry in the
 // copied frame buffer.
-static void CopyFrame(const Code& optimized_code, const StackFrame& frame) {
+static void CopyFrame(DeoptContext* deopt_context,
+                      const Code& optimized_code,
+                      const StackFrame& frame) {
   const Function& function = Function::Handle(optimized_code.function());
   // Do not copy incoming arguments if there are optional arguments (they
   // are copied into local space at method entry).
   const intptr_t num_args =
       function.HasOptionalParameters() ? 0 : function.num_fixed_parameters();
   // The fixed size section of the (fake) Dart frame called via a stub by the
   // optimized function contains FP, PP (ARM and MIPS only), PC-marker and
   // return-address. This section is copied as well, so that its contained
   // values can be updated before returning to the deoptimized function.
   const intptr_t frame_copy_size =
       + kDartFrameFixedSize  // For saved values below sp.
       + ((frame.fp() - frame.sp()) / kWordSize)  // For frame size incl. sp.
       + 1  // For fp.
       + kParamEndSlotFromFp  // For saved values above fp.
       + num_args;  // For arguments.
   intptr_t* frame_copy = new intptr_t[frame_copy_size];
   ASSERT(frame_copy != NULL);
   intptr_t* start = reinterpret_cast<intptr_t*>(
       frame.sp() - (kDartFrameFixedSize * kWordSize));
   for (intptr_t i = 0; i < frame_copy_size; i++) {
     frame_copy[i] = *(start + i);
   }
-  Isolate::Current()->SetDeoptFrameCopy(frame_copy, frame_copy_size);
+  deopt_context->SetFromFrame(frame_copy, frame_copy_size);
 }
 
 
 // Copies saved registers and caller's frame into temporary buffers.
 // Returns the stack size of unoptimized frame.
 DEFINE_LEAF_RUNTIME_ENTRY(intptr_t, DeoptimizeCopyFrame,
                           1, uword saved_registers_address) {
   Isolate* isolate = Isolate::Current();
   StackZone zone(isolate);
   HANDLESCOPE(isolate);
 
   // All registers have been saved below last-fp as if they were locals.
   const uword last_fp = saved_registers_address
                         + (kNumberOfCpuRegisters * kWordSize)
                         + (kNumberOfFpuRegisters * kFpuRegisterSize)
                         - ((kFirstLocalSlotFromFp + 1) * kWordSize);
-  CopySavedRegisters(saved_registers_address);
+  fpu_register_t* fpu_registers;
+  intptr_t* cpu_registers;
+  CopySavedRegisters(saved_registers_address, &fpu_registers, &cpu_registers);
 
   // Get optimized code and frame that need to be deoptimized.
   DartFrameIterator iterator(last_fp);
   StackFrame* caller_frame = iterator.NextFrame();
   ASSERT(caller_frame != NULL);
   const Code& optimized_code = Code::Handle(caller_frame->LookupDartCode());
   ASSERT(optimized_code.is_optimized());
 
   intptr_t deopt_reason = kDeoptUnknown;
   const DeoptInfo& deopt_info = DeoptInfo::Handle(
       optimized_code.GetDeoptInfoAtPc(caller_frame->pc(), &deopt_reason));
   ASSERT(!deopt_info.IsNull());
 
-  CopyFrame(optimized_code, *caller_frame);
+  // Create the DeoptContext for this deoptimization.  Store in isolate.
+  const Function& function = Function::Handle(optimized_code.function());
+  const intptr_t num_args =
+      function.HasOptionalParameters() ? 0 : function.num_fixed_parameters();
+  DeoptContext* deopt_context = new DeoptContext(
+      Array::Handle(optimized_code.object_table()),
+      num_args,
+      static_cast<DeoptReasonId>(deopt_reason));
+  isolate->set_deopt_context(deopt_context);
+
+  // TODO(turnidge): Why can't I move CopySavedRegisters here?
+  deopt_context->SetSavedRegisters(fpu_registers, cpu_registers);

[Florian Schneider, 2013/09/30 11:32:29]

Wouldn't it work to pass fpu_registers and cpu_registers as arguments to the
DeoptContext constructor instead of initalizing them here?

[turnidge, 2013/09/30 17:40:22]

See next reply for full discussion.

+  CopyFrame(deopt_context, optimized_code, *caller_frame);

[Florian Schneider, 2013/09/30 11:32:29]

It is quite confusing that the different members of DeoptContext computed here
are initialized in different places: in the constructor, in SetSavedRegisters,
SetFromFrame. Maybe you can do all this from the constructor?

[turnidge, 2013/09/30 17:40:22]

SetToFrame is done in the following runtime call, so I can't combine all
initialization in one place.

I could move regs + from stack initialization to the constructor, but that is
asymmetric with SetToFrame, which is an accessor.

Of note: I am also modifying this further in another change I am working on, so
the constructor arguments could change further to include DeoptInfo, etc.

How about:

1. Constructor is initialized with general info about the deopt: function,
reason, pc, or whatever.
2. Source arguments (regs + frame) are set by a single call: SetSources()?
3. Destination args set by a call to something like SetDest or SetTarget.

While I'm at it: I dislike the "To" and "From" names.  Functions like
GetFromPc() sound like they are getting something from a pc offset rather than
getting the "from" pc.  Since "from" is a preposition it leads readers down the
wrong path.  Any objections to changing these?

[turnidge, 2013/09/30 19:35:10]

Okay, I have cleanup up the interface a bit here in the latest revision.  
Hopefully this is less confusing.

On 2013/09/30 17:40:22, turnidge wrote:

+
   if (FLAG_trace_deoptimization || FLAG_trace_deoptimization_verbose) {
-    Function& function = Function::Handle(optimized_code.function());
     OS::PrintErr(
         "Deoptimizing (reason %" Pd " '%s') at pc %#" Px " '%s' (count %d)\n",
         deopt_reason,
         DeoptReasonToText(deopt_reason),
         caller_frame->pc(),
         function.ToFullyQualifiedCString(),
         function.deoptimization_counter());
   }
 
   // Compute the stack size of the unoptimized frame.  For functions with
   // optional arguments the deoptimization info does not describe the
   // incoming arguments.
-  const Function& function = Function::Handle(optimized_code.function());
-  const intptr_t num_args =
-      function.HasOptionalParameters() ? 0 : function.num_fixed_parameters();
   const intptr_t unoptimized_stack_size =
       + deopt_info.FrameSize()
       - kDartFrameFixedSize
       - num_args
       - kParamEndSlotFromFp
       - 1;  // For fp.
   return unoptimized_stack_size * kWordSize;  // Stack size (FP - SP) in bytes.
 }
 END_LEAF_RUNTIME_ENTRY
 
 
-static void DeoptimizeWithDeoptInfo(const Code& code,
+static void DeoptimizeWithDeoptInfo(DeoptContext* deopt_context,
+                                    const Code& code,
                                     const DeoptInfo& deopt_info,
-                                    const StackFrame& caller_frame,
-                                    intptr_t deopt_reason) {
+                                    const StackFrame& caller_frame) {
   const intptr_t len = deopt_info.TranslationLength();
   GrowableArray<DeoptInstr*> deopt_instructions(len);
   const Array& deopt_table = Array::Handle(code.deopt_info_array());
   ASSERT(!deopt_table.IsNull());
   deopt_info.ToInstructions(deopt_table, &deopt_instructions);
 
   intptr_t* start = reinterpret_cast<intptr_t*>(
       caller_frame.sp() - (kDartFrameFixedSize * kWordSize));
   const Function& function = Function::Handle(code.function());
   const intptr_t num_args =
       function.HasOptionalParameters() ? 0 : function.num_fixed_parameters();
   const intptr_t to_frame_size =
       + kDartFrameFixedSize  // For saved values below sp.
       + (caller_frame.fp() - caller_frame.sp()) / kWordSize
       + 1  // For fp.
       + kParamEndSlotFromFp
       + num_args;
-  DeoptimizationContext deopt_context(start,
-                                      to_frame_size,
-                                      Array::Handle(code.object_table()),
-                                      num_args,
-                                      static_cast<DeoptReasonId>(deopt_reason));
+
+  deopt_context->SetToFrame(start, to_frame_size);
+
   const intptr_t frame_size = deopt_info.FrameSize();
 
   // All kMaterializeObject instructions are emitted before the instructions
   // that describe stack frames. Skip them and defer materialization of
   // objects until the frame is fully reconstructed and it is safe to perform
   // GC.
   // Arguments (class of the instance to allocate and field-value pairs) are
   // described as part of the expression stack for the bottom-most deoptimized
   // frame. They will be used during materialization and removed from the stack
   // right before control switches to the unoptimized code.
   const intptr_t num_materializations = len - frame_size;
-  Isolate::Current()->PrepareForDeferredMaterialization(num_materializations);
+  deopt_context->PrepareForDeferredMaterialization(num_materializations);
   for (intptr_t from_index = 0, to_index = kDartFrameFixedSize;
        from_index < num_materializations;
        from_index++) {
     const intptr_t field_count =
         DeoptInstr::GetFieldCount(deopt_instructions[from_index]);
-    intptr_t* args = deopt_context.GetToFrameAddressAt(to_index);
+    intptr_t* args = deopt_context->GetToFrameAddressAt(to_index);
     DeferredObject* obj = new DeferredObject(field_count, args);
-    Isolate::Current()->SetDeferredObjectAt(from_index, obj);
+    deopt_context->SetDeferredObjectAt(from_index, obj);
     to_index += obj->ArgumentCount();
   }
 
   // Populate stack frames.
   for (intptr_t to_index = frame_size - 1, from_index = len - 1;
        to_index >= 0;
        to_index--, from_index--) {
-    intptr_t* to_addr = deopt_context.GetToFrameAddressAt(to_index);
-    deopt_instructions[from_index]->Execute(&deopt_context, to_addr);
+    intptr_t* to_addr = deopt_context->GetToFrameAddressAt(to_index);
+    deopt_instructions[from_index]->Execute(deopt_context, to_addr);
   }
 
   if (FLAG_trace_deoptimization_verbose) {
     for (intptr_t i = 0; i < frame_size; i++) {
       OS::PrintErr("*%" Pd ". [%" Px "] %#014" Px " [%s]\n",
                    i,
                    reinterpret_cast<uword>(&start[i]),
                    start[i],
                    deopt_instructions[i + (len - frame_size)]->ToCString());
     }
@@ skipping 11 matching lines @@
   DartFrameIterator iterator(last_fp);
   StackFrame* caller_frame = iterator.NextFrame();
   ASSERT(caller_frame != NULL);
   const Code& optimized_code = Code::Handle(caller_frame->LookupDartCode());
   const Function& function = Function::Handle(optimized_code.function());
   ASSERT(!function.IsNull());
   const Code& unoptimized_code = Code::Handle(function.unoptimized_code());
   ASSERT(!optimized_code.IsNull() && optimized_code.is_optimized());
   ASSERT(!unoptimized_code.IsNull() && !unoptimized_code.is_optimized());
 
-  intptr_t* frame_copy = isolate->deopt_frame_copy();
-  intptr_t* cpu_registers_copy = isolate->deopt_cpu_registers_copy();
-  fpu_register_t* fpu_registers_copy = isolate->deopt_fpu_registers_copy();
-
   intptr_t deopt_reason = kDeoptUnknown;
   const DeoptInfo& deopt_info = DeoptInfo::Handle(
       optimized_code.GetDeoptInfoAtPc(caller_frame->pc(), &deopt_reason));
   ASSERT(!deopt_info.IsNull());
 
-  DeoptimizeWithDeoptInfo(optimized_code,
+  DeoptContext* deopt_context = isolate->deopt_context();
+  DeoptimizeWithDeoptInfo(deopt_context,
+                          optimized_code,
                           deopt_info,
-                          *caller_frame,
-                          deopt_reason);
-
-  isolate->SetDeoptFrameCopy(NULL, 0);
-  isolate->set_deopt_cpu_registers_copy(NULL);
-  isolate->set_deopt_fpu_registers_copy(NULL);
-  delete[] frame_copy;
-  delete[] cpu_registers_copy;
-  delete[] fpu_registers_copy;
+                          *caller_frame);
 }
 END_LEAF_RUNTIME_ENTRY
 
 
 // This is the last step in the deoptimization, GC can occur.
 // Returns number of bytes to remove from the expression stack of the
 // bottom-most deoptimized frame. Those arguments were artificially injected
 // under return address to keep them discoverable by GC that can occur during
 // materialization phase.
 DEFINE_RUNTIME_ENTRY(DeoptimizeMaterialize, 0) {
-  // First materialize all unboxed "primitive" values (doubles, mints, simd)
-  // then materialize objects. The order is important: objects might be
-  // referencing boxes allocated on the first step. At the same time
-  // objects can't be referencing other deferred objects because storing
-  // an object into a field is always conservatively treated as escaping by
-  // allocation sinking and load forwarding.
-  isolate->MaterializeDeferredBoxes();
-  isolate->MaterializeDeferredObjects();
+  DeoptContext* deopt_context = isolate->deopt_context();
 
-  // Compute total number of artificial arguments used during deoptimization.
-  intptr_t deopt_arguments = 0;
-  for (intptr_t i = 0; i < isolate->DeferredObjectsCount(); i++) {
-    deopt_arguments += isolate->GetDeferredObject(i)->ArgumentCount();
-  }
-  Isolate::Current()->DeleteDeferredObjects();
+  intptr_t deopt_arg_count = deopt_context->MaterializeDeferredObjects();
+
+  // Delete temporary copies of frame and registers.
+  isolate->set_deopt_context(NULL);
+  delete[] deopt_context->from_frame();  // Allocated in CopyFrame.
+  delete[] deopt_context->fpu_registers();  // Allocated in CopySavedRegisters.
+  delete[] deopt_context->cpu_registers();  // Allocated in CopySavedRegisters.
+  delete deopt_context;

[Florian Schneider, 2013/09/30 11:32:29]

Maybe change the destructor of DeoptContext so that it deletes everything
attached to it including fpu_registers_, cpu_registers_, and from_frame_

[turnidge, 2013/09/30 17:40:22]

I agree that it is unsightly to have this memory management here, but here's
some more information...

I have another change coming where from_frame_ isn't a copy -- it's the actual
stack.  This will be used by the debugging api to access variables from
optimized/inlined frames.  In that case, it would be wrong to deallocate
from_frame_.  Since there will be no consistent ownership story across both
uses, I chose to leave memory ownership outside of the class.

[Ivan Posva, 2013/09/30 18:00:01]

Would it be possible to keep track whether these should be deleted in the
DeoptContext object and then have the destructor conditionally delete them?

[turnidge, 2013/09/30 19:35:10]

You and Florian are right here.  Changed.

On 2013/09/30 18:00:01, Ivan Posva wrote:

 
   // Return value tells deoptimization stub to remove the given number of bytes
   // from the stack.
-  arguments.SetReturn(Smi::Handle(Smi::New(deopt_arguments * kWordSize)));
+  arguments.SetReturn(Smi::Handle(Smi::New(deopt_arg_count * kWordSize)));
 
   // Since this is the only step where GC can occur during deoptimization,
   // use it to report the source line where deoptimization occured.
   if (FLAG_trace_deoptimization || FLAG_trace_deoptimization_verbose) {
     DartFrameIterator iterator;
     StackFrame* top_frame = iterator.NextFrame();
     ASSERT(top_frame != NULL);
     const Code& code = Code::Handle(top_frame->LookupDartCode());
     const Function& top_function = Function::Handle(code.function());
     const Script& script = Script::Handle(top_function.script());
     const intptr_t token_pos = code.GetTokenIndexOfPC(top_frame->pc());
     intptr_t line, column;
     script.GetTokenLocation(token_pos, &line, &column);
     String& line_string = String::Handle(script.GetLine(line));
     OS::PrintErr("  Function: %s\n", top_function.ToFullyQualifiedCString());
     OS::PrintErr("  Line %" Pd ": '%s'\n", line, line_string.ToCString());
-    OS::PrintErr("  Deopt args: %" Pd "\n", deopt_arguments);
+    OS::PrintErr("  Deopt args: %" Pd "\n", deopt_arg_count);
   }
 }
 
 
 DEFINE_LEAF_RUNTIME_ENTRY(intptr_t,
                           BigintCompare,
                           2,
                           RawBigint* left,
                           RawBigint* right) {
   Isolate* isolate = Isolate::Current();
@@ skipping 58 matching lines @@
   field.UpdateCid(cid);
   intptr_t list_length = Field::kNoFixedLength;
   if ((field.guarded_cid() != kDynamicCid) &&
       field.is_final() && RawObject::IsBuiltinListClassId(cid)) {
     list_length = GetListLength(value);
   }
   field.UpdateLength(list_length);
 }
 
 }  // namespace dart