Commit 8b8e044f authored by Shu-yu Guo's avatar Shu-yu Guo Committed by V8 LUCI CQ

[heap] Add Parking* variants of blocking primitives

Due to shared GCs it's easy to accidentally deadlock V8 by forgetting to
park a thread before blocking.

This CL does the following:

- Adds ParkingConditionVariable and ParkingSemaphore, which hide
the Wait[For] methods in favor of ParkedWait[For], which parks the
thread before blocking the thread.
- Migrate to the Parking* variants in JS shared memory tests.

Bug: v8:11708
Change-Id: I6d1b2b26a05e7df0a69a1614c03308f538a8782f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3708017Reviewed-by: 's avatarMichael Lippautz <mlippautz@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81225}
parent 35a6ac72
......@@ -33,7 +33,7 @@ class TimeDelta;
// the mutex and suspend the execution of the calling thread. When the condition
// variable is notified, the thread is awakened, and the mutex is reacquired.
class V8_BASE_EXPORT ConditionVariable final {
class V8_BASE_EXPORT ConditionVariable {
public:
ConditionVariable();
ConditionVariable(const ConditionVariable&) = delete;
......
......@@ -36,7 +36,7 @@ class TimeDelta;
// count reaches zero, threads waiting for the semaphore blocks until the
// count becomes non-zero.
class V8_BASE_EXPORT Semaphore final {
class V8_BASE_EXPORT Semaphore {
public:
explicit Semaphore(int count);
Semaphore(const Semaphore&) = delete;
......@@ -76,7 +76,6 @@ class V8_BASE_EXPORT Semaphore final {
NativeHandle native_handle_;
};
// POD Semaphore initialized lazily (i.e. the first time Pointer() is called).
// Usage:
// // The following semaphore starts at 0.
......
......@@ -5,7 +5,9 @@
#ifndef V8_HEAP_PARKED_SCOPE_H_
#define V8_HEAP_PARKED_SCOPE_H_
#include "src/base/platform/condition-variable.h"
#include "src/base/platform/mutex.h"
#include "src/base/platform/semaphore.h"
#include "src/execution/local-isolate.h"
#include "src/heap/local-heap.h"
......@@ -112,6 +114,68 @@ class V8_NODISCARD ParkedSharedMutexGuardIf final {
base::SharedMutex* mutex_ = nullptr;
};
// A subclass of base::ConditionVariable that automatically parks the thread
// while waiting.
class V8_NODISCARD ParkingConditionVariable final
: public base::ConditionVariable {
public:
ParkingConditionVariable(const ParkingConditionVariable&) = delete;
ParkingConditionVariable& operator=(const ParkingConditionVariable&) = delete;
void ParkedWait(LocalIsolate* local_isolate, base::Mutex* mutex) {
ParkedWait(local_isolate->heap(), mutex);
}
void ParkedWait(LocalHeap* local_heap, base::Mutex* mutex) {
ParkedScope scope(local_heap);
Wait(mutex);
}
bool ParkedWaitFor(LocalIsolate* local_isolate, base::Mutex* mutex,
const base::TimeDelta& rel_time) V8_WARN_UNUSED_RESULT {
return ParkedWaitFor(local_isolate->heap(), mutex, rel_time);
}
bool ParkedWaitFor(LocalHeap* local_heap, base::Mutex* mutex,
const base::TimeDelta& rel_time) V8_WARN_UNUSED_RESULT {
ParkedScope scope(local_heap);
return WaitFor(mutex, rel_time);
}
private:
using base::ConditionVariable::Wait;
using base::ConditionVariable::WaitFor;
};
// A subclass of base::Semaphore that automatically parks the thread while
// waiting.
class V8_NODISCARD ParkingSemaphore final : public base::Semaphore {
public:
explicit ParkingSemaphore(int count) : base::Semaphore(count) {}
ParkingSemaphore(const ParkingSemaphore&) = delete;
ParkingSemaphore& operator=(const ParkingSemaphore&) = delete;
void ParkedWait(LocalIsolate* local_isolate) {
ParkedWait(local_isolate->heap());
}
void ParkedWait(LocalHeap* local_heap) {
ParkedScope scope(local_heap);
Wait();
}
bool ParkedWaitFor(LocalIsolate* local_isolate,
const base::TimeDelta& rel_time) V8_WARN_UNUSED_RESULT {
return ParkedWaitFor(local_isolate->heap(), rel_time);
}
bool ParkedWaitFor(LocalHeap* local_heap,
const base::TimeDelta& rel_time) V8_WARN_UNUSED_RESULT {
ParkedScope scope(local_heap);
return WaitFor(rel_time);
}
private:
using base::Semaphore::Wait;
using base::Semaphore::WaitFor;
};
} // namespace internal
} // namespace v8
......
This diff is collapsed.
......@@ -3,8 +3,8 @@
// found in the LICENSE file.
#include "src/base/platform/platform.h"
#include "src/base/platform/semaphore.h"
#include "src/base/platform/time.h"
#include "src/heap/parked-scope.h"
#include "src/objects/js-atomics-synchronization-inl.h"
#include "test/unittests/test-utils.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -39,9 +39,9 @@ class ClientIsolateWithContextWrapper final {
class LockingThread final : public v8::base::Thread {
public:
LockingThread(v8::Isolate* shared_isolate, Handle<JSAtomicsMutex> mutex,
base::Semaphore* sema_ready,
base::Semaphore* sema_execute_start,
base::Semaphore* sema_execute_complete)
ParkingSemaphore* sema_ready,
ParkingSemaphore* sema_execute_start,
ParkingSemaphore* sema_execute_complete)
: Thread(Options("ThreadWithAtomicsMutex")),
shared_isolate_(shared_isolate),
mutex_(mutex),
......@@ -54,7 +54,7 @@ class LockingThread final : public v8::base::Thread {
Isolate* isolate = client_isolate_wrapper.isolate();
sema_ready_->Signal();
sema_execute_start_->Wait();
sema_execute_start_->ParkedWait(isolate->main_thread_local_isolate());
HandleScope scope(isolate);
JSAtomicsMutex::Lock(isolate, mutex_);
......@@ -66,12 +66,19 @@ class LockingThread final : public v8::base::Thread {
sema_execute_complete_->Signal();
}
void ParkedJoin(const ParkedScope& scope) {
USE(scope);
Join();
}
protected:
using base::Thread::Join;
v8::Isolate* shared_isolate_;
Handle<JSAtomicsMutex> mutex_;
base::Semaphore* sema_ready_;
base::Semaphore* sema_execute_start_;
base::Semaphore* sema_execute_complete_;
ParkingSemaphore* sema_ready_;
ParkingSemaphore* sema_execute_start_;
ParkingSemaphore* sema_execute_complete_;
};
} // namespace
......@@ -86,11 +93,12 @@ TEST_F(JSAtomicsMutexTest, Contention) {
constexpr int kThreads = 32;
Isolate* isolate = client_isolate_wrapper.isolate();
Handle<JSAtomicsMutex> contended_mutex =
JSAtomicsMutex::Create(client_isolate_wrapper.isolate());
base::Semaphore sema_ready(0);
base::Semaphore sema_execute_start(0);
base::Semaphore sema_execute_complete(0);
ParkingSemaphore sema_ready(0);
ParkingSemaphore sema_execute_start(0);
ParkingSemaphore sema_execute_complete(0);
std::vector<std::unique_ptr<LockingThread>> threads;
for (int i = 0; i < kThreads; i++) {
auto thread = std::make_unique<LockingThread>(
......@@ -100,12 +108,18 @@ TEST_F(JSAtomicsMutexTest, Contention) {
threads.push_back(std::move(thread));
}
for (int i = 0; i < kThreads; i++) sema_ready.Wait();
LocalIsolate* local_isolate = isolate->main_thread_local_isolate();
for (int i = 0; i < kThreads; i++) {
sema_ready.ParkedWait(local_isolate);
}
for (int i = 0; i < kThreads; i++) sema_execute_start.Signal();
for (int i = 0; i < kThreads; i++) sema_execute_complete.Wait();
for (int i = 0; i < kThreads; i++) {
sema_execute_complete.ParkedWait(local_isolate);
}
ParkedScope parked(local_isolate);
for (auto& thread : threads) {
thread->Join();
thread->ParkedJoin(parked);
}
EXPECT_FALSE(contended_mutex->IsHeld());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment