cppgc: Rely on per-heap platform objects

Split platform into a process-global initialization part and per-heap
platform objects.

These platform objects still contain allocators and executors. With
per-heap platforms GetForegroundTaskRunner() returns by definition the
correct runner.

In future, when initialized throuhg V8, an adapter can be used to
translate between the different platforms, avoiding the needed for V8
embedders to provide additional information.

Bug: chromium:1056170
Change-Id: I11bdd15e945687cfbdf38cae4137facb02559e0a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2218030
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68059}

include/cppgc/heap.h

@@ -10,6 +10,7 @@
 
 #include "cppgc/common.h"
 #include "cppgc/custom-space.h"
+#include "cppgc/platform.h"
 #include "v8config.h"  // NOLINT(build/include_directory)
 
 /**
@@ -51,10 +52,12 @@ class V8_EXPORT Heap {
   /**
    * Creates a new heap that can be used for object allocation.
    *
+   * \param platform implemented and provided by the embedder.
    * \param options HeapOptions specifying various properties for the Heap.
    * \returns a new Heap instance.
    */
   static std::unique_ptr<Heap> Create(
+      std::shared_ptr<Platform> platform,
       HeapOptions options = HeapOptions::Default());
 
   virtual ~Heap() = default;

include/cppgc/platform.h

