[V8 Platform] Better WorkerThreads APIs.

As discussed @ https://chromium-review.googlesource.com/c/chromium/src/+/957761#message-4ba6c1bf637f91507544efc89a31e3e4dd407715
and again @ https://chromium-review.googlesource.com/c/chromium/src/+/957761#message-6d0430e640c82f2d5463259fecdc7fabf945b958

Get rid of task runners for WorkerThreads API (use case is always a
one-off task in which case a static call is fine -- just like in
Chromium's base/task_scheduler/post_task.h)

Calling into V8Platform* from any worker thread is safe, what was previously
unsafe was using an Isolate* from worker threads but Isolate* was dropped
from the new worker threads APIs so this is now irrelevant.

Bug: chromium:817421
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Change-Id: Idd2dbc081edfbcb8985eeb45eb64ffb2555fcf7c
Reviewed-on: https://chromium-review.googlesource.com/978443
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52893}

include/v8-platform.h

@@ -289,11 +289,11 @@ class Platform {
   virtual bool OnCriticalMemoryPressure(size_t length) { return false; }
 
   /**
-   * Gets the number of worker threads used by GetWorkerThreadsTaskRunner() and
-   * CallOnWorkerThread(). This can be used to estimate the number of tasks a
-   * work package should be split into. A return value of 0 means that there are
-   * no worker threads available. Note that a value of 0 won't prohibit V8 from
-   * posting tasks using |CallOnWorkerThread|.
+   * Gets the number of worker threads used by
+   * Call(BlockingTask)OnWorkerThread(). This can be used to estimate the number
+   * of tasks a work package should be split into. A return value of 0 means
+   * that there are no worker threads available. Note that a value of 0 won't
+   * prohibit V8 from posting tasks using |CallOnWorkerThread|.
    */
   virtual int NumberOfWorkerThreads() {
     return static_cast<int>(NumberOfAvailableBackgroundThreads());
@@ -328,34 +328,14 @@ class Platform {
    */
   V8_DEPRECATE_SOON(
       "GetBackgroundTaskRunner() is deprecated, use "
-      "GetWorkerThreadsTaskRunner() "
-      "instead.",
+      "CallOnWorkerThread() instead.",
       virtual std::shared_ptr<v8::TaskRunner> GetBackgroundTaskRunner(
           Isolate* isolate)) {
-    // TODO(gab): Remove this method when all embedders have moved to
-    // GetWorkerThreadsTaskRunner().
-
-    // An implementation needs to be provided here because this is called by the
-    // default GetWorkerThreadsTaskRunner() implementation below. In practice
-    // however, all code either:
-    //  - Overrides GetWorkerThreadsTaskRunner() (thus not making this call) --
-    //    i.e. all v8 code.
-    //  - Overrides this method (thus not making this call) -- i.e. all
-    //    unadapted embedders.
+    // Deprecated and unused.
+    // TODO(gab): Remove this when embedders no longer override it.
     abort();
   }
 
-  /**
-   * Returns a TaskRunner which can be used to post async tasks on a worker.
-   * This function should only be called from a foreground thread.
-   */
-  virtual std::shared_ptr<v8::TaskRunner> GetWorkerThreadsTaskRunner(
-      Isolate* isolate) {
-    // TODO(gab): Make this function abstract after it got implemented on all
-    // platforms.
-    return GetBackgroundTaskRunner(isolate);
-  }
-
   /**
    * Schedules a task to be invoked on a background thread. |expected_runtime|
    * indicates that the task will run a long time. The Platform implementation

src/d8.cc

@@ -181,13 +181,6 @@ class PredictablePlatform : public Platform {
     return platform_->GetForegroundTaskRunner(isolate);
   }
 
-  std::shared_ptr<TaskRunner> GetWorkerThreadsTaskRunner(
-      v8::Isolate* isolate) override {
-    // Return the foreground task runner here, so that all tasks get executed
-    // sequentially in a predictable order.
-    return platform_->GetForegroundTaskRunner(isolate);
-  }
-
   void CallOnWorkerThread(std::unique_ptr<Task> task) override {
     // It's not defined when background tasks are being executed, so we can just
     // execute them right away.

src/libplatform/default-platform.cc

@@ -193,19 +193,15 @@ std::shared_ptr<TaskRunner> DefaultPlatform::GetForegroundTaskRunner(
   return foreground_task_runner_map_[isolate];
 }
 
-std::shared_ptr<TaskRunner> DefaultPlatform::GetWorkerThreadsTaskRunner(
-    v8::Isolate*) {
-  EnsureBackgroundTaskRunnerInitialized();
-  return worker_threads_task_runner_;
-}
-
 void DefaultPlatform::CallOnWorkerThread(std::unique_ptr<Task> task) {
-  GetWorkerThreadsTaskRunner(nullptr)->PostTask(std::move(task));
+  EnsureBackgroundTaskRunnerInitialized();
+  worker_threads_task_runner_->PostTask(std::move(task));
 }
 
 void DefaultPlatform::CallDelayedOnWorkerThread(std::unique_ptr<Task> task,
                                                 double delay_in_seconds) {
-  GetWorkerThreadsTaskRunner(nullptr)->PostDelayedTask(std::move(task),
+  EnsureBackgroundTaskRunnerInitialized();
+  worker_threads_task_runner_->PostDelayedTask(std::move(task),
                                                delay_in_seconds);
 }
 

src/libplatform/default-platform.h

@@ -58,8 +58,6 @@ class V8_PLATFORM_EXPORT DefaultPlatform : public NON_EXPORTED_BASE(Platform) {
   int NumberOfWorkerThreads() override;
   std::shared_ptr<TaskRunner> GetForegroundTaskRunner(
       v8::Isolate* isolate) override;
-  std::shared_ptr<TaskRunner> GetWorkerThreadsTaskRunner(
-      v8::Isolate* isolate) override;
   void CallOnWorkerThread(std::unique_ptr<Task> task) override;
   void CallDelayedOnWorkerThread(std::unique_ptr<Task> task,
                                  double delay_in_seconds) override;

src/wasm/module-compiler.cc

@@ -187,7 +187,6 @@ class CompilationState {
   // the CompilationState in order to cleanly clean up.
   CancelableTaskManager background_task_manager_;
   CancelableTaskManager foreground_task_manager_;
-  std::shared_ptr<v8::TaskRunner> background_task_runner_;
   std::shared_ptr<v8::TaskRunner> foreground_task_runner_;
 
   const size_t max_background_tasks_ = 0;
@@ -2776,7 +2775,6 @@ AsyncCompileJob::AsyncCompileJob(Isolate* isolate,
   v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate);
   v8::Platform* platform = V8::GetCurrentPlatform();
   foreground_task_runner_ = platform->GetForegroundTaskRunner(v8_isolate);
-  background_task_runner_ = platform->GetWorkerThreadsTaskRunner(v8_isolate);
   // The handles for the context and promise must be deferred.
   DeferredHandleScope deferred(isolate);
   context_ = Handle<Context>(*context);
@@ -2976,12 +2974,15 @@ void AsyncCompileJob::DoSync(Args&&... args) {
 }
 
 void AsyncCompileJob::StartBackgroundTask() {
+  auto task = base::make_unique<CompileTask>(this, false);
+
   // If --wasm-num-compilation-tasks=0 is passed, do only spawn foreground
   // tasks. This is used to make timing deterministic.
-  v8::TaskRunner* task_runner = FLAG_wasm_num_compilation_tasks > 0
-                                    ? background_task_runner_.get()
-                                    : foreground_task_runner_.get();
-  task_runner->PostTask(base::make_unique<CompileTask>(this, false));
+  if (FLAG_wasm_num_compilation_tasks > 0) {
+    V8::GetCurrentPlatform()->CallOnWorkerThread(std::move(task));
+  } else {
+    foreground_task_runner_->PostTask(std::move(task));
+  }
 }
 
 template <typename Step, typename... Args>
@@ -3471,7 +3472,6 @@ CompilationState::CompilationState(internal::Isolate* isolate, ModuleEnv& env)
   v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate_);
   v8::Platform* platform = V8::GetCurrentPlatform();
   foreground_task_runner_ = platform->GetForegroundTaskRunner(v8_isolate);
-  background_task_runner_ = platform->GetWorkerThreadsTaskRunner(v8_isolate);
 
   // Register task manager for clean shutdown in case of an isolate shutdown.
   isolate_->wasm_engine()->Register(&background_task_manager_);
@@ -3641,14 +3641,17 @@ void CompilationState::RestartBackgroundTasks(size_t max) {
     num_background_tasks_ += num_restart;
   }
 
+  for (; num_restart > 0; --num_restart) {
+    auto task = base::make_unique<BackgroundCompileTask>(
+        this, &background_task_manager_);
+
     // If --wasm-num-compilation-tasks=0 is passed, do only spawn foreground
     // tasks. This is used to make timing deterministic.
-  v8::TaskRunner* task_runner = FLAG_wasm_num_compilation_tasks > 0
-                                    ? background_task_runner_.get()
-                                    : foreground_task_runner_.get();
-  for (; num_restart > 0; --num_restart) {
-    task_runner->PostTask(base::make_unique<BackgroundCompileTask>(
-        this, &background_task_manager_));
+    if (FLAG_wasm_num_compilation_tasks > 0) {
+      V8::GetCurrentPlatform()->CallOnWorkerThread(std::move(task));
+    } else {
+      foreground_task_runner_->PostTask(std::move(task));
+    }
   }
 }
 

src/wasm/module-compiler.h

@@ -159,7 +159,6 @@ class AsyncCompileJob {
   Handle<Code> centry_stub_;
 
   std::shared_ptr<v8::TaskRunner> foreground_task_runner_;
-  std::shared_ptr<v8::TaskRunner> background_task_runner_;
 
   // For async compilation the AsyncCompileJob is the only finisher. For
   // streaming compilation also the AsyncStreamingProcessor has to finish before

test/cctest/cctest.h

@@ -685,11 +685,6 @@ class TestPlatform : public v8::Platform {
     return old_platform_->GetForegroundTaskRunner(isolate);
   }
 
-  std::shared_ptr<v8::TaskRunner> GetWorkerThreadsTaskRunner(
-      v8::Isolate* isolate) override {
-    return old_platform_->GetWorkerThreadsTaskRunner(isolate);
-  }
-
   void CallOnWorkerThread(std::unique_ptr<v8::Task> task) override {
     old_platform_->CallOnWorkerThread(std::move(task));
   }

test/cctest/wasm/test-streaming-compilation.cc

@@ -35,11 +35,6 @@ class MockPlatform final : public TestPlatform {
     return task_runner_;
   }
 
-  std::shared_ptr<TaskRunner> GetWorkerThreadsTaskRunner(
-      v8::Isolate* isolate) override {
-    return task_runner_;
-  }
-
   void CallOnForegroundThread(v8::Isolate* isolate, Task* task) override {
     task_runner_->PostTask(std::unique_ptr<Task>(task));
   }

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

@@ -102,14 +102,7 @@ class MockPlatform : public v8::Platform {
 
   std::shared_ptr<TaskRunner> GetForegroundTaskRunner(
       v8::Isolate* isolate) override {
-    constexpr bool is_foreground_task_runner = true;
-    return std::make_shared<MockTaskRunner>(this, is_foreground_task_runner);
-  }
-
-  std::shared_ptr<TaskRunner> GetWorkerThreadsTaskRunner(
-      v8::Isolate* isolate) override {
-    constexpr bool is_foreground_task_runner = false;
-    return std::make_shared<MockTaskRunner>(this, is_foreground_task_runner);
+    return std::make_shared<MockForegroundTaskRunner>(this);
   }
 
   void CallOnWorkerThread(std::unique_ptr<Task> task) override {
@@ -259,19 +252,14 @@ class MockPlatform : public v8::Platform {
     DISALLOW_COPY_AND_ASSIGN(TaskWrapper);
   };
 
-  class MockTaskRunner final : public TaskRunner {
+  class MockForegroundTaskRunner final : public TaskRunner {
    public:
-    MockTaskRunner(MockPlatform* platform, bool is_foreground_task_runner)
-        : platform_(platform),
-          is_foreground_task_runner_(is_foreground_task_runner) {}
+    explicit MockForegroundTaskRunner(MockPlatform* platform)
+        : platform_(platform) {}
 
     void PostTask(std::unique_ptr<v8::Task> task) override {
       base::LockGuard<base::Mutex> lock(&platform_->mutex_);
-      if (is_foreground_task_runner_) {
       platform_->foreground_tasks_.push_back(std::move(task));
-      } else {
-        platform_->worker_tasks_.push_back(std::move(task));
-      }
     }
 
     void PostDelayedTask(std::unique_ptr<Task> task,
@@ -286,14 +274,10 @@ class MockPlatform : public v8::Platform {
       platform_->idle_task_ = task.release();
     }
 
-    bool IdleTasksEnabled() override {
-      // Idle tasks are enabled only in the foreground task runner.
-      return is_foreground_task_runner_;
-    };
+    bool IdleTasksEnabled() override { return true; };
 
    private:
     MockPlatform* platform_;
-    bool is_foreground_task_runner_;
   };
 
   double time_;

test/unittests/libplatform/default-platform-unittest.cc

@@ -258,48 +258,32 @@ class TestBackgroundTask : public Task {
 }  // namespace
 
 TEST(DefaultPlatformTest, RunBackgroundTask) {
-  int dummy;
-  Isolate* isolate = reinterpret_cast<Isolate*>(&dummy);
-
   DefaultPlatform platform;
   platform.SetThreadPoolSize(1);
-  std::shared_ptr<TaskRunner> taskrunner =
-      platform.GetWorkerThreadsTaskRunner(isolate);
 
   base::Semaphore sem(0);
   bool task_executed = false;
   StrictMock<TestBackgroundTask>* task =
       new StrictMock<TestBackgroundTask>(&sem, &task_executed);
   EXPECT_CALL(*task, Die());
-  taskrunner->PostTask(std::unique_ptr<Task>(task));
+  platform.CallOnWorkerThread(std::unique_ptr<Task>(task));
   EXPECT_TRUE(sem.WaitFor(base::TimeDelta::FromSeconds(1)));
   EXPECT_TRUE(task_executed);
 }
 
-TEST(DefaultPlatformTest, NoIdleTasksInBackground) {
-  int dummy;
-  Isolate* isolate = reinterpret_cast<Isolate*>(&dummy);
-  DefaultPlatform platform;
-  platform.SetThreadPoolSize(1);
-  std::shared_ptr<TaskRunner> taskrunner =
-      platform.GetWorkerThreadsTaskRunner(isolate);
-  EXPECT_FALSE(taskrunner->IdleTasksEnabled());
-}
-
-TEST(DefaultPlatformTest, PostTaskAfterPlatformTermination) {
+TEST(DefaultPlatformTest, PostForegroundTaskAfterPlatformTermination) {
   std::shared_ptr<TaskRunner> foreground_taskrunner;
-  std::shared_ptr<TaskRunner> background_taskrunner;
   {
+    DefaultPlatformWithMockTime platform;
+
     int dummy;
     Isolate* isolate = reinterpret_cast<Isolate*>(&dummy);
 
-    DefaultPlatformWithMockTime platform;
     platform.SetThreadPoolSize(1);
     foreground_taskrunner = platform.GetForegroundTaskRunner(isolate);
-    background_taskrunner = platform.GetWorkerThreadsTaskRunner(isolate);
   }
-  // It should still be possible to post tasks, even when the platform does not
-  // exist anymore.
+  // It should still be possible to post foreground tasks, even when the
+  // platform does not exist anymore.
   StrictMock<MockTask>* task1 = new StrictMock<MockTask>;
   EXPECT_CALL(*task1, Die());
   foreground_taskrunner->PostTask(std::unique_ptr<Task>(task1));
@@ -311,10 +295,6 @@ TEST(DefaultPlatformTest, PostTaskAfterPlatformTermination) {
   StrictMock<MockIdleTask>* task3 = new StrictMock<MockIdleTask>;
   EXPECT_CALL(*task3, Die());
   foreground_taskrunner->PostIdleTask(std::unique_ptr<IdleTask>(task3));
-
-  StrictMock<MockTask>* task4 = new StrictMock<MockTask>;
-  EXPECT_CALL(*task4, Die());
-  background_taskrunner->PostTask(std::unique_ptr<Task>(task4));
 }
 
 }  // namespace default_platform_unittest