Use background tasks for the compiler dispatcher

test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc

Index: test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc
diff --git a/test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc b/test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc
index ae176408fdf47e02106ed5bea7f95e778b8c0734..b06dad93233875a14eeef82683784ea39efd393a 100644
--- a/test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc
+++ b/test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc
@@ -5,10 +5,13 @@
 #include "src/compiler-dispatcher/compiler-dispatcher.h"
 
 #include "include/v8-platform.h"
+#include "src/base/platform/semaphore.h"
 #include "src/compiler-dispatcher/compiler-dispatcher-job.h"
+#include "src/compiler-dispatcher/compiler-dispatcher-tracer.h"
 #include "src/flags.h"
 #include "src/handles.h"
 #include "src/objects-inl.h"
+#include "src/v8.h"
 #include "test/unittests/compiler-dispatcher/compiler-dispatcher-helper.h"
 #include "test/unittests/test-utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -40,16 +43,44 @@ class CompilerDispatcherTest : public TestWithContext {
 
 bool CompilerDispatcherTest::old_flag_;
 
+class IgnitionCompilerDispatcherTest : public CompilerDispatcherTest {
+ public:
+  IgnitionCompilerDispatcherTest() = default;
+  ~IgnitionCompilerDispatcherTest() override = default;
+
+  static void SetUpTestCase() {
+    old_flag_ = i::FLAG_ignition;
+    i::FLAG_ignition = true;

[vogelheim, 2017/01/03 10:32:37]

Is this necessary?

unittests/ uses gtest, which has a flag saver mechanism built in. Would this not
cover this flag?

[jochen (gone - plz use gerrit), 2017/01/03 12:59:36]

I never heard of that? how does it work?

I need to enable --ignition, otherwise compile jobs might end up using FCG which
doesn't support background compilation

+    CompilerDispatcherTest::SetUpTestCase();
+  }
+
+  static void TearDownTestCase() {
+    CompilerDispatcherTest::TearDownTestCase();
+    i::FLAG_ignition = old_flag_;
+  }
+
+ private:
+  static bool old_flag_;
+  DISALLOW_COPY_AND_ASSIGN(IgnitionCompilerDispatcherTest);
+};
+
+bool IgnitionCompilerDispatcherTest::old_flag_;
+
 namespace {
 
 class MockPlatform : public v8::Platform {
  public:
-  MockPlatform() : task_(nullptr), time_(0.0), time_step_(0.0) {}
-  ~MockPlatform() override = default;
+  MockPlatform() : idle_task_(nullptr), time_(0.0), time_step_(0.0), sem_(0) {}
+  ~MockPlatform() override {
+    EXPECT_TRUE(tasks_.empty());
+    EXPECT_TRUE(idle_task_ == nullptr);
+  }
+
+  size_t NumberOfAvailableBackgroundThreads() override { return 1; }
 
   void CallOnBackgroundThread(Task* task,
                               ExpectedRuntime expected_runtime) override {
-    UNREACHABLE();
+    tasks_.push_back(task);
   }
 
   void CallOnForegroundThread(v8::Isolate* isolate, Task* task) override {
@@ -63,7 +94,7 @@ class MockPlatform : public v8::Platform {
 
   void CallIdleOnForegroundThread(v8::Isolate* isolate,
                                   IdleTask* task) override {
-    task_ = task;
+    idle_task_ = task;

[vogelheim, 2017/01/03 10:32:37]

ASSERT_TRUE(idle_task_ == nullptr) before this line?

[jochen (gone - plz use gerrit), 2017/01/03 12:59:36]

done

   }
 
   bool IdleTasksEnabled(v8::Isolate* isolate) override { return true; }
@@ -74,21 +105,78 @@ class MockPlatform : public v8::Platform {
   }
 
   void RunIdleTask(double deadline_in_seconds, double time_step) {
-    ASSERT_TRUE(task_ != nullptr);
+    ASSERT_TRUE(idle_task_ != nullptr);
     time_step_ = time_step;
-    IdleTask* task = task_;
-    task_ = nullptr;
+    IdleTask* task = idle_task_;
+    idle_task_ = nullptr;
     task->Run(deadline_in_seconds);
     delete task;
   }
 
-  bool IdleTaskPending() const { return !!task_; }
+  bool IdleTaskPending() const { return !!idle_task_; }

[vogelheim, 2017/01/03 10:32:37]

nitpick: The !! is a no-op here, since pointer-to-bool conversion is well
defined in C++. (And we rely on it in every single if(ptr) .. statement.)

+
+  bool BackgroundTasksPending() const { return !tasks_.empty(); }
+
+  void RunBackgroundTasksAndBlock(Platform* platform) {
+    std::vector<Task*> tasks;
+    tasks.swap(tasks_);
+    platform->CallOnBackgroundThread(new TaskWrapper(this, tasks, true),
+                                     kShortRunningTask);
+    sem_.Wait();
+  }
+
+  void RunBackgroundTasks(Platform* platform) {
+    std::vector<Task*> tasks;
+    tasks.swap(tasks_);
+    platform->CallOnBackgroundThread(new TaskWrapper(this, tasks, false),
+                                     kShortRunningTask);
+  }
+
+  void ClearBackgroundTasks() {
+    std::vector<Task*> tasks;
+    tasks.swap(tasks_);
+    for (auto&& task : tasks) {

[vogelheim, 2017/01/03 10:32:37]

C++ question: Why the auto&& ?

(I take it this makes no appreciable difference for auto resolving to a Task*;
but I guess it's a defensive programming thing that should work better in some
cases?)

[jochen (gone - plz use gerrit), 2017/01/03 12:59:36]

no reason actually, using auto& now

+      delete task;
+    }
+  }
+
+  void ClearIdleTask() {
+    ASSERT_TRUE(idle_task_ != nullptr);
+    delete idle_task_;
+    idle_task_ = nullptr;
+  }
 
  private:
-  IdleTask* task_;
+  class TaskWrapper : public Task {
+   public:
+    TaskWrapper(MockPlatform* platform, const std::vector<Task*>& tasks,
+                bool signal)
+        : platform_(platform), tasks_(tasks), signal_(signal) {}
+    ~TaskWrapper() = default;
+
+    void Run() override {
+      for (auto&& task : tasks_) {
+        task->Run();
+        delete task;
+      }
+      if (signal_) platform_->sem_.Signal();
+    }
+
+   private:
+    MockPlatform* platform_;
+    std::vector<Task*> tasks_;
+    bool signal_;
+
+    DISALLOW_COPY_AND_ASSIGN(TaskWrapper);
+  };
+
+  IdleTask* idle_task_;
   double time_;
   double time_step_;
 
+  std::vector<Task*> tasks_;
+  base::Semaphore sem_;
+
   DISALLOW_COPY_AND_ASSIGN(MockPlatform);
 };
 
@@ -112,8 +200,10 @@ TEST_F(CompilerDispatcherTest, IsEnqueued) {
   ASSERT_FALSE(dispatcher.IsEnqueued(shared));
   ASSERT_TRUE(dispatcher.Enqueue(shared));
   ASSERT_TRUE(dispatcher.IsEnqueued(shared));
-  dispatcher.Abort(shared, CompilerDispatcher::BlockingBehavior::kBlock);
+  dispatcher.AbortAll(CompilerDispatcher::BlockingBehavior::kBlock);
   ASSERT_FALSE(dispatcher.IsEnqueued(shared));
+  ASSERT_TRUE(platform.IdleTaskPending());
+  platform.ClearIdleTask();
 }
 
 TEST_F(CompilerDispatcherTest, FinishNow) {
@@ -132,6 +222,8 @@ TEST_F(CompilerDispatcherTest, FinishNow) {
   // Finishing removes the SFI from the queue.
   ASSERT_FALSE(dispatcher.IsEnqueued(shared));
   ASSERT_TRUE(shared->is_compiled());
+  ASSERT_TRUE(platform.IdleTaskPending());
+  platform.ClearIdleTask();
 }
 
 TEST_F(CompilerDispatcherTest, IdleTask) {
@@ -188,7 +280,7 @@ TEST_F(CompilerDispatcherTest, IdleTaskSmallIdleTime) {
   ASSERT_TRUE(dispatcher.jobs_.begin()->second->status() ==
               CompileJobStatus::kReadyToParse);
 
-  // Only grant a lot of idle time and freeze time.
+  // Now grant a lot of idle time and freeze time.
   platform.RunIdleTask(1000.0, 0.0);
 
   ASSERT_FALSE(dispatcher.IsEnqueued(shared));
@@ -225,5 +317,90 @@ TEST_F(CompilerDispatcherTest, IdleTaskException) {
   ASSERT_FALSE(try_catch.HasCaught());
 }
 
+TEST_F(IgnitionCompilerDispatcherTest, CompileOnBackgroundThread) {
+  MockPlatform platform;

[marja, 2017/01/03 10:42:41]

You could make MockPlatform a member of CompilerDispatcherTest, so you wouldn't
need to have it explicitly in each test...

[jochen (gone - plz use gerrit), 2017/01/03 12:59:36]

meh, unless you feel strongly about this

[marja, 2017/01/03 13:14:57]

I don't :)

+  CompilerDispatcher dispatcher(i_isolate(), &platform, FLAG_stack_size);
+
+  const char script[] =
+      "function g() { var y = 1; function f6(x) { return x * y }; return f6; } "
+      "g();";
+  Handle<JSFunction> f = Handle<JSFunction>::cast(RunJS(isolate(), script));
+  Handle<SharedFunctionInfo> shared(f->shared(), i_isolate());
+
+  ASSERT_FALSE(platform.IdleTaskPending());
+  ASSERT_TRUE(dispatcher.Enqueue(shared));
+  ASSERT_TRUE(platform.IdleTaskPending());
+
+  ASSERT_EQ(dispatcher.jobs_.size(), 1u);
+  ASSERT_TRUE(dispatcher.jobs_.begin()->second->status() ==
+              CompileJobStatus::kInitial);
+
+  // Make compiling super expensive, and advance job as much as possible on the
+  // foreground thread.
+  dispatcher.tracer_->RecordCompile(50000.0, 1);
+  platform.RunIdleTask(10.0, 0.0);
+  ASSERT_TRUE(dispatcher.jobs_.begin()->second->status() ==
+              CompileJobStatus::kReadyToCompile);
+
+  ASSERT_TRUE(dispatcher.IsEnqueued(shared));
+  ASSERT_FALSE(shared->is_compiled());
+  ASSERT_FALSE(platform.IdleTaskPending());
+  ASSERT_TRUE(platform.BackgroundTasksPending());
+
+  platform.RunBackgroundTasksAndBlock(V8::GetCurrentPlatform());
+
+  ASSERT_TRUE(platform.IdleTaskPending());
+  ASSERT_FALSE(platform.BackgroundTasksPending());
+  ASSERT_TRUE(dispatcher.jobs_.begin()->second->status() ==
+              CompileJobStatus::kCompiled);
+
+  // Now grant a lot of idle time and freeze time.
+  platform.RunIdleTask(1000.0, 0.0);

[vogelheim, 2017/01/03 10:32:37]

[For my understanding:] What does this step accomplish?

This test ultimately relies on the fact that a very expensive compile job will
have exactly one background step?

[jochen (gone - plz use gerrit), 2017/01/03 12:59:36]

this step does the final step of compilation which needs to be on the main
thread (copy bytecode to the heap and the shared function info)

+
+  ASSERT_FALSE(dispatcher.IsEnqueued(shared));
+  ASSERT_TRUE(shared->is_compiled());
+  ASSERT_FALSE(platform.IdleTaskPending());
+}
+
+TEST_F(IgnitionCompilerDispatcherTest, FinishNowWithBackgroundTask) {
+  MockPlatform platform;
+  CompilerDispatcher dispatcher(i_isolate(), &platform, FLAG_stack_size);
+
+  const char script[] =
+      "function g() { var y = 1; function f7(x) { return x * y }; return f7; } "
+      "g();";
+  Handle<JSFunction> f = Handle<JSFunction>::cast(RunJS(isolate(), script));
+  Handle<SharedFunctionInfo> shared(f->shared(), i_isolate());
+
+  ASSERT_FALSE(platform.IdleTaskPending());
+  ASSERT_TRUE(dispatcher.Enqueue(shared));
+  ASSERT_TRUE(platform.IdleTaskPending());
+
+  ASSERT_EQ(dispatcher.jobs_.size(), 1u);
+  ASSERT_TRUE(dispatcher.jobs_.begin()->second->status() ==
+              CompileJobStatus::kInitial);
+
+  // Make compiling super expensive, and advance job as much as possible on the
+  // foreground thread.
+  dispatcher.tracer_->RecordCompile(50000.0, 1);
+  platform.RunIdleTask(10.0, 0.0);
+  ASSERT_TRUE(dispatcher.jobs_.begin()->second->status() ==
+              CompileJobStatus::kReadyToCompile);
+
+  ASSERT_TRUE(dispatcher.IsEnqueued(shared));
+  ASSERT_FALSE(shared->is_compiled());
+  ASSERT_FALSE(platform.IdleTaskPending());
+  ASSERT_TRUE(platform.BackgroundTasksPending());
+
+  platform.RunBackgroundTasks(V8::GetCurrentPlatform());

[marja, 2017/01/03 10:42:41]

For documenting what happens, you could assert the status of the job here.
Afaics it's now compiled...

You could also add another test for FinishNow:ing a task which is in the queue
for background processing but not done yet.

Looks like with this kind of tests you cannot easily test a situation where
you're actually doing the background work when FinishNow is called... or can you
mock that somehow too?

[jochen (gone - plz use gerrit), 2017/01/03 12:59:36]

RunBackgroundTasks() does not block, so this test creates a race between the
background task and FinishNow()

+
+  ASSERT_TRUE(dispatcher.FinishNow(shared));
+  // Finishing removes the SFI from the queue.
+  ASSERT_FALSE(dispatcher.IsEnqueued(shared));
+  ASSERT_TRUE(shared->is_compiled());
+  ASSERT_FALSE(platform.IdleTaskPending());
+  ASSERT_FALSE(platform.BackgroundTasksPending());
+}
+
 }  // namespace internal
 }  // namespace v8