[vm] Use std::unique_ptr for ThreadPool::Task

Updates #37244.

Change-Id: I32a5180a17fe43be5e18367d784cf756dffc6aeb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106009
Commit-Queue: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Régis Crelier <regis@google.com>

runtime/lib/isolate.cc

@@ -245,14 +245,7 @@ DEFINE_NATIVE_ENTRY(Isolate_spawnFunction, 0, 11) {
       // Since this is a call to Isolate.spawn, copy the parent isolate's code.
       state->isolate_flags()->copy_parent_code = true;
 
-      ThreadPool::Task* spawn_task =
-          new SpawnIsolateTask(isolate, std::move(state));
-
-      if (!Dart::thread_pool()->Run(spawn_task)) {
-        // Running on the thread pool failed. Clean up everything.
-        delete spawn_task;
-        spawn_task = NULL;
-      }
+      Dart::thread_pool()->Run<SpawnIsolateTask>(isolate, std::move(state));
       return Object::null();
     }
   }
@@ -373,14 +366,7 @@ DEFINE_NATIVE_ENTRY(Isolate_spawnUri, 0, 13) {
   // Since this is a call to Isolate.spawnUri, don't copy the parent's code.
   state->isolate_flags()->copy_parent_code = false;
 
-  ThreadPool::Task* spawn_task =
-      new SpawnIsolateTask(isolate, std::move(state));
-
-  if (!Dart::thread_pool()->Run(spawn_task)) {
-    // Running on the thread pool failed. Clean up everything.
-    delete spawn_task;
-    spawn_task = NULL;
-  }
+  Dart::thread_pool()->Run<SpawnIsolateTask>(isolate, std::move(state));
   return Object::null();
 }
 

runtime/vm/compiler/jit/compiler.cc

@@ -1389,8 +1389,7 @@ void BackgroundCompiler::Start() {
   if (running_ || !done_) return;
   running_ = true;
   done_ = false;
-  bool task_started =
-      Dart::thread_pool()->Run(new BackgroundCompilerTask(this));
+  bool task_started = Dart::thread_pool()->Run<BackgroundCompilerTask>(this);
   if (!task_started) {
     running_ = false;
     done_ = true;

runtime/vm/heap/compactor.cc

@@ -234,9 +234,9 @@ void GCCompactor::Compact(HeapPage* pages,
     intptr_t next_forwarding_task = 0;
 
     for (intptr_t task_index = 0; task_index < num_tasks; task_index++) {
-      Dart::thread_pool()->Run(new CompactorTask(
+      Dart::thread_pool()->Run<CompactorTask>(
           thread()->isolate(), this, &barrier, &next_forwarding_task,
-          heads[task_index], &tails[task_index], freelist));
+          heads[task_index], &tails[task_index], freelist);
     }
 
     // Plan pages.

runtime/vm/heap/marker.cc

@@ -804,8 +804,8 @@ void GCMarker::StartConcurrentMark(PageSpace* page_space) {
                                           &deferred_marking_stack_);
 
     // Begin marking on a helper thread.
-    bool result = Dart::thread_pool()->Run(
-        new ConcurrentMarkTask(this, isolate_, page_space, visitors_[i]));
+    bool result = Dart::thread_pool()->Run<ConcurrentMarkTask>(
+        this, isolate_, page_space, visitors_[i]);
     ASSERT(result);
   }
 
@@ -862,8 +862,8 @@ void GCMarker::MarkObjects(PageSpace* page_space) {
               isolate_, page_space, &marking_stack_, &deferred_marking_stack_);
         }
 
-        bool result = Dart::thread_pool()->Run(new ParallelMarkTask(
-            this, isolate_, &marking_stack_, &barrier, visitor, &num_busy));
+        bool result = Dart::thread_pool()->Run<ParallelMarkTask>(
+            this, isolate_, &marking_stack_, &barrier, visitor, &num_busy);
         ASSERT(result);
       }
       bool more_to_mark = false;

runtime/vm/heap/sweeper.cc

