Use background tasks for the compiler dispatcher

src/compiler-dispatcher/compiler-dispatcher.cc

Index: src/compiler-dispatcher/compiler-dispatcher.cc
diff --git a/src/compiler-dispatcher/compiler-dispatcher.cc b/src/compiler-dispatcher/compiler-dispatcher.cc
index 5cf10aa3eb8ac012143c4f4b486d0e69c3bcfc1c..0121ae4606fb197a66d54a307aded6ae604e1286 100644
--- a/src/compiler-dispatcher/compiler-dispatcher.cc
+++ b/src/compiler-dispatcher/compiler-dispatcher.cc
@@ -66,15 +66,65 @@ bool IsFinished(CompilerDispatcherJob* job) {
          job->status() == CompileJobStatus::kFailed;
 }
 
+bool CanRunOnAnyThread(CompilerDispatcherJob* job) {
+  return (job->status() == CompileJobStatus::kReadyToParse &&
+          job->can_parse_on_background_thread()) ||
+         (job->status() == CompileJobStatus::kReadyToCompile &&
+          job->can_compile_on_background_thread());
+}
+
+void DoNextStepOnBackgroundThread(CompilerDispatcherJob* job) {
+  DCHECK(CanRunOnAnyThread(job));
+  switch (job->status()) {
+    case CompileJobStatus::kReadyToParse:
+      job->Parse();
+      break;
+
+    case CompileJobStatus::kReadyToCompile:
+      job->Compile();
+      break;
+
+    default:
+      UNREACHABLE();
+  }
+}
+
 // Theoretically we get 50ms of idle time max, however it's unlikely that
 // we'll get all of it so try to be a conservative.
 const double kMaxIdleTimeToExpectInMs = 40;
 
 }  // namespace
 
