Use idle time to make progress on scheduled compilation jobs

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 8d503780cd5511b7af36cbbf865e6037277a5282..64c3723bcf2f9ea6e91de9df40eed457a191394e 100644
--- a/src/compiler-dispatcher/compiler-dispatcher.cc
+++ b/src/compiler-dispatcher/compiler-dispatcher.cc
@@ -4,6 +4,10 @@
 
 #include "src/compiler-dispatcher/compiler-dispatcher.h"
 
+#include "include/v8-platform.h"
+#include "include/v8.h"
+#include "src/base/platform/time.h"
+#include "src/cancelable-task.h"
 #include "src/compiler-dispatcher/compiler-dispatcher-job.h"
 #include "src/compiler-dispatcher/compiler-dispatcher-tracer.h"
 #include "src/objects-inl.h"
@@ -13,7 +17,12 @@ namespace internal {
 
 namespace {
 
-bool DoNextStepOnMainThread(CompilerDispatcherJob* job) {
+enum class ExceptionHandling { kSwallow, kKeep };
+
+bool DoNextStepOnMainThread(Isolate* isolate, CompilerDispatcherJob* job,
+                            ExceptionHandling exception_handling) {
+  DCHECK(ThreadId::Current().Equals(isolate->thread_id()));
+  v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
   switch (job->status()) {
     case CompileJobStatus::kInitial:
       job->PrepareToParseOnMainThread();
@@ -44,6 +53,10 @@ bool DoNextStepOnMainThread(CompilerDispatcherJob* job) {
       break;
   }
 
+  if (exception_handling == ExceptionHandling::kKeep && try_catch.HasCaught()) {
+    DCHECK(job->status() == CompileJobStatus::kFailed);

[vogelheim, 2016/12/15 15:24:12]

nitpick: DCHECK_EQ

[jochen (gone - plz use gerrit), 2016/12/15 15:37:59]

that would require implementation operator<< for the enum class :-/

+    try_catch.ReThrow();
+  }
   return job->status() != CompileJobStatus::kFailed;
 }
 
@@ -52,14 +65,48 @@ bool IsFinished(CompilerDispatcherJob* job) {
          job->status() == CompileJobStatus::kFailed;
 }
 
+// Theoretically, we get 50ms of idle time max, however, it's unlikely that

[vogelheim, 2016/12/15 15:24:11]

super nitpick: Remove some of the commas, like:
  Theoretically we get 50ms of idle time max, however it's unlikely [...]

[jochen (gone - plz use gerrit), 2016/12/15 15:37:59]

done

+// we'll get all of it, so try to be a conservative.
+const double kMaxIdleTimeToExpectInMs = 40;
+
 }  // namespace
 
-CompilerDispatcher::CompilerDispatcher(Isolate* isolate, size_t max_stack_size)
+class CompilerDispatcher::IdleTask : public CancelableIdleTask {
+ public:
+  IdleTask(Isolate* isolate, CompilerDispatcher* dispatcher);
+  ~IdleTask() override;
+
+  // CancelableIdleTask implementation.
+  void RunInternal(double deadline_in_seconds) override;
+
+ private:
+  CompilerDispatcher* dispatcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(IdleTask);
+};
+
+CompilerDispatcher::IdleTask::IdleTask(Isolate* isolate,
+                                       CompilerDispatcher* dispatcher)
+    : CancelableIdleTask(isolate), dispatcher_(dispatcher) {}
+
+CompilerDispatcher::IdleTask::~IdleTask() {}
+
+void CompilerDispatcher::IdleTask::RunInternal(double deadline_in_seconds) {
+  dispatcher_->DoIdleWork(deadline_in_seconds);
+}
+
+CompilerDispatcher::CompilerDispatcher(Isolate* isolate, Platform* platform,
+                                       size_t max_stack_size)
     : isolate_(isolate),
+      platform_(platform),
       max_stack_size_(max_stack_size),
-      tracer_(new CompilerDispatcherTracer(isolate_)) {}
+      tracer_(new CompilerDispatcherTracer(isolate_)),
+      idle_task_scheduled_(false) {}
 
-CompilerDispatcher::~CompilerDispatcher() {}
+CompilerDispatcher::~CompilerDispatcher() {
+  // To avoid crashing in unit tests due to unfished jobs.
+  AbortAll(BlockingBehavior::kBlock);
+}
 
 bool CompilerDispatcher::Enqueue(Handle<SharedFunctionInfo> function) {
   // We only handle functions (no eval / top-level code / wasm) that are
@@ -74,7 +121,9 @@ bool CompilerDispatcher::Enqueue(Handle<SharedFunctionInfo> function) {
       isolate_, tracer_.get(), function, max_stack_size_));
   std::pair<int, int> key(Script::cast(function->script())->id(),
                           function->function_literal_id());
-  jobs_.insert(std::make_pair(key, std::move(job)));
+  JobMap::const_iterator it = jobs_.insert(std::make_pair(key, std::move(job)));
+  ClassifyJob(it);
+  ScheduleIdleTaskIfNeeded();
   return true;
 }
 
@@ -89,9 +138,13 @@ bool CompilerDispatcher::FinishNow(Handle<SharedFunctionInfo> function) {
   // TODO(jochen): Check if there's an in-flight background task working on this
   // job.
   while (!IsFinished(job->second.get())) {
-    DoNextStepOnMainThread(job->second.get());
+    DoNextStepOnMainThread(isolate_, job->second.get(),
+                           ExceptionHandling::kKeep);
   }
   bool result = job->second->status() != CompileJobStatus::kFailed;
+  job->second->ResetOnMainThread();
+  jobs_for_background_thread_.erase(job);
+  jobs_for_main_thread_.erase(job);
   jobs_.erase(job);
   return result;
 }
@@ -104,6 +157,9 @@ void CompilerDispatcher::Abort(Handle<SharedFunctionInfo> function,
 
   // TODO(jochen): Check if there's an in-flight background task working on this
   // job.
+  job->second->ResetOnMainThread();
+  jobs_for_background_thread_.erase(job);
+  jobs_for_main_thread_.erase(job);
   jobs_.erase(job);
 }
 
@@ -111,6 +167,11 @@ void CompilerDispatcher::AbortAll(BlockingBehavior blocking) {
   USE(blocking);
   // TODO(jochen): Check if there's an in-flight background task working on this
   // job.
+  for (auto& kv : jobs_) {
+    kv.second->ResetOnMainThread();
+  }
+  jobs_for_background_thread_.clear();
+  jobs_for_main_thread_.clear();
   jobs_.clear();
 }
 
@@ -126,5 +187,89 @@ CompilerDispatcher::JobMap::const_iterator CompilerDispatcher::GetJobFor(
   return jobs_.end();
 }
 
+void CompilerDispatcher::ScheduleIdleTaskIfNeeded() {
+  v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate_);
+  if (!platform_->IdleTasksEnabled(v8_isolate)) return;
+  if (idle_task_scheduled_) return;
+  if (jobs_.empty()) return;
+  idle_task_scheduled_ = true;
+  platform_->CallIdleOnForegroundThread(v8_isolate,
+                                        new IdleTask(isolate_, this));
+}
+
+void CompilerDispatcher::DoIdleWork(double deadline_in_seconds) {
+  idle_task_scheduled_ = false;
+
+  DoIdleWorkForJobSet(deadline_in_seconds, &jobs_for_main_thread_);
+  if (deadline_in_seconds <= platform_->MonotonicallyIncreasingTime()) return;
+  DoIdleWorkForJobSet(deadline_in_seconds, &jobs_for_background_thread_);
+}
+
+void CompilerDispatcher::DoIdleWorkForJobSet(

[marja, 2016/12/15 15:56:43]

Can you add some high-level comments to CompilerDispatcher about what it does
and how? E.g., that it goes through the job set in an undefined order and how
all the different job queues and sets interact with each other.

+    double deadline_in_seconds,
+    std::unordered_set<JobMap::const_iterator, JobIteratorHash>* job_set) {
+  bool work_left = false;
+
+  for (auto it = job_set->begin(); it != job_set->end();) {
+    double idle_time_in_seconds =
+        deadline_in_seconds - platform_->MonotonicallyIncreasingTime();
+    if (idle_time_in_seconds <= 0.0) {
+      work_left = true;
+      break;
+    }
+
+    auto job = (*it);
+    // We'll put the job back on the right queue if we don't manage to process
+    // it completely.
+    it = jobs_for_main_thread_.erase(it);

[marja, 2016/12/15 15:56:43]

How is this supposed to work? it is an iterator for job_set and now you
jobs_for_main_thread_.erase with it, and moreover, save the returned iterator
back. That shouldn't do anything meaningful afaics. Why doesn't any of your
tests fail? :)

(Hmm, job_set can be the same as jobs_for_main_thread_ but it can also be
jobs_for_backround_thread_.)

[jochen (gone - plz use gerrit), 2016/12/15 20:18:44]

erase() in C++11 returns the next iterator, it's short for

auto old = it++;
set.erase(old);

+
+    // Advance the current job as much as possible.
+    for (;;) {
+      double estimate_in_ms = job->second->EstimateRuntimeOfNextStepInMs();
+      if (idle_time_in_seconds <
+          (estimate_in_ms /
+           static_cast<double>(base::Time::kMillisecondsPerSecond))) {
+        // If there's not enough time left, try to estimate whether we would
+        // 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) work_left = true;
+        // As there's remaining work, put the job back on the queue.
+        ClassifyJob(job);

[marja, 2016/12/15 15:56:43]

As job_set is a pointer to one of the underlying unordered_sets, and we modify
them in ClassifyJob, it's slightly non-trivial to see that the modifications are
OK and don't invalidate any iterators. I think they are now (I googled), it's
just slightly error prone... if somebody changes the underlying data structure.
Can you add a comment to where jobs_on_main_thread_ and so are declared about
that? (That they need to be data structures where modifications don't invalidate
iterators.)

[marja, 2016/12/15 15:56:43]

The interaction between the queues and the job set is unclear after just
skimming through the code. Does the job end up in the job set in multiple times
even though its next step is too big to complete during idle time?

Hmm, I read the code more, and looks indeed like it. Why does the job need to be
in the sets if the only way for it to get processed is that the function it
represents needs to be executed?

Hmm2, looks like it's possible to have big functions that are too big to process
as idle tasks but never called, and we'll just happily keep the tasks (and
whatever memory they hang on to) alive. Is that intentional?

[jochen (gone - plz use gerrit), 2016/12/15 20:18:44]

once we also have a background scheduler, they'll make progress there

+        break;
+      }
+      DoNextStepOnMainThread(isolate_, job->second.get(),
+                             ExceptionHandling::kSwallow);
+
+      if (IsFinished(job->second.get())) {
+        job->second->ResetOnMainThread();
+        jobs_.erase(job);

[marja, 2016/12/15 15:56:43]

There's jobs_ and job_set, could you name them somehow in a more illustrative
manner?

+        break;
+      }
+
+      idle_time_in_seconds =
+          deadline_in_seconds - platform_->MonotonicallyIncreasingTime();
+    }
+  }
+  if (work_left) ScheduleIdleTaskIfNeeded();