@@ -182,8 +182,8 @@ void GCSweeper::SweepConcurrent(Isolate* isolate,
                                 HeapPage* first,
                                 HeapPage* last,
                                 FreeList* freelist) {
-  bool result = Dart::thread_pool()->Run(new ConcurrentSweeperTask(
-      isolate, isolate->heap()->old_space(), first, last, freelist));
+  bool result = Dart::thread_pool()->Run<ConcurrentSweeperTask>(
+      isolate, isolate->heap()->old_space(), first, last, freelist);
   ASSERT(result);
 }
 

runtime/vm/isolate_test.cc

@@ -130,7 +130,7 @@ TEST_CASE(StackLimitInterrupts) {
                         isolate->heap()->barrier_done());
   // Start all tasks. They will busy-wait until interrupted in the first round.
   for (intptr_t task = 0; task < InterruptChecker::kTaskCount; task++) {
-    Dart::thread_pool()->Run(new InterruptChecker(thread, &barrier));
+    Dart::thread_pool()->Run<InterruptChecker>(thread, &barrier);
   }
   // Wait for all tasks to get ready for the first round.
   barrier.Sync();

runtime/vm/kernel_isolate.cc

@@ -232,7 +232,7 @@ void KernelIsolate::Run() {
     KernelIsolate::InitializingFailed();
     return;
   }
-  bool task_started = Dart::thread_pool()->Run(new RunKernelTask());
+  bool task_started = Dart::thread_pool()->Run<RunKernelTask>();
   ASSERT(task_started);
 }
 

runtime/vm/message_handler.cc

@@ -114,7 +114,7 @@ void MessageHandler::Run(ThreadPool* pool,
   start_callback_ = start_callback;
   end_callback_ = end_callback;
   callback_data_ = data;
-  task_running_ = pool_->Run(new MessageHandlerTask(this));
+  task_running_ = pool_->Run<MessageHandlerTask>(this);
   ASSERT(task_running_);
 }
 
@@ -157,7 +157,7 @@ void MessageHandler::PostMessage(std::unique_ptr<Message> message,
 
     if ((pool_ != NULL) && !task_running_) {
       ASSERT(!delete_me_);
-      task_running_ = pool_->Run(new MessageHandlerTask(this));
+      task_running_ = pool_->Run<MessageHandlerTask>(this);
       ASSERT(task_running_);
     }
   }

runtime/vm/service_isolate.cc

@@ -483,7 +483,7 @@ void ServiceIsolate::Run() {
     ServiceIsolate::InitializingFailed();
     return;
   }
-  bool task_started = Dart::thread_pool()->Run(new RunServiceTask());
+  bool task_started = Dart::thread_pool()->Run<RunServiceTask>();
   ASSERT(task_started);
 }
 

runtime/vm/thread_barrier_test.cc