@@ -10,19 +10,112 @@
 
 namespace cppgc {
 
-// TODO(v8:10346): Put PageAllocator and Platform in a non-V8 include header to
-// avoid depending on namespace v8.
+// TODO(v8:10346): Create separate includes for concepts that are not
+// V8-specific.
+using JobHandle = v8::JobHandle;
+using JobTask = v8::JobTask;
 using PageAllocator = v8::PageAllocator;
-using Platform = v8::Platform;
+using TaskPriority = v8::TaskPriority;
+using TaskRunner = v8::TaskRunner;
 
-// Initializes the garbage collector with the provided platform. Must be called
-// before creating a Heap.
-V8_EXPORT void InitializePlatform(Platform* platform);
+/**
+ * Platform interface used by Heap. Contains allocators and executors.
+ */
+class V8_EXPORT Platform {
+ public:
+  virtual ~Platform() = default;
 
-V8_EXPORT Platform* GetPlatform();
+  /**
+   * Returns the allocator used by cppgc to allocate its heap and various
+   * support structures.
+   */
+  virtual PageAllocator* GetPageAllocator() = 0;
 
-// Must be called after destroying the last used heap.
-V8_EXPORT void ShutdownPlatform();
+  /**
+   * Monotonically increasing time in seconds from an arbitrary fixed point in
+   * the past. This function is expected to return at least
+   * millisecond-precision values. For this reason,
+   * it is recommended that the fixed point be no further in the past than
+   * the epoch.
+   **/
+  virtual double MonotonicallyIncreasingTime() = 0;
+
+  /**
+   * Foreground task runner that should be used by a Heap.
+   */
+  virtual std::shared_ptr<TaskRunner> GetForegroundTaskRunner() {
+    return nullptr;
+  }
+
+  /**
+   * Posts |job_task| to run in parallel. Returns a JobHandle associated with
+   * the Job, which can be joined or canceled.
+   * This avoids degenerate cases:
+   * - Calling CallOnWorkerThread() for each work item, causing significant
+   *   overhead.
+   * - Fixed number of CallOnWorkerThread() calls that split the work and might
+   *   run for a long time. This is problematic when many components post
+   *   "num cores" tasks and all expect to use all the cores. In these cases,
+   *   the scheduler lacks context to be fair to multiple same-priority requests
+   *   and/or ability to request lower priority work to yield when high priority
+   *   work comes in.
+   * A canonical implementation of |job_task| looks like:
+   * class MyJobTask : public JobTask {
+   *  public:
+   *   MyJobTask(...) : worker_queue_(...) {}
+   *   // JobTask:
+   *   void Run(JobDelegate* delegate) override {
+   *     while (!delegate->ShouldYield()) {
+   *       // Smallest unit of work.
+   *       auto work_item = worker_queue_.TakeWorkItem(); // Thread safe.
+   *       if (!work_item) return;
+   *       ProcessWork(work_item);
+   *     }
+   *   }
+   *
+   *   size_t GetMaxConcurrency() const override {
+   *     return worker_queue_.GetSize(); // Thread safe.
+   *   }
+   * };
+   * auto handle = PostJob(TaskPriority::kUserVisible,
+   *                       std::make_unique<MyJobTask>(...));
+   * handle->Join();
+   *
+   * PostJob() and methods of the returned JobHandle/JobDelegate, must never be
+   * called while holding a lock that could be acquired by JobTask::Run or
+   * JobTask::GetMaxConcurrency -- that could result in a deadlock. This is
+   * because [1] JobTask::GetMaxConcurrency may be invoked while holding
+   * internal lock (A), hence JobTask::GetMaxConcurrency can only use a lock (B)
+   * if that lock is *never* held while calling back into JobHandle from any
+   * thread (A=>B/B=>A deadlock) and [2] JobTask::Run or
+   * JobTask::GetMaxConcurrency may be invoked synchronously from JobHandle
+   * (B=>JobHandle::foo=>B deadlock).
+   *
+   * A sufficient PostJob() implementation that uses the default Job provided in
+   * libplatform looks like:
+   *  std::unique_ptr<JobHandle> PostJob(
+   *      TaskPriority priority, std::unique_ptr<JobTask> job_task) override {
+   *    return std::make_unique<DefaultJobHandle>(
+   *        std::make_shared<DefaultJobState>(
+   *            this, std::move(job_task), kNumThreads));
+   * }
+   */
+  virtual std::unique_ptr<JobHandle> PostJob(
+      TaskPriority priority, std::unique_ptr<JobTask> job_task) {
+    return nullptr;
+  }
+};
+
+/**
+ * Process-global initialization of the garbage collector. Must be called before
+ * creating a Heap.
+ */
+V8_EXPORT void InitializeProcess(PageAllocator*);
+
+/**
+ * Must be called after destroying the last used heap.
+ */
+V8_EXPORT void ShutdownProcess();
 
 namespace internal {
 

src/heap/cppgc/heap.cc

@@ -31,9 +31,12 @@ void VerifyCustomSpaces(
 
 }  // namespace
 
-std::unique_ptr<Heap> Heap::Create(cppgc::Heap::HeapOptions options) {
+std::unique_ptr<Heap> Heap::Create(std::shared_ptr<cppgc::Platform> platform,
+                                   cppgc::Heap::HeapOptions options) {
+  DCHECK(platform.get());
   VerifyCustomSpaces(options.custom_spaces);
-  return std::make_unique<internal::Heap>(options.custom_spaces.size());
+  return std::make_unique<internal::Heap>(std::move(platform),
+                                          options.custom_spaces.size());
 }
 
 void Heap::ForceGarbageCollectionSlow(const char* source, const char* reason,
@@ -81,11 +84,13 @@ cppgc::LivenessBroker LivenessBrokerFactory::Create() {
   return cppgc::LivenessBroker();
 }
 
-Heap::Heap(size_t custom_spaces)
+Heap::Heap(std::shared_ptr<cppgc::Platform> platform, size_t custom_spaces)
     : raw_heap_(this, custom_spaces),
-      page_backend_(std::make_unique<PageBackend>(&system_allocator_)),
+      platform_(std::move(platform)),
+      page_backend_(
+          std::make_unique<PageBackend>(platform_->GetPageAllocator())),
       object_allocator_(&raw_heap_),
-      sweeper_(&raw_heap_),
+      sweeper_(&raw_heap_, platform_.get()),
       stack_(std::make_unique<Stack>(v8::base::Stack::GetStackStart())),
       prefinalizer_handler_(std::make_unique<PreFinalizerHandler>()) {}
 

src/heap/cppgc/heap.h

@@ -80,7 +80,8 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
 
   static Heap* From(cppgc::Heap* heap) { return static_cast<Heap*>(heap); }
 
-  explicit Heap(size_t custom_spaces);
+  explicit Heap(std::shared_ptr<cppgc::Platform> platform,
+                size_t custom_spaces);
   ~Heap() final;
 
   inline void* Allocate(size_t size, GCInfoIndex index);
@@ -126,7 +127,7 @@ class V8_EXPORT_PRIVATE Heap final : public cppgc::Heap {
 
   RawHeap raw_heap_;
 
-  v8::base::PageAllocator system_allocator_;
+  std::shared_ptr<cppgc::Platform> platform_;
   std::unique_ptr<PageBackend> page_backend_;
   ObjectAllocator object_allocator_;
   Sweeper sweeper_;

src/heap/cppgc/platform.cc

@@ -8,20 +8,12 @@
 #include "src/heap/cppgc/gc-info-table.h"
 
 namespace cppgc {
-namespace internal {
 
-static Platform* g_platform;
-
-}  // namespace internal
-
-void InitializePlatform(Platform* platform) {
-  internal::g_platform = platform;
-  internal::GlobalGCInfoTable::Create(internal::g_platform->GetPageAllocator());
+void InitializeProcess(PageAllocator* page_allocator) {
+  internal::GlobalGCInfoTable::Create(page_allocator);
 }
 
-Platform* GetPlatform() { return internal::g_platform; }
-
-void ShutdownPlatform() { internal::g_platform = nullptr; }
+void ShutdownProcess() {}
 
 namespace internal {
 

src/heap/cppgc/sweeper.cc

@@ -225,7 +225,7 @@ typename FinalizationBuilder::ResultType SweepNormalPage(NormalPage* page) {
 // - merges freelists to the space's freelist.
 class SweepFinalizer final {
  public:
-  explicit SweepFinalizer(v8::Platform* platform) : platform_(platform) {}
+  explicit SweepFinalizer(cppgc::Platform* platform) : platform_(platform) {}
 
   void FinalizeHeap(SpaceStates* space_states) {
     for (SpaceState& space_state : *space_states) {
@@ -292,14 +292,14 @@ class SweepFinalizer final {
   }
 
  private:
-  v8::Platform* platform_;
+  cppgc::Platform* platform_;
 };
 
 class MutatorThreadSweeper final : private HeapVisitor<MutatorThreadSweeper> {
   friend class HeapVisitor<MutatorThreadSweeper>;
 
  public:
-  explicit MutatorThreadSweeper(SpaceStates* states, v8::Platform* platform)
+  explicit MutatorThreadSweeper(SpaceStates* states, cppgc::Platform* platform)
       : states_(states), platform_(platform) {}
 
   void Sweep() {
@@ -374,7 +374,7 @@ class MutatorThreadSweeper final : private HeapVisitor<MutatorThreadSweeper> {
   }
 
   SpaceStates* states_;
-  v8::Platform* platform_;
+  cppgc::Platform* platform_;
 };
 
 class ConcurrentSweepTask final : public v8::JobTask,
@@ -466,13 +466,11 @@ class PrepareForSweepVisitor final
 
 class Sweeper::SweeperImpl final {
  public:
-  explicit SweeperImpl(RawHeap* heap)
+  explicit SweeperImpl(RawHeap* heap, cppgc::Platform* platform)
       : heap_(heap),
         space_states_(heap->size()),
-        platform_(cppgc::GetPlatform()),
-        foreground_task_runner_(platform_->GetForegroundTaskRunner(nullptr)) {
-    // TODO(chromium:1056170): support Isolate independent task runner.
-  }
+        platform_(platform),
+        foreground_task_runner_(platform_->GetForegroundTaskRunner()) {}
 
   ~SweeperImpl() { CancelSweepers(); }
 
@@ -600,14 +598,15 @@ class Sweeper::SweeperImpl final {
 
   RawHeap* heap_;
   SpaceStates space_states_;
-  v8::Platform* platform_;
+  cppgc::Platform* platform_;
   std::shared_ptr<v8::TaskRunner> foreground_task_runner_;
   IncrementalSweepTask::Handle incremental_sweeper_handle_;
   std::unique_ptr<v8::JobHandle> concurrent_sweeper_handle_;
   bool is_in_progress_ = false;
 };
 
-Sweeper::Sweeper(RawHeap* heap) : impl_(std::make_unique<SweeperImpl>(heap)) {}
+Sweeper::Sweeper(RawHeap* heap, cppgc::Platform* platform)
+    : impl_(std::make_unique<SweeperImpl>(heap, platform)) {}
 
 Sweeper::~Sweeper() = default;
 

src/heap/cppgc/sweeper.h

@@ -10,6 +10,9 @@
 #include "src/base/macros.h"
 
 namespace cppgc {
+
+class Platform;
+
 namespace internal {
 
 class RawHeap;
@@ -18,7 +21,7 @@ class V8_EXPORT_PRIVATE Sweeper final {
  public:
   enum class Config { kAtomic, kIncrementalAndConcurrent };
 
-  explicit Sweeper(RawHeap*);
+  explicit Sweeper(RawHeap*, cppgc::Platform*);
   ~Sweeper();
 
   Sweeper(const Sweeper&) = delete;

src/heap/sweeper.h

@@ -47,7 +47,7 @@ class Sweeper {
   // after exiting this scope.
   class FilterSweepingPagesScope final {
    public:
-    explicit FilterSweepingPagesScope(
+    FilterSweepingPagesScope(
         Sweeper* sweeper, const PauseOrCompleteScope& pause_or_complete_scope);
     ~FilterSweepingPagesScope();
 

test/unittests/heap/cppgc/concurrent-sweeper-unittest.cc

@@ -275,7 +275,7 @@ TEST_F(ConcurrentSweeperTest, IncrementalSweeping) {
   testing::TestPlatform::DisableBackgroundTasksScope disable_concurrent_sweeper(
       &GetPlatform());
 
-  auto task_runner = GetPlatform().GetForegroundTaskRunner(nullptr);
+  auto task_runner = GetPlatform().GetForegroundTaskRunner();
 
   // Create two unmarked objects.
   MakeGarbageCollected<NormalFinalizable>(GetHeap());

test/unittests/heap/cppgc/custom-spaces-unittest.cc

@@ -32,7 +32,7 @@ class TestWithHeapWithCustomSpaces : public testing::TestWithPlatform {
     Heap::HeapOptions options;
     options.custom_spaces.emplace_back(std::make_unique<CustomSpace1>());
     options.custom_spaces.emplace_back(std::make_unique<CustomSpace2>());
-    heap_ = Heap::Create(std::move(options));
+    heap_ = Heap::Create(platform_, std::move(options));
     g_destructor_callcount = 0;
   }
 

test/unittests/heap/cppgc/test-platform.cc

@@ -84,20 +84,6 @@ TestPlatform::TestPlatform()
 
 TestPlatform::~TestPlatform() V8_NOEXCEPT { WaitAllBackgroundTasks(); }
 
-void TestPlatform::CallOnWorkerThread(std::unique_ptr<v8::Task> task) {
-  if (AreBackgroundTasksDisabled()) return;
-
-  auto thread = std::make_unique<WorkerThread>(std::move(task));
-  if (thread->Start()) {
-    threads_.push_back(std::move(thread));
-  }
-}
-
-void TestPlatform::CallDelayedOnWorkerThread(std::unique_ptr<v8::Task> task,
-                                             double) {
-  CallOnWorkerThread(std::move(task));
-}
-
 std::unique_ptr<v8::JobHandle> TestPlatform::PostJob(
     v8::TaskPriority, std::unique_ptr<v8::JobTask> job_task) {
   if (AreBackgroundTasksDisabled()) return {};
@@ -112,23 +98,15 @@ double TestPlatform::MonotonicallyIncreasingTime() {
          static_cast<double>(v8::base::Time::kMicrosecondsPerSecond);
 }
 
-double TestPlatform::CurrentClockTimeMillis() {
-  return v8::base::OS::TimeCurrentMillis();
-}
-
 void TestPlatform::WaitAllForegroundTasks() {
   foreground_task_runner_->RunUntilIdle();
 }
 
 void TestPlatform::WaitAllBackgroundTasks() {
-  for (auto& thread : threads_) {
-    thread->Join();
-  }
-  threads_.clear();
   for (auto& thread : job_threads_) {
     thread->Join();
   }
-  threads_.clear();
+  job_threads_.clear();
 }
 
 TestPlatform::DisableBackgroundTasksScope::DisableBackgroundTasksScope(

test/unittests/heap/cppgc/test-platform.h

@@ -53,30 +53,14 @@ class TestPlatform : public Platform {
 
   PageAllocator* GetPageAllocator() override { return &page_allocator_; }
 
-  int NumberOfWorkerThreads() override {
-    return static_cast<int>(threads_.size());
-  }
-
-  std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner(
-      v8::Isolate*) override {
+  std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner() override {
     return foreground_task_runner_;
   }
 
-  void CallOnWorkerThread(std::unique_ptr<v8::Task> task) override;
-  void CallDelayedOnWorkerThread(std::unique_ptr<v8::Task> task,
-                                 double) override;
-
-  bool IdleTasksEnabled(v8::Isolate*) override { return true; }
-
   std::unique_ptr<v8::JobHandle> PostJob(
       v8::TaskPriority, std::unique_ptr<v8::JobTask> job_task) override;
 
   double MonotonicallyIncreasingTime() override;
-  double CurrentClockTimeMillis() override;
-
-  v8::TracingController* GetTracingController() override {
-    return &tracing_controller_;
-  }
 
   void WaitAllForegroundTasks();
   void WaitAllBackgroundTasks();
@@ -122,9 +106,7 @@ class TestPlatform : public Platform {
 
   v8::base::PageAllocator page_allocator_;
   std::shared_ptr<TestTaskRunner> foreground_task_runner_;
-  std::vector<std::unique_ptr<WorkerThread>> threads_;
   std::vector<std::shared_ptr<JobThread>> job_threads_;
-  v8::TracingController tracing_controller_;
   size_t disabled_background_tasks_ = 0;
 };
 

test/unittests/heap/cppgc/tests.cc

@@ -13,21 +13,21 @@ namespace internal {
 namespace testing {
 
 // static
-std::unique_ptr<TestPlatform> TestWithPlatform::platform_;
+std::shared_ptr<TestPlatform> TestWithPlatform::platform_;
 
 // static
 void TestWithPlatform::SetUpTestSuite() {
   platform_ = std::make_unique<TestPlatform>();
-  cppgc::InitializePlatform(platform_.get());
+  cppgc::InitializeProcess(platform_->GetPageAllocator());
 }
 
 // static
 void TestWithPlatform::TearDownTestSuite() {
-  cppgc::ShutdownPlatform();
+  cppgc::ShutdownProcess();
   platform_.reset();
 }
 
-TestWithHeap::TestWithHeap() : heap_(Heap::Create()) {}
+TestWithHeap::TestWithHeap() : heap_(Heap::Create(platform_)) {}
 
 TestSupportingAllocationOnly::TestSupportingAllocationOnly()
     : no_gc_scope_(internal::Heap::From(GetHeap())) {}

test/unittests/heap/cppgc/tests.h

@@ -22,8 +22,8 @@ class TestWithPlatform : public ::testing::Test {
 
   TestPlatform& GetPlatform() const { return *platform_; }
 
- private:
-  static std::unique_ptr<TestPlatform> platform_;
+ protected:
+  static std::shared_ptr<TestPlatform> platform_;
 };
 
 class TestWithHeap : public TestWithPlatform {