[vm/bytecode] Prevent triggering optimizing compilation from interpreter

Previously, in rare cases it was possible that interpreter would trigger
optimizing compilation (if function usage counter reached threshold
in the interpreter once again after unoptimized compilation finished).

This results in producing optimized code without any feedback (as
unoptimized code hasn't executed).

Change-Id: Ibd52b7dfe3a808ade97c02e743b40aa466b1e3c2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108687
Commit-Queue: Alexander Markov <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>
Reviewed-by: Régis Crelier <[email protected]>

Author: alexmarkov

runtime/vm/compiler/jit/compiler.cc

@@ -235,7 +235,7 @@ DEFINE_RUNTIME_ENTRY(CompileFunction, 1) {
       // If interpreter is enabled and there is bytecode, LazyCompile stub
       // (which calls CompileFunction) should proceed to InterpretCall in order
       // to enter interpreter. In such case, compilation is postponed and
-      // triggered by interpreter later via OptimizeInvokedFunction.
+      // triggered by interpreter later via CompileInterpretedFunction.
       return;
     }
     // No bytecode, fall back to compilation.
@@ -1226,13 +1226,14 @@ class BackgroundCompilationQueue {
   DISALLOW_COPY_AND_ASSIGN(BackgroundCompilationQueue);
 };
 
-BackgroundCompiler::BackgroundCompiler(Isolate* isolate)
+BackgroundCompiler::BackgroundCompiler(Isolate* isolate, bool optimizing)
     : isolate_(isolate),
       queue_monitor_(),
       function_queue_(new BackgroundCompilationQueue()),
       done_monitor_(),
       running_(false),
       done_(true),
+      optimizing_(optimizing),
       disabled_depth_(0) {}
 
 // Fields all deleted in ::Stop; here clear them.
@@ -1257,14 +1258,11 @@ void BackgroundCompiler::Run() {
         function = function_queue()->PeekFunction();
       }
       while (running_ && !function.IsNull()) {
-        // This is false if we are compiling bytecode -> unoptimized code.
-        const bool optimizing = function.ShouldCompilerOptimize();
-        ASSERT(FLAG_enable_interpreter || optimizing);
-
-        if (optimizing) {
+        if (is_optimizing()) {
           Compiler::CompileOptimizedFunction(thread, function,
                                              Compiler::kNoOSRDeoptId);
         } else {
+          ASSERT(FLAG_enable_interpreter);
           Compiler::CompileFunction(thread, function);
         }
 
@@ -1279,7 +1277,7 @@ void BackgroundCompiler::Run() {
             const Function& old = Function::Handle(qelem->Function());
             // If an optimizable method is not optimized, put it back on
             // the background queue (unless it was passed to foreground).
-            if ((optimizing && !old.HasOptimizedCode() &&
+            if ((is_optimizing() && !old.HasOptimizedCode() &&
                  old.IsOptimizable()) ||
                 FLAG_stress_test_background_compilation) {
               if (old.is_background_optimizable() &&

runtime/vm/compiler/jit/compiler.h

@@ -130,7 +130,7 @@ class Compiler : public AllStatic {
 // No OSR compilation in the background compiler.
 class BackgroundCompiler {
  public:
-  explicit BackgroundCompiler(Isolate* isolate);
+  explicit BackgroundCompiler(Isolate* isolate, bool optimizing);
   virtual ~BackgroundCompiler();
 
   static void Start(Isolate* isolate) {
@@ -191,6 +191,7 @@ class BackgroundCompiler {
 
   BackgroundCompilationQueue* function_queue() const { return function_queue_; }
   bool is_running() const { return running_; }
+  bool is_optimizing() const { return optimizing_; }
 
   void Run();
 
@@ -210,6 +211,7 @@ class BackgroundCompiler {
   Monitor done_monitor_;    // Notify/wait that the thread is done.
   bool running_;            // While true, will try to read queue and compile.
   bool done_;               // True if the thread is done.
+  bool optimizing_;
 
   int16_t disabled_depth_;
 

runtime/vm/compiler/runtime_offsets_extracted.h

@@ -1133,11 +1133,11 @@ DART_FORCE_INLINE bool Interpreter::InstanceCall2(Thread* thread,
     if (UNLIKELY(FLAG_compilation_counter_threshold >= 0 &&                    \
                  counter >= FLAG_compilation_counter_threshold &&              \
                  !Function::HasCode(function))) {                              \
-      SP[1] = 0; /* Unused code result. */                                     \
+      SP[1] = 0; /* Unused result. */                                          \
       SP[2] = function;                                                        \
       Exit(thread, FP, SP + 3, pc);                                            \
       NativeArguments native_args(thread, 1, SP + 2, SP + 1);                  \
-      INVOKE_RUNTIME(DRT_OptimizeInvokedFunction, native_args);                \
+      INVOKE_RUNTIME(DRT_CompileInterpretedFunction, native_args);             \
       function = FrameFunction(FP);                                            \
     }                                                                          \
   }
@@ -1766,11 +1766,11 @@ RawObject* Interpreter::Call(RawFunction* function,
     if (UNLIKELY(FLAG_compilation_counter_threshold >= 0 &&
                  counter >= FLAG_compilation_counter_threshold &&
                  !Function::HasCode(function))) {
-      SP[1] = 0;  // Unused code result.
+      SP[1] = 0;  // Unused result.
       SP[2] = function;
       Exit(thread, FP, SP + 3, pc);
       NativeArguments native_args(thread, 1, SP + 2, SP + 1);
-      INVOKE_RUNTIME(DRT_OptimizeInvokedFunction, native_args);
+      INVOKE_RUNTIME(DRT_CompileInterpretedFunction, native_args);
     }
     DISPATCH();
   }

runtime/vm/interpreter.cc

@@ -963,10 +963,11 @@ Isolate::Isolate(IsolateGroup* isolate_group,
   }
 
   if (FLAG_enable_interpreter) {
-    NOT_IN_PRECOMPILED(background_compiler_ = new BackgroundCompiler(this));
+    NOT_IN_PRECOMPILED(background_compiler_ = new BackgroundCompiler(
+                           this, /* optimizing = */ false));
   }
   NOT_IN_PRECOMPILED(optimizing_background_compiler_ =
-                         new BackgroundCompiler(this));
+                         new BackgroundCompiler(this, /* optimizing = */ true));
 
   isolate_group->RegisterIsolate(this);
   isolate_group_ = isolate_group;

runtime/vm/isolate.cc

@@ -2292,28 +2292,56 @@ DEFINE_RUNTIME_ENTRY(TraceICCall, 2) {
                ic_data.NumberOfChecks(), function.ToFullyQualifiedCString());
 }
 
+// This is called from interpreter when function usage counter reached
+// compilation threshold and function needs to be compiled.
+DEFINE_RUNTIME_ENTRY(CompileInterpretedFunction, 1) {
+#if !defined(DART_PRECOMPILED_RUNTIME)
+  const Function& function = Function::CheckedHandle(zone, arguments.ArgAt(0));
+  ASSERT(!function.IsNull());
+  ASSERT(FLAG_enable_interpreter);
+
+#if !defined(PRODUCT)
+  if (Debugger::IsDebugging(thread, function)) {
+    return;
+  }
+#endif  // !defined(PRODUCT)
+
+  if (FLAG_background_compilation) {
+    if (!BackgroundCompiler::IsDisabled(isolate,
+                                        /* optimizing_compilation = */ false) &&
+        function.is_background_optimizable()) {
+      // Ensure background compiler is running, if not start it.
+      BackgroundCompiler::Start(isolate);
+      // Reduce the chance of triggering a compilation while the function is
+      // being compiled in the background. INT_MIN should ensure that it
+      // takes long time to trigger a compilation.
+      // Note that the background compilation queue rejects duplicate entries.
+      function.SetUsageCounter(INT_MIN);
+      isolate->background_compiler()->Compile(function);
+      return;
+    }
+  }
+
+  // Reset usage counter for future optimization.
+  function.SetUsageCounter(0);
+  Object& result =
+      Object::Handle(zone, Compiler::CompileFunction(thread, function));
+  ThrowIfError(result);
+#else
+  UNREACHABLE();
+#endif  // !DART_PRECOMPILED_RUNTIME
+}
+
 // This is called from function that needs to be optimized.
 // The requesting function can be already optimized (reoptimization).
 // Returns the Code object where to continue execution.
 DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
 #if !defined(DART_PRECOMPILED_RUNTIME)
   const Function& function = Function::CheckedHandle(zone, arguments.ArgAt(0));
   ASSERT(!function.IsNull());
+  ASSERT(function.HasCode());
 
-  // If running with interpreter, do the unoptimized compilation first.
-  const bool optimizing_compilation = function.ShouldCompilerOptimize();
-  ASSERT(FLAG_enable_interpreter || optimizing_compilation);
-  ASSERT((!optimizing_compilation) || function.HasCode() ||
-         function.ForceOptimize());
-
-#if defined(PRODUCT)
-  if (!optimizing_compilation ||
-      Compiler::CanOptimizeFunction(thread, function)) {
-#else
-  if ((!optimizing_compilation && !Debugger::IsDebugging(thread, function)) ||
-      (optimizing_compilation &&
-       Compiler::CanOptimizeFunction(thread, function))) {
-#endif  // defined(PRODUCT)
+  if (Compiler::CanOptimizeFunction(thread, function)) {
     if (FLAG_background_compilation) {
       if (FLAG_enable_inlining_annotations) {
         FATAL("Cannot enable inlining annotations and background compilation");
@@ -2328,7 +2356,8 @@ DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
         // Get next field.
         field = isolate->GetDeoptimizingBoxedField();
       }
-      if (!BackgroundCompiler::IsDisabled(isolate, optimizing_compilation) &&
+      if (!BackgroundCompiler::IsDisabled(isolate,
+                                          /* optimizing_compiler = */ true) &&
           function.is_background_optimizable()) {
         // Ensure background compiler is running, if not start it.
         BackgroundCompiler::Start(isolate);
@@ -2337,12 +2366,7 @@ DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
         // takes long time to trigger a compilation.
         // Note that the background compilation queue rejects duplicate entries.
         function.SetUsageCounter(INT_MIN);
-        if (optimizing_compilation) {
-          isolate->optimizing_background_compiler()->Compile(function);
-        } else {
-          ASSERT(FLAG_enable_interpreter);
-          isolate->background_compiler()->Compile(function);
-        }
+        isolate->optimizing_background_compiler()->Compile(function);
         // Continue in the same code.
         arguments.SetReturn(function);
         return;
@@ -2358,12 +2382,8 @@ DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
                   function.ToFullyQualifiedCString());
       }
     }
-    Object& result = Object::Handle(zone);
-    if (optimizing_compilation) {
-      result = Compiler::CompileOptimizedFunction(thread, function);
-    } else {
-      result = Compiler::CompileFunction(thread, function);
-    }
+    Object& result = Object::Handle(
+        zone, Compiler::CompileOptimizedFunction(thread, function));
     ThrowIfError(result);
   }
   arguments.SetReturn(function);

runtime/vm/runtime_entry.cc

@@ -54,6 +54,7 @@ namespace dart {
   V(UpdateFieldCid)                                                            \
   V(InitStaticField)                                                           \
   V(CompileFunction)                                                           \
+  V(CompileInterpretedFunction)                                                \
   V(MonomorphicMiss)                                                           \
   V(SingleTargetMiss)                                                          \
   V(UnlinkedCall)