[vogelheim, 2016/12/15 15:24:12]

This would potentially be called twice, since DoIdleWorkForJobSet is called for
two different job sets. Maybe pull the check into DoIdleWork?

[vogelheim, 2016/12/15 15:24:12]

At this point, wouldn't (work_left == !job_set.empty()) ?

If so, you don't need the variable at all. If not... then I didn't get it. :)

[jochen (gone - plz use gerrit), 2016/12/15 15:37:59]

I found it more readable to put it here, instead of having a magic bool return
value
job_set could be full of jobs that are estimated to take more than the max
available idle time

+}
+
+void CompilerDispatcher::ClassifyJob(JobMap::const_iterator it) {

[marja, 2016/12/15 15:56:43]

This function name is misleading, it doesn't just classify the job but it also
puts it into the right queue.

+  bool can_go_on_background_thread = false;
+  CompilerDispatcherJob* job = it->second.get();
+  if (job->status() == CompileJobStatus::kReadyToParse &&
+      job->can_parse_on_background_thread()) {
+    can_go_on_background_thread = true;
+  } else if (job->status() == CompileJobStatus::kReadyToCompile &&
+             job->can_compile_on_background_thread()) {
+    can_go_on_background_thread = true;
+  }

[vogelheim, 2016/12/15 15:24:11]

style nitpick; feel free to ignore...  The if-else effectively implements an
or-operator, so the following might be more readable:

bool can_go_on_background_thread = 
  (job->status() == CompileJobStatus::kReadyToParse && 
   job->can_parse_on_background_thread()) ||
  (job->status() == CompileJobStatus::kReadyToCompile &&
   job->can_compile_on_background_thread());

[jochen (gone - plz use gerrit), 2016/12/15 15:37:59]

done

+
+  if (can_go_on_background_thread) {
+    jobs_for_background_thread_.insert(it);
+  } else {
+    jobs_for_main_thread_.insert(it);
+  }
+}
+
 }  // namespace internal
 }  // namespace v8