+class CompilerDispatcher::BackgroundTask : public CancelableTask {
+ public:
+  BackgroundTask(Isolate* isolate, CancelableTaskManager* task_manager,
+                 CompilerDispatcher* dispatcher);

[vogelheim, 2017/01/03 13:23:02]

[Here, and CompilerDispatcher::IdleTask below]:

Since the BackgroundTask (& IdleTask) always know the CompilerDispatcher*, you
could give it an accessor CompilerDispatcher::cancelable_task_manager() and use
that one, rather than requiring the client to pass it in. That way, you can't
mismatch the task and the appropriate task manager.

[jochen (gone - plz use gerrit), 2017/01/03 14:01:45]

that's true, but I like dependency injection 

+  ~BackgroundTask() override;
+
+  // CancelableTask implementation.
+  void RunInternal() override;
+
+ private:
+  CompilerDispatcher* dispatcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(BackgroundTask);
+};
+
+CompilerDispatcher::BackgroundTask::BackgroundTask(
+    Isolate* isolate, CancelableTaskManager* task_manager,
+    CompilerDispatcher* dispatcher)
+    : CancelableTask(isolate, task_manager), dispatcher_(dispatcher) {}
+
+CompilerDispatcher::BackgroundTask::~BackgroundTask() {}
+
+void CompilerDispatcher::BackgroundTask::RunInternal() {
+  dispatcher_->DoBackgroundWork();
+}
+
 class CompilerDispatcher::IdleTask : public CancelableIdleTask {
  public:
-  IdleTask(Isolate* isolate, CompilerDispatcher* dispatcher);
+  IdleTask(Isolate* isolate, CancelableTaskManager* task_manager,
+           CompilerDispatcher* dispatcher);
   ~IdleTask() override;
 
   // CancelableIdleTask implementation.
@@ -87,8 +137,9 @@ class CompilerDispatcher::IdleTask : public CancelableIdleTask {
 };
 
 CompilerDispatcher::IdleTask::IdleTask(Isolate* isolate,
+                                       CancelableTaskManager* task_manager,
                                        CompilerDispatcher* dispatcher)
-    : CancelableIdleTask(isolate), dispatcher_(dispatcher) {}
+    : CancelableIdleTask(isolate, task_manager), dispatcher_(dispatcher) {}
 
 CompilerDispatcher::IdleTask::~IdleTask() {}
 
@@ -102,11 +153,15 @@ CompilerDispatcher::CompilerDispatcher(Isolate* isolate, Platform* platform,
       platform_(platform),
       max_stack_size_(max_stack_size),
       tracer_(new CompilerDispatcherTracer(isolate_)),
-      idle_task_scheduled_(false) {}
+      task_manager_(new CancelableTaskManager()),
+      idle_task_scheduled_(false),
+      num_scheduled_background_tasks_(0),
+      main_thread_blocking_on_job_(nullptr) {}
 
 CompilerDispatcher::~CompilerDispatcher() {
   // To avoid crashing in unit tests due to unfished jobs.
   AbortAll(BlockingBehavior::kBlock);
+  task_manager_->CancelAndWait();
 }
 
 bool CompilerDispatcher::Enqueue(Handle<SharedFunctionInfo> function) {
@@ -138,12 +193,27 @@ bool CompilerDispatcher::IsEnqueued(Handle<SharedFunctionInfo> function) const {
   return GetJobFor(function) != jobs_.end();
 }
 
+void CompilerDispatcher::WaitForJobIfRunningOnBackground(
+    CompilerDispatcherJob* job) {
+  base::LockGuard<base::Mutex> lock(&mutex_);
+  if (running_background_jobs_.find(job) == running_background_jobs_.end()) {
+    pending_background_jobs_.erase(job);
+    return;
+  }
+  DCHECK_NULL(main_thread_blocking_on_job_);
+  main_thread_blocking_on_job_ = job;
+  while (main_thread_blocking_on_job_ != nullptr) {
+    main_thread_blocking_signal_.Wait(&mutex_);
+  }
+  DCHECK(pending_background_jobs_.find(job) == pending_background_jobs_.end());
+  DCHECK(running_background_jobs_.find(job) == running_background_jobs_.end());
+}
+
 bool CompilerDispatcher::FinishNow(Handle<SharedFunctionInfo> function) {
   JobMap::const_iterator job = GetJobFor(function);
   CHECK(job != jobs_.end());
 
-  // TODO(jochen): Check if there's an in-flight background task working on this
-  // job.
+  WaitForJobIfRunningOnBackground(job->second.get());
   while (!IsFinished(job->second.get())) {
     DoNextStepOnMainThread(isolate_, job->second.get(),
                            ExceptionHandling::kThrow);
@@ -154,23 +224,11 @@ bool CompilerDispatcher::FinishNow(Handle<SharedFunctionInfo> function) {
   return result;
 }
 
-void CompilerDispatcher::Abort(Handle<SharedFunctionInfo> function,
-                               BlockingBehavior blocking) {
-  USE(blocking);
-  JobMap::const_iterator job = GetJobFor(function);
-  CHECK(job != jobs_.end());
-
-  // TODO(jochen): Check if there's an in-flight background task working on this
-  // job.
-  job->second->ResetOnMainThread();
-  jobs_.erase(job);
-}
-
 void CompilerDispatcher::AbortAll(BlockingBehavior blocking) {
-  USE(blocking);
-  // TODO(jochen): Check if there's an in-flight background task working on this
-  // job.
+  // TODO(jochen): Implement support for non-blocking abort.
+  DCHECK(blocking == BlockingBehavior::kBlock);
   for (auto& kv : jobs_) {
+    WaitForJobIfRunningOnBackground(kv.second.get());

[vogelheim, 2017/01/03 13:23:02]

If we want to abort the task (as in, we don't care whether they finish), and if
we have cancel-able tasks, wouldn't we try to cancel those, instead of waiting
for them?

[jochen (gone - plz use gerrit), 2017/01/03 14:01:44]

we still need to reset the job on the main thread, so the background task needs
to schedule this once it's done

     kv.second->ResetOnMainThread();
   }
   jobs_.clear();
@@ -188,18 +246,87 @@ CompilerDispatcher::JobMap::const_iterator CompilerDispatcher::GetJobFor(
   return jobs_.end();
 }
 
-void CompilerDispatcher::ScheduleIdleTaskIfNeeded() {
+void CompilerDispatcher::ScheduleIdleTaskFromAnyThread() {
   v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate_);
   DCHECK(platform_->IdleTasksEnabled(v8_isolate));
-  if (idle_task_scheduled_) return;
+  {
+    base::LockGuard<base::Mutex> lock(&mutex_);
+    if (idle_task_scheduled_) return;
+    idle_task_scheduled_ = true;
+  }
+  platform_->CallIdleOnForegroundThread(
+      v8_isolate, new IdleTask(isolate_, task_manager_.get(), this));
+}
+
+void CompilerDispatcher::ScheduleIdleTaskIfNeeded() {
   if (jobs_.empty()) return;
-  idle_task_scheduled_ = true;
-  platform_->CallIdleOnForegroundThread(v8_isolate,
-                                        new IdleTask(isolate_, this));
+  ScheduleIdleTaskFromAnyThread();
+}
+
+void CompilerDispatcher::ConsiderJobForBackgroundProcessing(
+    CompilerDispatcherJob* job) {
+  if (!CanRunOnAnyThread(job)) return;
+  {
+    base::LockGuard<base::Mutex> lock(&mutex_);
+    pending_background_jobs_.insert(job);
+  }
+  ScheduleMoreBackgroundTasksIfNeeded();
+}
+
+void CompilerDispatcher::ScheduleMoreBackgroundTasksIfNeeded() {
+  if (FLAG_single_threaded) return;
+  {
+    base::LockGuard<base::Mutex> lock(&mutex_);
+    if (pending_background_jobs_.empty()) return;
+    if (platform_->NumberOfAvailableBackgroundThreads() <=
+        num_scheduled_background_tasks_) {
+      return;
+    }
+    ++num_scheduled_background_tasks_;
+  }
+  platform_->CallOnBackgroundThread(
+      new BackgroundTask(isolate_, task_manager_.get(), this),
+      v8::Platform::kShortRunningTask);
+}
+
+void CompilerDispatcher::DoBackgroundWork() {
+  CompilerDispatcherJob* job = nullptr;
+  {
+    base::LockGuard<base::Mutex> lock(&mutex_);
+    --num_scheduled_background_tasks_;
+    if (!pending_background_jobs_.empty()) {
+      auto it = pending_background_jobs_.begin();
+      job = *it;
+      pending_background_jobs_.erase(it);
+      running_background_jobs_.insert(job);
+    }
+  }
+  if (job == nullptr) return;
+  DoNextStepOnBackgroundThread(job);
+
+  ScheduleMoreBackgroundTasksIfNeeded();
+  // Unconditionally schedule an idle task, as all background steps have to be
+  // followed by a main thread step.
+  ScheduleIdleTaskFromAnyThread();
+
+  {
+    base::LockGuard<base::Mutex> lock(&mutex_);
+    running_background_jobs_.erase(job);
+
+    if (main_thread_blocking_on_job_ == job) {
+      main_thread_blocking_on_job_ = nullptr;
+      main_thread_blocking_signal_.NotifyOne();
+    }
+  }
+  // Don't touch |this| anymore after this point, as it might have been
+  // deleted.

[vogelheim, 2017/01/03 13:23:02]

I'm confused. (Might be me...) How can it be that *this is deleted here, but
wasn't in line #318, where we merrily access *this?

[jochen (gone - plz use gerrit), 2017/01/03 14:01:45]

NotifyOne() might unblock the main thread which then proceeds to run the dtor

 }
 
 void CompilerDispatcher::DoIdleWork(double deadline_in_seconds) {
-  idle_task_scheduled_ = false;
+  {
+    base::LockGuard<base::Mutex> lock(&mutex_);
+    idle_task_scheduled_ = false;
+  }
 
   // Number of jobs that are unlikely to make progress during any idle callback
   // due to their estimated duration.
@@ -214,6 +341,21 @@ void CompilerDispatcher::DoIdleWork(double deadline_in_seconds) {
        job != jobs_.end() && idle_time_in_seconds > 0.0;
        idle_time_in_seconds =
            deadline_in_seconds - platform_->MonotonicallyIncreasingTime()) {
+    {
+      // Don't work on jobs that are being worked on by background tasks.
+      // Similarly, remove jobs we work on from the set of available background
+      // jobs.
+      base::LockGuard<base::Mutex> lock(&mutex_);
+      if (running_background_jobs_.find(job->second.get()) !=
+          running_background_jobs_.end()) {
+        ++job;
+        continue;
+      }
+      auto it = pending_background_jobs_.find(job->second.get());
+      if (it != pending_background_jobs_.end()) {
+        pending_background_jobs_.erase(it);
+      }
+    }
     double estimate_in_ms = job->second->EstimateRuntimeOfNextStepInMs();
     if (idle_time_in_seconds <
         (estimate_in_ms /
@@ -222,6 +364,7 @@ void CompilerDispatcher::DoIdleWork(double deadline_in_seconds) {
       // have managed to finish the job in a large idle task to assess
       // whether we should ask for another idle callback.
       if (estimate_in_ms > kMaxIdleTimeToExpectInMs) ++too_long_jobs;
+      ConsiderJobForBackgroundProcessing(job->second.get());
       ++job;
     } else if (IsFinished(job->second.get())) {
       job->second->ResetOnMainThread();