Improve internal compiler API so that OSR code is never installed on function.

runtime/vm/compiler.cc

Index: runtime/vm/compiler.cc
diff --git a/runtime/vm/compiler.cc b/runtime/vm/compiler.cc
index 0a452b337b1a412b47d1b82ca76ea63fc7d60420..1dcc199068693299b495e4c640a42832685c9424 100644
--- a/runtime/vm/compiler.cc
+++ b/runtime/vm/compiler.cc
@@ -204,14 +204,14 @@ CompilationPipeline* CompilationPipeline::New(Zone* zone,
 DEFINE_RUNTIME_ENTRY(CompileFunction, 1) {
   const Function& function = Function::CheckedHandle(arguments.ArgAt(0));
   ASSERT(!function.HasCode());
-  const Error& error =
-      Error::Handle(Compiler::CompileFunction(thread, function));
-  if (!error.IsNull()) {
-    if (error.IsLanguageError()) {
-      Exceptions::ThrowCompileTimeError(LanguageError::Cast(error));
+  const Object& result =
+      Object::Handle(Compiler::CompileFunction(thread, function));
+  if (result.IsError()) {
+    if (result.IsLanguageError()) {
+      Exceptions::ThrowCompileTimeError(LanguageError::Cast(result));
       UNREACHABLE();
     }
-    Exceptions::PropagateError(error);
+    Exceptions::PropagateError(Error::Cast(result));
   }
 }
 
@@ -496,7 +496,7 @@ class CompileParsedFunctionHelper : public ValueObject {
         loading_invalidation_gen_at_start_(
             isolate()->loading_invalidation_gen()) {}
 
-  bool Compile(CompilationPipeline* pipeline);
+  RawObject* Compile(CompilationPipeline* pipeline);
 
  private:
   ParsedFunction* parsed_function() const { return parsed_function_; }
@@ -507,9 +507,9 @@ class CompileParsedFunctionHelper : public ValueObject {
   intptr_t loading_invalidation_gen_at_start() const {
     return loading_invalidation_gen_at_start_;
   }
-  void FinalizeCompilation(Assembler* assembler,
-                           FlowGraphCompiler* graph_compiler,
-                           FlowGraph* flow_graph);
+  RawObject* FinalizeCompilation(Assembler* assembler,
+                                 FlowGraphCompiler* graph_compiler,
+                                 FlowGraph* flow_graph);
   void CheckIfBackgroundCompilerIsBeingStopped();
 
   ParsedFunction* parsed_function_;
@@ -522,7 +522,7 @@ class CompileParsedFunctionHelper : public ValueObject {
 };
 
 
-void CompileParsedFunctionHelper::FinalizeCompilation(
+RawObject* CompileParsedFunctionHelper::FinalizeCompilation(

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

It looks like this function always returns a Code - so make it RawCode*

[erikcorry, 2017/03/30 12:26:33]

Done.

     Assembler* assembler,
     FlowGraphCompiler* graph_compiler,
     FlowGraph* flow_graph) {
@@ -587,13 +587,16 @@ void CompileParsedFunctionHelper::FinalizeCompilation(
   graph_compiler->FinalizeStaticCallTargetsTable(code);
   graph_compiler->FinalizeCodeSourceMap(code);
 
+  bool code_was_compiled = false;

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

Maybe you don't need this? 

How about just using code and setting it to `null` if validation failed.

[erikcorry, 2017/03/30 12:26:33]

Done.

+
   if (optimized()) {
-    bool code_was_installed = false;
     // Installs code while at safepoint.
     if (thread()->IsMutatorThread()) {
       const bool is_osr = osr_id() != Compiler::kNoOSRDeoptId;
-      function.InstallOptimizedCode(code, is_osr);
-      code_was_installed = true;
+      if (!is_osr) {

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

InstallOptimizedCode() also does set the owner on the code (in the AttachCode).

Who does that for the OSR code? Maybe this needs to be done manually. 

[erikcorry, 2017/03/30 12:26:34]

That's done on line 545.  I added an assert.

+        function.InstallOptimizedCode(code);
+      }
+      code_was_compiled = true;
     } else {
       // Background compilation.
       // Before installing code check generation counts if the code may
@@ -637,8 +640,8 @@ void CompileParsedFunctionHelper::FinalizeCompilation(
       if (code_is_valid && Compiler::CanOptimizeFunction(thread(), function)) {
         const bool is_osr = osr_id() != Compiler::kNoOSRDeoptId;
         ASSERT(!is_osr);  // OSR is not compiled in background.
-        function.InstallOptimizedCode(code, is_osr);
-        code_was_installed = true;
+        function.InstallOptimizedCode(code);
+        code_was_compiled = true;
       }

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

} else {
  code = Code::null();
}

and use !code.IsNull() everywhere instead of code_was_compiled.

[erikcorry, 2017/03/30 12:26:34]

Done.

       if (function.usage_counter() < 0) {
         // Reset to 0 so that it can be recompiled if needed.
@@ -651,7 +654,7 @@ void CompileParsedFunctionHelper::FinalizeCompilation(
       }
     }
 
-    if (code_was_installed) {
+    if (code_was_compiled) {
       // The generated code was compiled under certain assumptions about
       // class hierarchy and field types. Register these dependencies
       // to ensure that the code will be deoptimized if they are violated.
@@ -673,6 +676,7 @@ void CompileParsedFunctionHelper::FinalizeCompilation(
     }
     function.set_unoptimized_code(code);
     function.AttachCode(code);
+    code_was_compiled = true;
   }
   if (parsed_function()->HasDeferredPrefixes()) {
     ASSERT(!FLAG_load_deferred_eagerly);
@@ -682,6 +686,10 @@ void CompileParsedFunctionHelper::FinalizeCompilation(
       (*prefixes)[i]->RegisterDependentCode(code);
     }
   }
+  if (code_was_compiled) {
+    return code.raw();
+  }
+  return Object::null();

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

here you then will be able to just return code.raw().

[erikcorry, 2017/03/30 12:26:34]

Done.

 }
 
 
@@ -695,16 +703,15 @@ void CompileParsedFunctionHelper::CheckIfBackgroundCompilerIsBeingStopped() {
 }
 
 
-// Return false if bailed out.
+// Return null if bailed out.
 // If optimized_result_code is not NULL then it is caller's responsibility
 // to install code.
-bool CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
+RawObject* CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

This returns Code or null. So maybe this should be RawCode*?

[erikcorry, 2017/03/30 12:26:33]

Done.

   ASSERT(!FLAG_precompiled_mode);
   const Function& function = parsed_function()->function();
   if (optimized() && !function.IsOptimizable()) {
-    return false;
+    return Object::null();
   }
-  bool is_compiled = false;
   Zone* const zone = thread()->zone();
   NOT_IN_PRODUCT(TimelineStream* compiler_timeline =
                      Timeline::GetCompilerStream());
@@ -722,7 +729,9 @@ bool CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
   volatile bool use_far_branches = false;
   const bool use_speculative_inlining = false;
 
+  Code& result = Code::ZoneHandle(zone);

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

this variable needs to be volatile because of setjmp(...).

[erikcorry, 2017/03/30 12:26:33]

Yuck!

   while (!done) {
+    result = Code::null();
     const intptr_t prev_deopt_id = thread()->deopt_id();
     thread()->set_deopt_id(0);
     LongJumpScope jump;
@@ -1135,7 +1144,8 @@ bool CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
         NOT_IN_PRODUCT(TimelineDurationScope tds(thread(), compiler_timeline,
                                                  "FinalizeCompilation"));
         if (thread()->IsMutatorThread()) {
-          FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
+          result ^=
+              FinalizeCompilation(&assembler, &graph_compiler, flow_graph);

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

This can become result = FinalizeCompilation() if you change FinalizeCompilation
to just return RawCode* - which I think it always does.

[erikcorry, 2017/03/30 12:26:33]

Done.

         } else {
           // This part of compilation must be at a safepoint.
           // Stop mutator thread before creating the instruction object and
@@ -1151,7 +1161,8 @@ bool CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
             // heap to grow.
             NoHeapGrowthControlScope no_growth_control;
             CheckIfBackgroundCompilerIsBeingStopped();
-            FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
+            result ^=
+                FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
           }
           // TODO(srdjan): Enable this and remove the one from
           // 'BackgroundCompiler::CompileOptimized' once cause of time-outs
@@ -1162,7 +1173,6 @@ bool CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
         }
       }
       // Exit the loop and the function with the correct result value.
-      is_compiled = true;
       done = true;
     } else {
       // We bailed out or we encountered an error.
@@ -1179,8 +1189,7 @@ bool CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
         UNREACHABLE();
       } else {
         // If the error isn't due to an out of range branch offset, we don't
-        // try again (done = true), and indicate that we did not finish
-        // compiling (is_compiled = false).
+        // try again (done = true).
         if (FLAG_trace_bailout) {
           THR_Print("%s\n", error.ToErrorCString());
         }
@@ -1194,19 +1203,18 @@ bool CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
           (LanguageError::Cast(error).kind() == Report::kBailout)) {
         thread()->clear_sticky_error();
       }
-      is_compiled = false;
     }
     // Reset global isolate state.
     thread()->set_deopt_id(prev_deopt_id);
   }
-  return is_compiled;
+  return result.raw();
 }
 
 
-static RawError* CompileFunctionHelper(CompilationPipeline* pipeline,
-                                       const Function& function,
-                                       bool optimized,
-                                       intptr_t osr_id) {
+static RawObject* CompileFunctionHelper(CompilationPipeline* pipeline,
+                                        const Function& function,
+                                        bool optimized,
+                                        intptr_t osr_id) {
   ASSERT(!FLAG_precompiled_mode);
   ASSERT(!optimized || function.was_compiled());
   LongJumpScope jump;
@@ -1262,8 +1270,8 @@ static RawError* CompileFunctionHelper(CompilationPipeline* pipeline,
       }
     }
 
-    const bool success = helper.Compile(pipeline);
-    if (success) {
+    Object& result = Object::Handle(helper.Compile(pipeline));

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

This probably needs to be Code because you expect it to be either Code object or
null.

[erikcorry, 2017/03/30 12:26:33]

Done.

+    if (!result.IsNull()) {
       if (!optimized) {
         function.set_was_compiled(true);
       }
@@ -1305,6 +1313,7 @@ static RawError* CompileFunctionHelper(CompilationPipeline* pipeline,
         function.SetIsOptimizable(false);
         return Error::null();
       } else {
+        ASSERT(!optimized);

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

I don't get this code. We are inside 

if (optimized) {
  ...
}

condition. 

[erikcorry, 2017/03/30 12:26:33]

No.  I added the assert exactly to clarify which "{" the "}" belonged to.

         // Encountered error.
         Error& error = Error::Handle();
         // We got an error during compilation.
@@ -1318,11 +1327,12 @@ static RawError* CompileFunctionHelper(CompilationPipeline* pipeline,
                 LanguageError::Cast(error).kind() != Report::kBailout));
         return error.raw();
       }
+      UNREACHABLE();
     }
 
     per_compile_timer.Stop();
 
-    if (trace_compiler && success) {
+    if (trace_compiler) {
       THR_Print("--> '%s' entry: %#" Px " size: %" Pd " time: %" Pd64 " us\n",
                 function.ToFullyQualifiedCString(),
                 Code::Handle(function.CurrentCode()).PayloadStart(),
@@ -1341,7 +1351,7 @@ static RawError* CompileFunctionHelper(CompilationPipeline* pipeline,
       Disassembler::DisassembleCode(function, true);
     }
 
-    return Error::null();
+    return result.raw();
   } else {
     Thread* const thread = Thread::Current();
     StackZone stack_zone(thread);
@@ -1424,7 +1434,7 @@ static RawError* ParseFunctionHelper(CompilationPipeline* pipeline,
 }
 
 
-RawError* Compiler::CompileFunction(Thread* thread, const Function& function) {
+RawObject* Compiler::CompileFunction(Thread* thread, const Function& function) {
 #ifdef DART_PRECOMPILER
   if (FLAG_precompiled_mode) {
     return Precompiler::CompileFunction(
@@ -1487,19 +1497,20 @@ RawError* Compiler::EnsureUnoptimizedCode(Thread* thread,
   }
   CompilationPipeline* pipeline =
       CompilationPipeline::New(thread->zone(), function);
-  const Error& error = Error::Handle(
+  const Object& result = Object::Handle(
       CompileFunctionHelper(pipeline, function, false, /* not optimized */
                             kNoOSRDeoptId));
-  if (!error.IsNull()) {
-    return error.raw();
+  if (result.IsError()) {
+    return Error::Cast(result).raw();
   }
   // Since CompileFunctionHelper replaces the current code, re-attach the
   // the original code if the function was already compiled.
-  if (!original_code.IsNull() &&
-      (original_code.raw() != function.CurrentCode())) {
+  if (!original_code.IsNull() && result.raw() == function.CurrentCode() &&
+      !original_code.IsDisabled()) {
     function.AttachCode(original_code);
   }
   ASSERT(function.unoptimized_code() != Object::null());
+  ASSERT(function.unoptimized_code() == result.raw());
   if (FLAG_trace_compiler) {
     THR_Print("Ensure unoptimized code for %s\n", function.ToCString());
   }
@@ -1507,9 +1518,9 @@ RawError* Compiler::EnsureUnoptimizedCode(Thread* thread,
 }
 
 
-RawError* Compiler::CompileOptimizedFunction(Thread* thread,
-                                             const Function& function,
-                                             intptr_t osr_id) {
+RawObject* Compiler::CompileOptimizedFunction(Thread* thread,
+                                              const Function& function,
+                                              intptr_t osr_id) {
 #if !defined(PRODUCT)
   VMTagScope tagScope(thread, VMTag::kCompileOptimizedTagId);
   const char* event_name;
@@ -1592,14 +1603,14 @@ void Compiler::ComputeLocalVarDescriptors(const Code& code) {
 RawError* Compiler::CompileAllFunctions(const Class& cls) {
   Thread* thread = Thread::Current();
   Zone* zone = thread->zone();
-  Error& error = Error::Handle(zone);
+  Object& result = Object::Handle(zone);
   Array& functions = Array::Handle(zone, cls.functions());
   Function& func = Function::Handle(zone);
   // Class dynamic lives in the vm isolate. Its array fields cannot be set to
   // an empty array.
   if (functions.IsNull()) {
     ASSERT(cls.IsDynamicClass());
-    return error.raw();
+    return Error::null();
   }
   // Compile all the regular functions.
   for (int i = 0; i < functions.Length(); i++) {
@@ -1612,15 +1623,16 @@ RawError* Compiler::CompileAllFunctions(const Class& cls) {
         // Skipping optional parameters in mixin application.
         continue;
       }
-      error = CompileFunction(thread, func);
-      if (!error.IsNull()) {
-        return error.raw();
+      result = CompileFunction(thread, func);
+      if (result.IsError()) {
+        return Error::Cast(result).raw();
       }
+      ASSERT(!result.IsNull());
       func.ClearICDataArray();
       func.ClearCode();
     }
   }
-  return error.raw();
+  return Error::null();
 }
 
 
@@ -2197,7 +2209,7 @@ RawError* Compiler::CompileClass(const Class& cls) {
 }
 
 
-RawError* Compiler::CompileFunction(Thread* thread, const Function& function) {
+RawObject* Compiler::CompileFunction(Thread* thread, const Function& function) {
   UNREACHABLE();
   return Error::null();
 }