@@ -45,7 +45,7 @@ VM_UNIT_TEST_CASE(ThreadBarrier) {
   {
     ThreadBarrier barrier(kNumTasks + 1, monitor, monitor_done);
     for (intptr_t i = 0; i < kNumTasks; ++i) {
-      Dart::thread_pool()->Run(new FuzzTask(kNumRounds, &barrier, i + 1));
+      Dart::thread_pool()->Run<FuzzTask>(kNumRounds, &barrier, i + 1);
     }
     for (intptr_t i = 0; i < kNumRounds; ++i) {
       barrier.Sync();

runtime/vm/thread_pool.cc

@@ -30,7 +30,7 @@ ThreadPool::~ThreadPool() {
   Shutdown();
 }
 
-bool ThreadPool::Run(Task* task) {
+bool ThreadPool::RunImpl(std::unique_ptr<Task> task) {
   Worker* worker = NULL;
   bool new_worker = false;
   {
@@ -63,7 +63,7 @@ bool ThreadPool::Run(Task* task) {
 
   // Release ThreadPool::mutex_ before calling Worker functions.
   ASSERT(worker != NULL);
-  worker->SetTask(task);
+  worker->SetTask(std::move(task));
   if (new_worker) {
     // Call StartThread after we've assigned the first task.
     worker->StartThread();
@@ -314,7 +314,7 @@ ThreadPool::Task::~Task() {}
 
 ThreadPool::Worker::Worker(ThreadPool* pool)
     : pool_(pool),
-      task_(NULL),
+      task_(nullptr),
       id_(OSThread::kInvalidThreadId),
       done_(false),
       owned_(false),
@@ -332,7 +332,7 @@ void ThreadPool::Worker::StartThread() {
   // Must call SetTask before StartThread.
   {  // NOLINT
     MonitorLocker ml(&monitor_);
-    ASSERT(task_ != NULL);
+    ASSERT(task_ != nullptr);
   }
 #endif
   int result = OSThread::Start("Dart ThreadPool Worker", &Worker::Main,
@@ -342,10 +342,10 @@ void ThreadPool::Worker::StartThread() {
   }
 }
 
-void ThreadPool::Worker::SetTask(Task* task) {
+void ThreadPool::Worker::SetTask(std::unique_ptr<Task> task) {
   MonitorLocker ml(&monitor_);
-  ASSERT(task_ == NULL);
-  task_ = task;
+  ASSERT(task_ == nullptr);
+  task_ = std::move(task);
   ml.Notify();
 }
 
@@ -372,18 +372,17 @@ bool ThreadPool::Worker::Loop() {
   MonitorLocker ml(&monitor_);
   int64_t idle_start;
   while (true) {
-    ASSERT(task_ != NULL);
-    Task* task = task_;
-    task_ = NULL;
+    ASSERT(task_ != nullptr);
+    std::unique_ptr<Task> task = std::move(task_);
 
     // Release monitor while handling the task.
     ml.Exit();
     task->Run();
     ASSERT(Isolate::Current() == NULL);
-    delete task;
+    task.reset();
     ml.Enter();
 
-    ASSERT(task_ == NULL);
+    ASSERT(task_ == nullptr);
     if (IsDone()) {
       return false;
     }
@@ -392,7 +391,7 @@ bool ThreadPool::Worker::Loop() {
     idle_start = OS::GetCurrentMonotonicMicros();
     while (true) {
       Monitor::WaitResult result = ml.WaitMicros(ComputeTimeout(idle_start));
-      if (task_ != NULL) {
+      if (task_ != nullptr) {
         // We've found a task.  Process it, regardless of whether the
         // worker is done_.
         break;

runtime/vm/thread_pool.h

@@ -5,6 +5,9 @@
 #ifndef RUNTIME_VM_THREAD_POOL_H_
 #define RUNTIME_VM_THREAD_POOL_H_
 
+#include <memory>
+#include <utility>
+
 #include "vm/allocation.h"
 #include "vm/globals.h"
 #include "vm/os_thread.h"
@@ -35,7 +38,10 @@ class ThreadPool {
   ~ThreadPool();
 
   // Runs a task on the thread pool.
-  bool Run(Task* task);
+  template <typename T, typename... Args>
+  bool Run(Args&&... args) {
+    return RunImpl(std::unique_ptr<Task>(new T(std::forward<Args>(args)...)));
+  }
 
   // Some simple stats.
   uint64_t workers_running() const { return count_running_; }
@@ -49,7 +55,7 @@ class ThreadPool {
     explicit Worker(ThreadPool* pool);
 
     // Sets a task on the worker.
-    void SetTask(Task* task);
+    void SetTask(std::unique_ptr<Task> task);
 
     // Starts the thread for the worker.  This should only be called
     // after a task has been set by the initial call to SetTask().
@@ -76,7 +82,7 @@ class ThreadPool {
     // Fields owned by Worker.
     Monitor monitor_;
     ThreadPool* pool_;
-    Task* task_;
+    std::unique_ptr<Task> task_;
     ThreadId id_;
     bool done_;
 
@@ -110,6 +116,7 @@ class ThreadPool {
     DISALLOW_COPY_AND_ASSIGN(JoinList);
   };
 
+  bool RunImpl(std::unique_ptr<Task> task);
   void Shutdown();
 
   // Expensive.  Use only in assertions.

runtime/vm/thread_pool_test.cc

@@ -44,7 +44,7 @@ VM_UNIT_TEST_CASE(ThreadPool_RunOne) {
   ThreadPool thread_pool;
   Monitor sync;
   bool done = true;
-  thread_pool.Run(new TestTask(&sync, &done));
+  thread_pool.Run<TestTask>(&sync, &done);
   {
     MonitorLocker ml(&sync);
     done = false;
@@ -68,7 +68,7 @@ VM_UNIT_TEST_CASE(ThreadPool_RunMany) {
 
   for (int i = 0; i < kTaskCount; i++) {
     done[i] = true;
-    thread_pool.Run(new TestTask(&sync[i], &done[i]));
+    thread_pool.Run<TestTask>(&sync[i], &done[i]);
   }
   for (int i = 0; i < kTaskCount; i++) {
     MonitorLocker ml(&sync[i]);
@@ -124,7 +124,7 @@ VM_UNIT_TEST_CASE(ThreadPool_WorkerShutdown) {
 
   // Run a single task.
   for (int i = 0; i < kTaskCount; i++) {
-    thread_pool->Run(new SleepTask(&sync, &started_count, &slept_count, 2));
+    thread_pool->Run<SleepTask>(&sync, &started_count, &slept_count, 2);
   }
 
   {
@@ -162,7 +162,7 @@ VM_UNIT_TEST_CASE(ThreadPool_WorkerTimeout) {
   // Run a worker.
   Monitor sync;
   bool done = true;
-  thread_pool.Run(new TestTask(&sync, &done));
+  thread_pool.Run<TestTask>(&sync, &done);
   EXPECT_EQ(1U, thread_pool.workers_started());
   EXPECT_EQ(0U, thread_pool.workers_stopped());
   {
@@ -197,11 +197,10 @@ class SpawnTask : public ThreadPool::Task {
 
     // Spawn 0-2 children.
     if (todo_ > 0) {
-      pool_->Run(
-          new SpawnTask(pool_, sync_, todo_ - child_todo, total_, done_));
+      pool_->Run<SpawnTask>(pool_, sync_, todo_ - child_todo, total_, done_);
     }
     if (todo_ > 1) {
-      pool_->Run(new SpawnTask(pool_, sync_, child_todo, total_, done_));
+      pool_->Run<SpawnTask>(pool_, sync_, child_todo, total_, done_);
     }
 
     {
@@ -226,8 +225,8 @@ VM_UNIT_TEST_CASE(ThreadPool_RecursiveSpawn) {
   Monitor sync;
   const int kTotalTasks = 500;
   int done = 0;
-  thread_pool.Run(
-      new SpawnTask(&thread_pool, &sync, kTotalTasks, kTotalTasks, &done));
+  thread_pool.Run<SpawnTask>(&thread_pool, &sync, kTotalTasks, kTotalTasks,
+                             &done);
   {
     MonitorLocker ml(&sync);
     while (done < kTotalTasks) {

runtime/vm/thread_test.cc

@@ -180,8 +180,8 @@ ISOLATE_UNIT_TEST_CASE(ManyTasksWithZones) {
   isolate->heap()->DisableGrowthControl();
   for (int i = 0; i < kTaskCount; i++) {
     done[i] = false;
-    Dart::thread_pool()->Run(
-        new TaskWithZoneAllocation(isolate, &sync[i], &done[i], i));
+    Dart::thread_pool()->Run<TaskWithZoneAllocation>(isolate, &sync[i],
+                                                     &done[i], i);
   }
   bool in_isolate = true;
   for (int i = 0; i < kTaskCount; i++) {
@@ -302,8 +302,8 @@ ISOLATE_UNIT_TEST_CASE(ManySimpleTasksWithZones) {
   EXPECT(isolate->heap()->GrowthControlState());
   isolate->heap()->DisableGrowthControl();
   for (intptr_t i = 0; i < kTaskCount; i++) {
-    Dart::thread_pool()->Run(new SimpleTaskWithZoneAllocation(
-        (i + 1), isolate, &threads[i], &sync, &monitor, &done_count, &wait));
+    Dart::thread_pool()->Run<SimpleTaskWithZoneAllocation>(
+        (i + 1), isolate, &threads[i], &sync, &monitor, &done_count, &wait);
   }
   // Wait until all spawned tasks finish their memory operations.
   {
@@ -497,8 +497,8 @@ ISOLATE_UNIT_TEST_CASE(ICDataTest) {
   }
 
   for (int i = 0; i < ICDataTestTask::kTaskCount; i++) {
-    Dart::thread_pool()->Run(
-        new ICDataTestTask(isolate, ic_datas, &monitor, &exited, &done));
+    Dart::thread_pool()->Run<ICDataTestTask>(isolate, ic_datas, &monitor,
+                                             &exited, &done);
   }
 
   for (int i = 0; i < 0x10000; i++) {
@@ -630,8 +630,8 @@ TEST_CASE(SafepointTestDart) {
   intptr_t total_done = 0;
   intptr_t exited = 0;
   for (int i = 0; i < SafepointTestTask::kTaskCount; i++) {
-    Dart::thread_pool()->Run(new SafepointTestTask(
-        isolate, &monitor, &expected_count, &total_done, &exited));
+    Dart::thread_pool()->Run<SafepointTestTask>(
+        isolate, &monitor, &expected_count, &total_done, &exited);
   }
 // Run Dart code on the main thread long enough to allow all helpers
 // to get their verification done and exit. Use a specific UserTag
@@ -681,8 +681,8 @@ ISOLATE_UNIT_TEST_CASE(SafepointTestVM) {
   intptr_t total_done = 0;
   intptr_t exited = 0;
   for (int i = 0; i < SafepointTestTask::kTaskCount; i++) {
-    Dart::thread_pool()->Run(new SafepointTestTask(
-        isolate, &monitor, &expected_count, &total_done, &exited));
+    Dart::thread_pool()->Run<SafepointTestTask>(
+        isolate, &monitor, &expected_count, &total_done, &exited);
   }
   String& label = String::Handle(String::New("foo"));
   UserTag& tag = UserTag::Handle(UserTag::New(label));
@@ -802,8 +802,8 @@ ISOLATE_UNIT_TEST_CASE(SafepointTestVM2) {
   intptr_t total_done = 0;
   intptr_t exited = 0;
   for (int i = 0; i < SafepointTestTask::kTaskCount; i++) {
-    Dart::thread_pool()->Run(new SafepointTestTask(
-        isolate, &monitor, &expected_count, &total_done, &exited));
+    Dart::thread_pool()->Run<SafepointTestTask>(
+        isolate, &monitor, &expected_count, &total_done, &exited);
   }
   bool all_helpers = false;
   do {
@@ -833,8 +833,8 @@ ISOLATE_UNIT_TEST_CASE(RecursiveSafepointTest2) {
   intptr_t total_done = 0;
   intptr_t exited = 0;
   for (int i = 0; i < SafepointTestTask::kTaskCount; i++) {
-    Dart::thread_pool()->Run(new SafepointTestTask(
-        isolate, &monitor, &expected_count, &total_done, &exited));
+    Dart::thread_pool()->Run<SafepointTestTask>(
+        isolate, &monitor, &expected_count, &total_done, &exited);
   }
   bool all_helpers = false;
   do {
@@ -899,7 +899,7 @@ ISOLATE_UNIT_TEST_CASE(HelperAllocAndGC) {
   Monitor done_monitor;
   bool done = false;
   Isolate* isolate = thread->isolate();
-  Dart::thread_pool()->Run(new AllocAndGCTask(isolate, &done_monitor, &done));
+  Dart::thread_pool()->Run<AllocAndGCTask>(isolate, &done_monitor, &done);
   {
     while (true) {
       TransitionVMToBlocked transition(thread);
@@ -951,8 +951,8 @@ ISOLATE_UNIT_TEST_CASE(ExerciseTLABs) {
   Isolate* isolate = thread->isolate();
   for (int i = 0; i < NUMBER_TEST_THREADS; i++) {
     done[i] = false;
-    Dart::thread_pool()->Run(
-        new AllocateGlobsOfMemoryTask(isolate, &done_monitor[i], &done[i]));
+    Dart::thread_pool()->Run<AllocateGlobsOfMemoryTask>(
+        isolate, &done_monitor[i], &done[i]);
   }
 
   for (int i = 0; i < NUMBER_TEST_THREADS; i++) {