[vm, gc] Ensure BackgroundCompiler can't be deleted during a safepoint.

Previously, the background compiler was deleted by the thread pool, which could have raced ahead of the mutator clearing its reference. Now it is deleted by the mutator.

Change-Id: Id353a96022f699131ee60fe31156cd0404dfd285
Reviewed-on: https://dart-review.googlesource.com/24342
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>

runtime/vm/clustered_snapshot.cc

@@ -5619,8 +5619,6 @@ FullSnapshotWriter::FullSnapshotWriter(Snapshot::Kind kind,
   isolate()->ValidateClassTable();
   isolate()->ValidateConstants();
 #endif  // DEBUG
-  // Can't have any mutation happening while we're serializing.
-  ASSERT(isolate()->background_compiler() == NULL);
 
   // TODO(rmacnak): The special case for AOT causes us to always generate the
   // same VM isolate snapshot for every app. AOT snapshots should be cleaned up

runtime/vm/compiler/jit/compiler.cc

@@ -1604,7 +1604,8 @@ RawObject* Compiler::CompileOptimizedFunction(Thread* thread,
   // this is either an OSR compilation or background compilation is
   // not currently allowed.
   ASSERT(!thread->IsMutatorThread() || (osr_id != kNoOSRDeoptId) ||
-         !FLAG_background_compilation || BackgroundCompiler::IsDisabled());
+         !FLAG_background_compilation ||
+         BackgroundCompiler::IsDisabled(Isolate::Current()));
   CompilationPipeline* pipeline =
       CompilationPipeline::New(thread->zone(), function);
   return CompileFunctionHelper(pipeline, function, true, /* optimized */
@@ -2016,22 +2017,18 @@ class BackgroundCompilationQueue {
 
 BackgroundCompiler::BackgroundCompiler(Isolate* isolate)
     : isolate_(isolate),
-      running_(true),
-      done_(new bool()),
       queue_monitor_(new Monitor()),
+      function_queue_(new BackgroundCompilationQueue()),
       done_monitor_(new Monitor()),
-      function_queue_(new BackgroundCompilationQueue()) {
-  *done_ = false;
-}
+      running_(false),
+      done_(true),
+      disabled_depth_(0) {}
 
 // Fields all deleted in ::Stop; here clear them.
 BackgroundCompiler::~BackgroundCompiler() {
-  isolate_ = NULL;
-  running_ = false;
-  done_ = NULL;
-  queue_monitor_ = NULL;
-  done_monitor_ = NULL;
-  function_queue_ = NULL;
+  delete queue_monitor_;
+  delete function_queue_;
+  delete done_monitor_;
 }
 
 void BackgroundCompiler::Run() {
@@ -2099,7 +2096,7 @@ void BackgroundCompiler::Run() {
   {
     // Notify that the thread is done.
     MonitorLocker ml_done(done_monitor_);
-    *done_ = true;
+    done_ = true;
     ml_done.Notify();
   }
 }
@@ -2127,74 +2124,25 @@ void BackgroundCompiler::VisitPointers(ObjectPointerVisitor* visitor) {
   function_queue_->VisitObjectPointers(visitor);
 }
 
-void BackgroundCompiler::Stop(Isolate* isolate) {
-  BackgroundCompiler* task = isolate->background_compiler();
-  if (task == NULL) {
-    // Nothing to stop.
-    return;
-  }
-  BackgroundCompilationQueue* function_queue = task->function_queue();
+class BackgroundCompilerTask : public ThreadPool::Task {
+ public:
+  explicit BackgroundCompilerTask(BackgroundCompiler* background_compiler)
+      : background_compiler_(background_compiler) {}
+  virtual ~BackgroundCompilerTask() {}
 
-  Monitor* queue_monitor = task->queue_monitor_;
-  Monitor* done_monitor = task->done_monitor_;
-  bool* task_done = task->done_;
-  // Wake up compiler task and stop it.
-  {
-    MonitorLocker ml(queue_monitor);
-    task->running_ = false;
-    function_queue->Clear();
-    // 'task' will be deleted by thread pool.
-    task = NULL;
-    ml.Notify();  // Stop waiting for the queue.
-  }
+ private:
+  virtual void Run() { background_compiler_->Run(); }
 
-  {
-    MonitorLocker ml_done(done_monitor);
-    while (!(*task_done)) {
-      ml_done.WaitWithSafepointCheck(Thread::Current());
-    }
-  }
-  delete task_done;
-  delete done_monitor;
-  delete queue_monitor;
-  delete function_queue;
-  isolate->set_background_compiler(NULL);
-}
+  BackgroundCompiler* background_compiler_;
 
-void BackgroundCompiler::Disable() {
+  DISALLOW_COPY_AND_ASSIGN(BackgroundCompilerTask);
+};
+
+void BackgroundCompiler::Start() {
   Thread* thread = Thread::Current();
-  ASSERT(thread != NULL);
-  Isolate* isolate = thread->isolate();
-  MutexLocker ml(isolate->mutex());
-  BackgroundCompiler* task = isolate->background_compiler();
-  if (task != NULL) {
-    // We should only ever have to stop the task if this is the first call to
-    // Disable.
-    ASSERT(!isolate->is_background_compiler_disabled());
-    BackgroundCompiler::Stop(isolate);
-  }
-  ASSERT(isolate->background_compiler() == NULL);
-  isolate->disable_background_compiler();
-}
-
-bool BackgroundCompiler::IsDisabled() {
-  Thread* thread = Thread::Current();
-  ASSERT(thread != NULL);
-  Isolate* isolate = thread->isolate();
-  MutexLocker ml(isolate->mutex());
-  return isolate->is_background_compiler_disabled();
-}
-
-void BackgroundCompiler::Enable() {
-  Thread* thread = Thread::Current();
-  ASSERT(thread != NULL);
-  Isolate* isolate = thread->isolate();
-  MutexLocker ml(isolate->mutex());
-  isolate->enable_background_compiler();
-}
-
-void BackgroundCompiler::EnsureInit(Thread* thread) {
   ASSERT(thread->IsMutatorThread());
+  ASSERT(!thread->IsAtSafepoint());
+
   // Finalize NoSuchMethodError, _Mint; occasionally needed in optimized
   // compilation.
   Class& cls = Class::Handle(
@@ -2207,21 +2155,54 @@ void BackgroundCompiler::EnsureInit(Thread* thread) {
   error = cls.EnsureIsFinalized(thread);
   ASSERT(error.IsNull());
 
-  bool start_task = false;
-  Isolate* isolate = thread->isolate();
+  MonitorLocker ml(done_monitor_);
+  if (running_ || !done_) return;
+  running_ = true;
+  done_ = false;
+  bool task_started =
+      Dart::thread_pool()->Run(new BackgroundCompilerTask(this));
+  if (!task_started) {
+    running_ = false;
+    done_ = true;
+  }
+}
+
+void BackgroundCompiler::Stop() {
+  Thread* thread = Thread::Current();
+  ASSERT(thread->IsMutatorThread());
+  ASSERT(!thread->IsAtSafepoint());
+
   {
-    MutexLocker ml(isolate->mutex());
-    if (isolate->background_compiler() == NULL) {
-      BackgroundCompiler* task = new BackgroundCompiler(isolate);
-      isolate->set_background_compiler(task);
-      start_task = true;
+    MonitorLocker ml(queue_monitor_);
+    running_ = false;
+    function_queue_->Clear();
+    ml.Notify();  // Stop waiting for the queue.
+  }
+
+  {
+    MonitorLocker ml_done(done_monitor_);
+    while (!done_) {
+      ml_done.WaitWithSafepointCheck(thread);
     }
   }
-  if (start_task) {
-    Dart::thread_pool()->Run(isolate->background_compiler());
+}
+
+void BackgroundCompiler::Enable() {
+  disabled_depth_--;
+  if (disabled_depth_ < 0) {
+    FATAL("Mismatched number of calls to BackgroundCompiler::Enable/Disable.");
   }
 }
 
+void BackgroundCompiler::Disable() {
+  Stop();
+  disabled_depth_++;
+}
+
+bool BackgroundCompiler::IsDisabled() {
+  return disabled_depth_ > 0;
+}
+
 #else  // DART_PRECOMPILED_RUNTIME
 
 bool UseKernelFrontEndFor(ParsedFunction* parsed_function) {
@@ -2329,15 +2310,11 @@ void BackgroundCompiler::VisitPointers(ObjectPointerVisitor* visitor) {
   UNREACHABLE();
 }
 
-void BackgroundCompiler::Stop(Isolate* isolate) {
+void BackgroundCompiler::Start() {
   UNREACHABLE();
 }
 
-void BackgroundCompiler::EnsureInit(Thread* thread) {
-  UNREACHABLE();
-}
-
-void BackgroundCompiler::Disable() {
+void BackgroundCompiler::Stop() {
   UNREACHABLE();
 }
 
@@ -2345,6 +2322,10 @@ void BackgroundCompiler::Enable() {
   UNREACHABLE();
 }
 
+void BackgroundCompiler::Disable() {
+  UNREACHABLE();
+}
+
 bool BackgroundCompiler::IsDisabled() {
   UNREACHABLE();
   return true;

runtime/vm/compiler/jit/compiler.h

@@ -161,21 +161,49 @@ class Compiler : public AllStatic {
 // Current implementation: one task per isolate, it dies with the owning
 // isolate.
 // No OSR compilation in the background compiler.
-class BackgroundCompiler : public ThreadPool::Task {
+class BackgroundCompiler {
  public:
+  explicit BackgroundCompiler(Isolate* isolate);
   virtual ~BackgroundCompiler();
 
-  static void EnsureInit(Thread* thread);
-
-  // Stops background compiler of the given isolate.
-  // TODO(turnidge): Give Stop and Disable more distinct names.
-  static void Stop(Isolate* isolate);
-
-  static void Disable();
-
-  static void Enable();
-
-  static bool IsDisabled();
+  static void Start(Isolate* isolate) {
+    ASSERT(Thread::Current()->IsMutatorThread());
+    if (isolate->background_compiler() != NULL) {
+      isolate->background_compiler()->Start();
+    }
+  }
+  static void Stop(Isolate* isolate) {
+    ASSERT(Thread::Current()->IsMutatorThread());
+    if (isolate->background_compiler() != NULL) {
+      isolate->background_compiler()->Stop();
+    }
+  }
+  static void Enable(Isolate* isolate) {
+    ASSERT(Thread::Current()->IsMutatorThread());
+    if (isolate->background_compiler() != NULL) {
+      isolate->background_compiler()->Enable();
+    }
+  }
+  static void Disable(Isolate* isolate) {
+    ASSERT(Thread::Current()->IsMutatorThread());
+    if (isolate->background_compiler() != NULL) {
+      isolate->background_compiler()->Disable();
+    }
+  }
+  static bool IsDisabled(Isolate* isolate) {
+    ASSERT(Thread::Current()->IsMutatorThread());
+    if (isolate->background_compiler() != NULL) {
+      return isolate->background_compiler()->IsDisabled();
+    }
+    return false;
+  }
+  static bool IsRunning(Isolate* isolate) {
+    ASSERT(Thread::Current()->IsMutatorThread());
+    if (isolate->background_compiler() != NULL) {
+      return isolate->background_compiler()->IsRunning();
+    }
+    return false;
+  }
 
   // Call to optimize a function in the background, enters the function in the
   // compilation queue.
@@ -186,19 +214,27 @@ class BackgroundCompiler : public ThreadPool::Task {
   BackgroundCompilationQueue* function_queue() const { return function_queue_; }
   bool is_running() const { return running_; }
 
- private:
-  explicit BackgroundCompiler(Isolate* isolate);
+  void Run();
 
-  virtual void Run();
+ private:
+  void Start();
+  void Stop();
+  void Enable();
+  void Disable();
+  bool IsDisabled();
+  bool IsRunning() { return !done_; }
 
   Isolate* isolate_;
-  bool running_;            // While true, will try to read queue and compile.
-  bool* done_;              // True if the thread is done.
-  Monitor* queue_monitor_;  // Controls access to the queue.
-  Monitor* done_monitor_;   // Notify/wait that the thread is done.
 
+  Monitor* queue_monitor_;  // Controls access to the queue.
   BackgroundCompilationQueue* function_queue_;
 
+  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.
+
+  int16_t disabled_depth_;
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(BackgroundCompiler);
 };
 

runtime/vm/compiler_test.cc

@@ -95,9 +95,8 @@ ISOLATE_UNIT_TEST_CASE(CompileFunctionOnHelperThread) {
   // Constant in product mode.
   FLAG_background_compilation = true;
 #endif
-  BackgroundCompiler::EnsureInit(thread);
   Isolate* isolate = thread->isolate();
-  ASSERT(isolate->background_compiler() != NULL);
+  BackgroundCompiler::Start(isolate);
   isolate->background_compiler()->CompileOptimized(func);
   Monitor* m = new Monitor();
   {

runtime/vm/dart_api_impl.cc

@@ -1490,7 +1490,7 @@ Dart_CreateSnapshot(uint8_t** vm_snapshot_data_buffer,
   if (::Dart_IsError(state)) {
     return state;
   }
-  I->StopBackgroundCompiler();
+  BackgroundCompiler::Stop(I);
 
 #if defined(DEBUG)
   I->heap()->CollectAllGarbage();
@@ -6671,7 +6671,7 @@ DART_EXPORT Dart_Handle Dart_CreateCoreJITSnapshotAsBlobs(
   if (::Dart_IsError(state)) {
     return state;
   }
-  I->StopBackgroundCompiler();
+  BackgroundCompiler::Stop(I);
 
   ProgramVisitor::Dedup();
   Symbols::Compact(I);
@@ -6726,7 +6726,7 @@ Dart_CreateAppJITSnapshotAsBlobs(uint8_t** isolate_snapshot_data_buffer,
   if (::Dart_IsError(state)) {
     return state;
   }
-  I->StopBackgroundCompiler();
+  BackgroundCompiler::Stop(I);
 
   ProgramVisitor::Dedup();
   Symbols::Compact(I);

runtime/vm/isolate.cc

@@ -234,7 +234,7 @@ void Isolate::ValidateConstants() {
   }
   // Verify that all canonical instances are correctly setup in the
   // corresponding canonical tables.
-  StopBackgroundCompiler();
+  BackgroundCompiler::Stop(this);
   heap()->CollectAllGarbage();
   Thread* thread = Thread::Current();
   HeapIterationScope iteration(thread);
@@ -880,7 +880,6 @@ Isolate::Isolate(const Dart_IsolateFlags& api_flags)
       class_table_(),
       single_step_(false),
       isolate_flags_(0),
-      background_compiler_disabled_depth_(0),
       background_compiler_(NULL),
 #if !defined(PRODUCT)
       debugger_(NULL),
@@ -928,7 +927,7 @@ Isolate::Isolate(const Dart_IsolateFlags& api_flags)
       message_handler_(NULL),
       spawn_state_(NULL),
       defer_finalization_count_(0),
-      pending_deopts_(new MallocGrowableArray<PendingLazyDeopt>),
+      pending_deopts_(new MallocGrowableArray<PendingLazyDeopt>()),
       deopt_context_(NULL),
       tag_table_(GrowableObjectArray::null()),
       deoptimized_code_array_(GrowableObjectArray::null()),
@@ -959,14 +958,20 @@ Isolate::Isolate(const Dart_IsolateFlags& api_flags)
         "which violates the Dart standard.\n"
         "         See dartbug.com/30524 for more information.\n");
   }
+
+  NOT_IN_PRECOMPILED(background_compiler_ = new BackgroundCompiler(this));
 }
 
 #undef REUSABLE_HANDLE_SCOPE_INIT
 #undef REUSABLE_HANDLE_INITIALIZERS
 
 Isolate::~Isolate() {
+  delete background_compiler_;
+  background_compiler_ = NULL;
+
 #if !defined(PRODUCT)
   delete debugger_;
+  debugger_ = NULL;
   if (FLAG_support_service) {
     delete object_id_ring_;
   }
@@ -1766,13 +1771,6 @@ void Isolate::LowLevelShutdown() {
 #endif  // !defined(PRODUCT)
 }
 
-void Isolate::StopBackgroundCompiler() {
-  // Wait until all background compilation has finished.
-  if (background_compiler_ != NULL) {
-    BackgroundCompiler::Stop(this);
-  }
-}
-
 #if !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME)
 void Isolate::MaybeIncreaseReloadEveryNStackOverflowChecks() {
   if (FLAG_reload_every_back_off) {
@@ -1791,7 +1789,9 @@ void Isolate::MaybeIncreaseReloadEveryNStackOverflowChecks() {
 
 void Isolate::Shutdown() {
   ASSERT(this == Isolate::Current());
-  StopBackgroundCompiler();
+  BackgroundCompiler::Stop(this);
+  delete background_compiler_;
+  background_compiler_ = NULL;
 
 #if defined(DEBUG)
   if (heap_ != NULL && FLAG_verify_on_transition) {

runtime/vm/isolate.h

@@ -450,26 +450,6 @@ class Isolate : public BaseIsolate {
   BackgroundCompiler* background_compiler() const {
     return background_compiler_;
   }
-  void set_background_compiler(BackgroundCompiler* value) {
-    // Do not overwrite a background compiler (memory leak).
-    ASSERT((value == NULL) || (background_compiler_ == NULL));
-    background_compiler_ = value;
-  }
-
-  void enable_background_compiler() {
-    background_compiler_disabled_depth_--;
-    if (background_compiler_disabled_depth_ < 0) {
-      FATAL(
-          "Mismatched number of calls to disable_background_compiler and "
-          "enable_background_compiler.");
-    }
-  }
-
-  void disable_background_compiler() { background_compiler_disabled_depth_++; }
-
-  bool is_background_compiler_disabled() const {
-    return background_compiler_disabled_depth_ > 0;
-  }
 
 #if !defined(PRODUCT)
   void UpdateLastAllocationProfileAccumulatorResetTimestamp() {
@@ -754,8 +734,6 @@ class Isolate : public BaseIsolate {
   static bool IsolateCreationEnabled();
   static bool IsVMInternalIsolate(Isolate* isolate);
 
-  void StopBackgroundCompiler();
-
 #if !defined(PRODUCT)
   intptr_t reload_every_n_stack_overflow_checks() const {
     return reload_every_n_stack_overflow_checks_;
@@ -877,7 +855,6 @@ class Isolate : public BaseIsolate {
   uint32_t isolate_flags_;
 
   // Background compilation.
-  int16_t background_compiler_disabled_depth_;
   BackgroundCompiler* background_compiler_;
 
 // Fields that aren't needed in a product build go here with boolean flags at

runtime/vm/isolate_reload.cc

@@ -624,7 +624,7 @@ void IsolateReloadContext::Reload(bool force_reload,
   become_enum_mappings_ = GrowableObjectArray::New(Heap::kOld);
 
   // Disable the background compiler while we are performing the reload.
-  BackgroundCompiler::Disable();
+  BackgroundCompiler::Disable(I);
 
   // Ensure all functions on the stack have unoptimized code.
   EnsuredUnoptimizedCodeForStack();
@@ -682,7 +682,7 @@ void IsolateReloadContext::Reload(bool force_reload,
   // WEIRD CONTROL FLOW ENDS.
 
   // Re-enable the background compiler. Do this before propagating any errors.
-  BackgroundCompiler::Enable();
+  BackgroundCompiler::Enable(I);
 
   if (result.IsUnwindError()) {
     if (thread->top_exit_frame_info() == 0) {

runtime/vm/runtime_entry.cc

@@ -1811,7 +1811,7 @@ DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
       if (FLAG_enable_inlining_annotations) {
         FATAL("Cannot enable inlining annotations and background compilation");
       }
-      if (!BackgroundCompiler::IsDisabled()) {
+      if (!BackgroundCompiler::IsDisabled(isolate)) {
         if (FLAG_background_compilation_stop_alot) {
           BackgroundCompiler::Stop(isolate);
         }
@@ -1820,8 +1820,7 @@ DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
         // takes long time to trigger optimization.
         // Note that the background compilation queue rejects duplicate entries.
         function.SetUsageCounter(INT_MIN);
-        BackgroundCompiler::EnsureInit(thread);
-        ASSERT(isolate->background_compiler() != NULL);
+        BackgroundCompiler::Start(isolate);
         isolate->background_compiler()->CompileOptimized(function);
         // Continue in the same code.
         arguments.SetReturn(function);

runtime/vm/symbols.cc

@@ -308,7 +308,6 @@ RawArray* Symbols::UnifiedSymbolTable() {
   Zone* zone = thread->zone();
 
   ASSERT(thread->IsMutatorThread());
-  ASSERT(isolate->background_compiler() == NULL);
 
   SymbolTable vm_table(zone,
                        Dart::vm_isolate()->object_store()->symbol_table());