From 20a5e25ec24d7d6f22ddf7d1246eac2183b501df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Fri, 7 Apr 2023 09:41:09 +0200 Subject: [PATCH 1/5] deps: V8: cherry-pick 8f61b4133805 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [heap] Cache ForegroundTaskRunner in CollectionBarrier The API doesn't allow us to invoke GetForegroundTaskRunner() from a background thread. We need to cache the task runner in the ctor of CollectionBarrier during heap setup. We can't create the CollectionBarrier inside Heap's ctor anymore because at that point the TaskRunner might not be set up (e.g. Node). Bug: v8:13902 Change-Id: I814b9dbecb0ded673108bfb7b730e7c9338cf5f5 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4403395 Reviewed-by: Michael Lippautz Commit-Queue: Dominik Inführ Cr-Commit-Position: refs/heads/main@{#86981} Refs: https://github.com/v8/v8/commit/8f61b413380585dea97ead6b0547d97d35f420f9 --- common.gypi | 2 +- deps/v8/src/heap/collection-barrier.cc | 24 +++++++++++++----------- deps/v8/src/heap/collection-barrier.h | 8 ++++---- deps/v8/src/heap/heap.cc | 3 ++- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/common.gypi b/common.gypi index 0dcf428002c4c7..9b37dc9ff581a3 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.3', + 'v8_embedder_string': '-node.4', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/heap/collection-barrier.cc b/deps/v8/src/heap/collection-barrier.cc index 9486c234b0f9db..41dcf3a3b1257b 100644 --- a/deps/v8/src/heap/collection-barrier.cc +++ b/deps/v8/src/heap/collection-barrier.cc @@ -17,6 +17,11 @@ namespace v8 { namespace internal { +CollectionBarrier::CollectionBarrier(Heap* heap) + : heap_(heap), + foreground_task_runner_(V8::GetCurrentPlatform()->GetForegroundTaskRunner( + reinterpret_cast(heap->isolate()))) {} + bool CollectionBarrier::WasGCRequested() { return collection_requested_.load(); } @@ -95,7 +100,14 @@ bool CollectionBarrier::AwaitCollectionBackground(LocalHeap* local_heap) { } // The first thread needs to activate the stack guard and post the task. - if (first_thread) ActivateStackGuardAndPostTask(); + if (first_thread) { + Isolate* isolate = heap_->isolate(); + ExecutionAccess access(isolate); + isolate->stack_guard()->RequestGC(); + + foreground_task_runner_->PostTask( + std::make_unique(heap_)); + } ParkedScope scope(local_heap); base::MutexGuard guard(&mutex_); @@ -109,16 +121,6 @@ bool CollectionBarrier::AwaitCollectionBackground(LocalHeap* local_heap) { return collection_performed_; } -void CollectionBarrier::ActivateStackGuardAndPostTask() { - Isolate* isolate = heap_->isolate(); - ExecutionAccess access(isolate); - isolate->stack_guard()->RequestGC(); - - V8::GetCurrentPlatform() - ->GetForegroundTaskRunner(reinterpret_cast(isolate)) - ->PostTask(std::make_unique(heap_)); -} - void CollectionBarrier::StopTimeToCollectionTimer() { if (collection_requested_.load()) { base::MutexGuard guard(&mutex_); diff --git a/deps/v8/src/heap/collection-barrier.h b/deps/v8/src/heap/collection-barrier.h index fd894324a6fca8..6db89842594839 100644 --- a/deps/v8/src/heap/collection-barrier.h +++ b/deps/v8/src/heap/collection-barrier.h @@ -22,7 +22,7 @@ class Heap; // This class stops and resumes all background threads waiting for GC. class CollectionBarrier { public: - explicit CollectionBarrier(Heap* heap) : heap_(heap) {} + explicit CollectionBarrier(Heap* heap); // Returns true when collection was requested. bool WasGCRequested(); @@ -49,9 +49,6 @@ class CollectionBarrier { bool AwaitCollectionBackground(LocalHeap* local_heap); private: - // Activate stack guards and posting a task to perform the GC. - void ActivateStackGuardAndPostTask(); - Heap* heap_; base::Mutex mutex_; base::ConditionVariable cv_wakeup_; @@ -72,6 +69,9 @@ class CollectionBarrier { // Will be set as soon as Isolate starts tear down. bool shutdown_requested_ = false; + + // Used to post tasks on the main thread. + std::shared_ptr foreground_task_runner_; }; } // namespace internal diff --git a/deps/v8/src/heap/heap.cc b/deps/v8/src/heap/heap.cc index 90aa3360b4baa7..e4d1120f3b4654 100644 --- a/deps/v8/src/heap/heap.cc +++ b/deps/v8/src/heap/heap.cc @@ -283,7 +283,6 @@ Heap::Heap() allocation_type_for_in_place_internalizable_strings_( isolate()->OwnsStringTables() ? AllocationType::kOld : AllocationType::kSharedOld), - collection_barrier_(new CollectionBarrier(this)), marking_state_(isolate_), non_atomic_marking_state_(isolate_), atomic_marking_state_(isolate_), @@ -5431,6 +5430,8 @@ void Heap::SetUp(LocalHeap* main_thread_local_heap) { code_page_allocator = isolate_->page_allocator(); } + collection_barrier_.reset(new CollectionBarrier(this)); + // Set up memory allocator. memory_allocator_.reset( new MemoryAllocator(isolate_, code_page_allocator, MaxReserved())); From 8c5b8decc1ef110a9c771e8dd2990a3357d4ab7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Fri, 7 Apr 2023 09:41:16 +0200 Subject: [PATCH 2/5] deps: V8: cherry-pick f820e706a0b9 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [heap] Cache GetForegroundTaskRunner in IncrementalMarkingJob Our API doesn't allow us to invoke GetForegroundTaskRunner() from a background thread. We need to cache the task runner in the ctor of IncrementalMarkingJob during heap setup. This is necessary because we may start incremental marking from background threads. Bug: v8:13902 Change-Id: I938c9bf016e05145f1807c27fd477a7b6c859ca4 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4405689 Commit-Queue: Dominik Inführ Reviewed-by: Michael Lippautz Cr-Commit-Position: refs/heads/main@{#86983} Refs: https://github.com/v8/v8/commit/f820e706a0b9223135fdacf2ce2915a4eb6b412a --- common.gypi | 2 +- deps/v8/src/heap/incremental-marking-job.cc | 16 +++++++++------- deps/v8/src/heap/incremental-marking-job.h | 3 ++- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/common.gypi b/common.gypi index 9b37dc9ff581a3..f8d98fb492db42 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.4', + 'v8_embedder_string': '-node.5', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/heap/incremental-marking-job.cc b/deps/v8/src/heap/incremental-marking-job.cc index 82cc8fb0c2693b..762282183ce144 100644 --- a/deps/v8/src/heap/incremental-marking-job.cc +++ b/deps/v8/src/heap/incremental-marking-job.cc @@ -36,6 +36,11 @@ class IncrementalMarkingJob::Task : public CancelableTask { const StackState stack_state_; }; +IncrementalMarkingJob::IncrementalMarkingJob(Heap* heap) V8_NOEXCEPT + : heap_(heap), + foreground_task_runner_(V8::GetCurrentPlatform()->GetForegroundTaskRunner( + reinterpret_cast(heap->isolate()))) {} + void IncrementalMarkingJob::ScheduleTask() { base::MutexGuard guard(&mutex_); @@ -44,11 +49,8 @@ void IncrementalMarkingJob::ScheduleTask() { return; } - v8::Isolate* isolate = reinterpret_cast(heap_->isolate()); is_task_pending_ = true; - auto taskrunner = V8::GetCurrentPlatform()->GetForegroundTaskRunner(isolate); - - const auto stack_state = taskrunner->NonNestableTasksEnabled() + const auto stack_state = foreground_task_runner_->NonNestableTasksEnabled() ? StackState::kNoHeapPointers : StackState::kMayContainHeapPointers; @@ -56,10 +58,10 @@ void IncrementalMarkingJob::ScheduleTask() { scheduled_time_ = heap_->MonotonicallyIncreasingTimeInMs(); - if (taskrunner->NonNestableTasksEnabled()) { - taskrunner->PostNonNestableTask(std::move(task)); + if (foreground_task_runner_->NonNestableTasksEnabled()) { + foreground_task_runner_->PostNonNestableTask(std::move(task)); } else { - taskrunner->PostTask(std::move(task)); + foreground_task_runner_->PostTask(std::move(task)); } } diff --git a/deps/v8/src/heap/incremental-marking-job.h b/deps/v8/src/heap/incremental-marking-job.h index ca590bf1140890..a2436342ba7829 100644 --- a/deps/v8/src/heap/incremental-marking-job.h +++ b/deps/v8/src/heap/incremental-marking-job.h @@ -18,7 +18,7 @@ class Isolate; // step and posts another task until the marking is completed. class IncrementalMarkingJob final { public: - explicit IncrementalMarkingJob(Heap* heap) V8_NOEXCEPT : heap_(heap) {} + explicit IncrementalMarkingJob(Heap* heap) V8_NOEXCEPT; void ScheduleTask(); double CurrentTimeToTask() const; @@ -31,6 +31,7 @@ class IncrementalMarkingJob final { base::Mutex mutex_; double scheduled_time_ = 0.0; bool is_task_pending_ = false; + std::shared_ptr foreground_task_runner_; }; } // namespace internal } // namespace v8 From 776a1cf64930fe35fce1eed0324abb964427ef21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Inf=C3=BChr?= Date: Thu, 6 Apr 2023 15:37:12 +0200 Subject: [PATCH 3/5] src: do not drain worker tasks on main thread --- src/node_platform.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_platform.cc b/src/node_platform.cc index 74ab4a3df8f09b..1c1f5b2a3d0f31 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -451,10 +451,10 @@ void NodePlatform::DrainTasks(Isolate* isolate) { std::shared_ptr per_isolate = ForNodeIsolate(isolate); if (!per_isolate) return; - do { - // Worker tasks aren't associated with an Isolate. - worker_thread_task_runner_->BlockingDrain(); - } while (per_isolate->FlushForegroundTasksInternal()); + // Drain foreground tasks but not worker tasks as this may cause deadlocks + // and v8::Isolate::Dispose will join V8's worker tasks for that isolate. + while (per_isolate->FlushForegroundTasksInternal()) { + } } bool PerIsolatePlatformData::FlushForegroundTasksInternal() { From 067a20d9ef3412cb18e12cf32d7d5872614b6bc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 6 Apr 2023 15:06:21 +0200 Subject: [PATCH 4/5] Revert "test: skip test-wasm-web-api on ARM" This reverts commit 65b79aba0f29c1f657ebddf57e5fb5c26fa4464a. --- test/parallel/parallel.status | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status index 2c105b230fae0b..363cea8f65fb14 100644 --- a/test/parallel/parallel.status +++ b/test/parallel/parallel.status @@ -32,8 +32,6 @@ test-http-server-request-timeouts-mixed: PASS,FLAKY # https://github.com/nodejs/node/pull/31178 test-crypto-dh-stateless: SKIP test-crypto-keygen: SKIP -# https://github.com/nodejs/node/issues/47297 -test-wasm-web-api: SKIP [$system==solaris] # Also applies to SmartOS # https://github.com/nodejs/node/issues/43457 From 81b1a4147803285b6dec72d45e04a990bc5bd8cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 6 Apr 2023 15:06:54 +0200 Subject: [PATCH 5/5] Revert "test: skip instantiateStreaming-bad-imports WPT" This reverts commit 3cf65bd2688d35152accaca4a07df4ec603bdf9e. --- test/wpt/status/wasm/webapi.json | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/wpt/status/wasm/webapi.json b/test/wpt/status/wasm/webapi.json index 3e2075655e5af2..6328e55dc18bc9 100644 --- a/test/wpt/status/wasm/webapi.json +++ b/test/wpt/status/wasm/webapi.json @@ -16,8 +16,5 @@ }, "status.any.js": { "skip": "WPTRunner does not support fetch()" - }, - "instantiateStreaming-bad-imports.any.js": { - "skip": "Flaky on ARM with V8 >= 11.2" } }