Skip to content

Commit

Permalink
Revert "[shared-struct] Add Mutex.lockAsync and Condition.waitAsync A…
Browse files Browse the repository at this point in the history 
…PIs."

This reverts commit 975dabc.

Reason for revert: Failed on V8 Linux64 TSAN - debug https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20TSAN%20-%20debug/5680/overview

Original change's description:
> [shared-struct] Add Mutex.lockAsync and Condition.waitAsync APIs.
>
> This CL add support for Atomics.Mutex.asyncLock(mutex, callback), its
> return value is always a promise.
> If callback's result is not a promise, then the mutex is released and
> the return promise is resolved once the callback finished execution.
> If callback's result is a promise, then the mutex is released and the
> return promise is resolved once the callback promise is resolved.
> The value the result promise passes to its handlers is an object
> {value: 'callback_result', success: boolean}.
>
> This CL also adds support for Atomics.Condition.asyncWait. Its return
> value is a promise which will be resolved when the corresponding
> condition variable is notified.
>
> Both API's support timeouts.
>
> Bug: v8:12547
> Change-Id: I95eaa3f78781211b2fb69dc89da98b0214dae892
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4873220
> Commit-Queue: Luis Fernando Pardo Sixtos <lpardosixtos@microsoft.com>
> Reviewed-by: Shu-yu Guo <syg@chromium.org>
> Reviewed-by: Adam Klein <adamk@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#93665}

Bug: v8:12547
Change-Id: Idd61303b6fbfded38ef9a9fba266391cedf2e823
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5506864
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Rezvan Mahdavi Hezaveh <rezvan@chromium.org>
Owners-Override: Rezvan Mahdavi Hezaveh <rezvan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#93667}
  • Loading branch information
@rmahdav
rmahdav authored and V8 LUCI CQ committed May 1, 2024
1 parent af484aa commit 04c7ba1
Show file tree
Hide file tree
Showing 26 changed files with 198 additions and 2,090 deletions.
153 changes: 14 additions & 139 deletions src/builtins/builtins-atomics-synchronization.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@

#include "src/builtins/builtins-utils-inl.h"
#include "src/objects/js-atomics-synchronization-inl.h"
#include "src/objects/promise-inl.h"

namespace v8 {
namespace internal {
namespace {

base::Optional<base::TimeDelta> GetTimeoutDelta(Handle<Object> timeout_obj) {
double ms = Object::Number(*timeout_obj);
if (!std::isnan(ms)) {
Expand All @@ -21,24 +19,19 @@ base::Optional<base::TimeDelta> GetTimeoutDelta(Handle<Object> timeout_obj) {
return base::nullopt;
}

Handle<JSPromise> UnlockAsyncLockedMutexFromPromiseHandler(Isolate* isolate) {
Handle<Context> context = Handle<Context>(isolate->context(), isolate);
Handle<Object> mutex = Handle<Object>(
context->get(JSAtomicsMutex::kMutexAsyncContextSlot), isolate);
Handle<Object> unlock_promise = Handle<Object>(
context->get(JSAtomicsMutex::kUnlockedPromiseAsyncContextSlot), isolate);
Handle<Object> waiter_wrapper_obj = Handle<Object>(
context->get(JSAtomicsMutex::kAsyncLockedWaiterAsyncContextSlot),
isolate);

Handle<JSAtomicsMutex> js_mutex = Handle<JSAtomicsMutex>::cast(mutex);
Handle<JSPromise> js_unlock_promise = Handle<JSPromise>::cast(unlock_promise);
Handle<Foreign> async_locked_waiter_wrapper =
Handle<Foreign>::cast(waiter_wrapper_obj);
js_mutex->UnlockAsyncLockedMutex(isolate, async_locked_waiter_wrapper);
return js_unlock_promise;
// TODO(lpardosixtos): Consider making and caching a canonical map for this
// result object, like we do for the iterator result object.
Handle<JSObject> CreateResultObject(Isolate* isolate, Handle<Object> value,
bool success) {
Handle<JSObject> result =
isolate->factory()->NewJSObject(isolate->object_function());
Handle<Object> success_value = isolate->factory()->ToBoolean(success);
JSObject::AddProperty(isolate, result, "value", value,
PropertyAttributes::NONE);
JSObject::AddProperty(isolate, result, "success", success_value,
PropertyAttributes::NONE);
return result;
}

} // namespace

BUILTIN(AtomicsMutexConstructor) {
Expand Down Expand Up @@ -124,7 +117,7 @@ BUILTIN(AtomicsMutexTryLock) {
}
}
Handle<JSObject> result =
JSAtomicsMutex::CreateResultObject(isolate, callback_result, success);
CreateResultObject(isolate, callback_result, success);
return *result;
}

Expand Down Expand Up @@ -183,74 +176,10 @@ BUILTIN(AtomicsMutexLockWithTimeout) {
}
}
Handle<JSObject> result =
JSAtomicsMutex::CreateResultObject(isolate, callback_result, success);
CreateResultObject(isolate, callback_result, success);
return *result;
}

BUILTIN(AtomicsMutexLockAsync) {
DCHECK(v8_flags.harmony_struct);
constexpr char method_name[] = "Atomics.Mutex.lockAsync";
HandleScope scope(isolate);

Handle<Object> js_mutex_obj = args.atOrUndefined(isolate, 1);
if (!IsJSAtomicsMutex(*js_mutex_obj)) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kMethodInvokedOnWrongType,
isolate->factory()->NewStringFromAsciiChecked(
method_name)));
}
Handle<JSAtomicsMutex> js_mutex = Handle<JSAtomicsMutex>::cast(js_mutex_obj);
Handle<Object> run_under_lock = args.atOrUndefined(isolate, 2);
if (!IsCallable(*run_under_lock)) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kNotCallable, run_under_lock));
}

Handle<Object> timeout_obj = args.atOrUndefined(isolate, 3);
base::Optional<base::TimeDelta> timeout = base::nullopt;
if (!IsUndefined(*timeout_obj, isolate)) {
if (!IsNumber(*timeout_obj)) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kIsNotNumber, timeout_obj,
Object::TypeOf(isolate, timeout_obj)));
}
timeout = GetTimeoutDelta(timeout_obj);
}

Handle<JSPromise> result_promise = JSAtomicsMutex::LockOrEnqueuePromise(
isolate, js_mutex, run_under_lock, timeout);

return *result_promise;
}

BUILTIN(AtomicsMutexAsyncUnlockResolveHandler) {
DCHECK(v8_flags.harmony_struct);
HandleScope scope(isolate);

Handle<Object> previous_result = args.atOrUndefined(isolate, 1);
Handle<JSPromise> js_unlock_promise =
UnlockAsyncLockedMutexFromPromiseHandler(isolate);

Handle<JSObject> result =
JSAtomicsMutex::CreateResultObject(isolate, previous_result, true);
auto resolve_result = JSPromise::Resolve(js_unlock_promise, result);
USE(resolve_result);
return *isolate->factory()->undefined_value();
}

BUILTIN(AtomicsMutexAsyncUnlockRejectHandler) {
DCHECK(v8_flags.harmony_struct);
HandleScope scope(isolate);

Handle<Object> error = args.atOrUndefined(isolate, 1);
Handle<JSPromise> js_unlock_promise =
UnlockAsyncLockedMutexFromPromiseHandler(isolate);

auto reject_result = JSPromise::Reject(js_unlock_promise, error);
USE(reject_result);
return *isolate->factory()->undefined_value();
}

BUILTIN(AtomicsConditionConstructor) {
DCHECK(v8_flags.harmony_struct);
HandleScope scope(isolate);
Expand Down Expand Up @@ -339,59 +268,5 @@ BUILTIN(AtomicsConditionNotify) {
JSAtomicsCondition::Notify(isolate, js_condition, count));
}

BUILTIN(AtomicsConditionWaitAsync) {
DCHECK(v8_flags.harmony_struct);
constexpr char method_name[] = "Atomics.Condition.waitAsync";
HandleScope scope(isolate);

Handle<Object> js_condition_obj = args.atOrUndefined(isolate, 1);
Handle<Object> js_mutex_obj = args.atOrUndefined(isolate, 2);
if (!IsJSAtomicsCondition(*js_condition_obj) ||
!IsJSAtomicsMutex(*js_mutex_obj)) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kMethodInvokedOnWrongType,
isolate->factory()->NewStringFromAsciiChecked(
method_name)));
}

Handle<Object> timeout_obj = args.atOrUndefined(isolate, 3);
base::Optional<base::TimeDelta> timeout = base::nullopt;
if (!IsUndefined(*timeout_obj, isolate)) {
if (!IsNumber(*timeout_obj)) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kIsNotNumber, timeout_obj,
Object::TypeOf(isolate, timeout_obj)));
}
timeout = GetTimeoutDelta(timeout_obj);
}

Handle<JSAtomicsCondition> js_condition =
Handle<JSAtomicsCondition>::cast(js_condition_obj);
Handle<JSAtomicsMutex> js_mutex = Handle<JSAtomicsMutex>::cast(js_mutex_obj);

if (!js_mutex->IsCurrentThreadOwner()) {
THROW_NEW_ERROR_RETURN_FAILURE(
isolate,
NewTypeError(MessageTemplate::kAtomicsMutexNotOwnedByCurrentThread));
}

Handle<JSPromise> result_promise =
JSAtomicsCondition::WaitAsync(isolate, js_condition, js_mutex, timeout);
return *result_promise;
}

BUILTIN(AtomicsConditionAcquireLock) {
DCHECK(v8_flags.harmony_struct);
HandleScope scope(isolate);

Handle<Context> context = Handle<Context>(isolate->context(), isolate);
Handle<Object> js_mutex_obj = Handle<Object>(
context->get(JSAtomicsCondition::kMutexAsyncContextSlot), isolate);
Handle<JSAtomicsMutex> js_mutex = Handle<JSAtomicsMutex>::cast(js_mutex_obj);
Handle<JSPromise> lock_promise =
JSAtomicsMutex::LockAsyncWrapperForWait(isolate, js_mutex);
return *lock_promise;
}

} // namespace internal
} // namespace v8
5 changes: 0 additions & 5 deletions src/builtins/builtins-definitions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1054,17 +1054,12 @@ namespace internal {
CPP(AtomicsMutexConstructor) \
CPP(AtomicsMutexIsMutex) \
CPP(AtomicsMutexLock) \
CPP(AtomicsMutexLockAsync) \
CPP(AtomicsMutexLockWithTimeout) \
CPP(AtomicsMutexTryLock) \
CPP(AtomicsMutexAsyncUnlockResolveHandler) \
CPP(AtomicsMutexAsyncUnlockRejectHandler) \
CPP(AtomicsConditionConstructor) \
CPP(AtomicsConditionAcquireLock) \
CPP(AtomicsConditionIsCondition) \
CPP(AtomicsConditionWait) \
CPP(AtomicsConditionNotify) \
CPP(AtomicsConditionWaitAsync) \
\
/* AsyncGenerator */ \
\
Expand Down
12 changes: 0 additions & 12 deletions src/execution/isolate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@
#include "src/objects/instance-type-inl.h"
#include "src/objects/js-array-buffer-inl.h"
#include "src/objects/js-array-inl.h"
#include "src/objects/js-atomics-synchronization-inl.h"
#include "src/objects/js-generator-inl.h"
#include "src/objects/js-struct-inl.h"
#include "src/objects/js-weak-refs-inl.h"
Expand All @@ -100,7 +99,6 @@
#include "src/objects/source-text-module-inl.h"
#include "src/objects/string-set-inl.h"
#include "src/objects/visitors.h"
#include "src/objects/waiter-queue-node.h"
#include "src/profiler/heap-profiler.h"
#include "src/profiler/tracing-cpu-profiler.h"
#include "src/regexp/regexp-stack.h"
Expand Down Expand Up @@ -4121,11 +4119,6 @@ void Isolate::Deinit() {
recorder_context_id_map_.clear();

FutexEmulation::IsolateDeinit(this);
if (v8_flags.harmony_struct) {
JSSynchronizationPrimitive::IsolateDeinit(this);
} else {
DCHECK(async_waiter_queue_nodes_.empty());
}

debug()->Unload();

Expand Down Expand Up @@ -7001,11 +6994,6 @@ Tagged<Object> Isolate::LocalsBlockListCacheGet(Handle<ScopeInfo> scope_info) {
return maybe_value;
}

std::list<std::unique_ptr<detail::WaiterQueueNode>>&
Isolate::async_waiter_queue_nodes() {
return async_waiter_queue_nodes_;
}

void DefaultWasmAsyncResolvePromiseCallback(
v8::Isolate* isolate, v8::Local<v8::Context> context,
v8::Local<v8::Promise::Resolver> resolver, v8::Local<v8::Value> result,
Expand Down
12 changes: 0 additions & 12 deletions src/execution/isolate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <atomic>
#include <cstddef>
#include <functional>
#include <list>
#include <memory>
#include <queue>
#include <unordered_map>
Expand Down Expand Up @@ -188,10 +187,6 @@ class WasmCodeLookupCache;
class WasmOrphanedGlobalHandle;
}

namespace detail {
class WaiterQueueNode;
} // namespace detail

#define RETURN_FAILURE_IF_EXCEPTION(isolate) \
do { \
Isolate* __isolate__ = (isolate); \
Expand Down Expand Up @@ -2247,9 +2242,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
battery_saver_mode_enabled_ = battery_saver_mode_enabled;
}

std::list<std::unique_ptr<detail::WaiterQueueNode>>&
async_waiter_queue_nodes();

private:
explicit Isolate(IsolateGroup* isolate_group);
~Isolate();
Expand Down Expand Up @@ -2702,10 +2694,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
ExternalBufferTable::Space* shared_external_buffer_space_ = nullptr;
#endif // V8_ENABLE_SANDBOX

// List to manage the lifetime of the WaiterQueueNodes used to track async
// waiters for JSSynchronizationPrimitives.
std::list<std::unique_ptr<detail::WaiterQueueNode>> async_waiter_queue_nodes_;

// Used to track and safepoint all client isolates attached to this shared
// isolate.
std::unique_ptr<GlobalSafepoint> global_safepoint_;
Expand Down
17 changes: 0 additions & 17 deletions src/heap/setup-heap-internal.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1566,23 +1566,6 @@ void Heap::CreateInitialMutableObjects() {
set_array_from_async_array_like_on_rejected_shared_fun(*info);
}

// Atomics.Mutex
{
Handle<SharedFunctionInfo> info = CreateSharedFunctionInfo(
isolate_, Builtin::kAtomicsMutexAsyncUnlockResolveHandler, 1);
set_atomics_mutex_async_unlock_resolve_handler_sfi(*info);
info = CreateSharedFunctionInfo(
isolate_, Builtin::kAtomicsMutexAsyncUnlockRejectHandler, 1);
set_atomics_mutex_async_unlock_reject_handler_sfi(*info);
}

// Atomics.Condition
{
Handle<SharedFunctionInfo> info = CreateSharedFunctionInfo(
isolate_, Builtin::kAtomicsConditionAcquireLock, 0);
set_atomics_condition_acquire_lock_sfi(*info);
}

// Trusted roots:
// TODO(saelo): these would ideally be read-only and shared, but we currently
// don't have a trusted RO space.
Expand Down
4 changes: 0 additions & 4 deletions src/init/bootstrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5695,8 +5695,6 @@ void Genesis::InitializeGlobal_harmony_struct() {
Builtin::kAtomicsMutexTryLock, 2, true);
SimpleInstallFunction(isolate(), mutex_fun, "isMutex",
Builtin::kAtomicsMutexIsMutex, 1, true);
SimpleInstallFunction(isolate(), mutex_fun, "lockAsync",
Builtin::kAtomicsMutexLockAsync, 2, true);
}

{ // Atomics.Condition
Expand All @@ -5717,8 +5715,6 @@ void Genesis::InitializeGlobal_harmony_struct() {
Builtin::kAtomicsConditionNotify, 2, false);
SimpleInstallFunction(isolate(), condition_fun, "isCondition",
Builtin::kAtomicsConditionIsCondition, 1, true);
SimpleInstallFunction(isolate(), condition_fun, "waitAsync",
Builtin::kAtomicsConditionWaitAsync, 2, false);
}
}

Expand Down
15 changes: 3 additions & 12 deletions src/objects/js-atomics-synchronization-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,8 @@ JSAtomicsMutex::TryLockGuard::TryLockGuard(Isolate* isolate,
: LockGuardBase(isolate, mutex, mutex->TryLock()) {}

// static
bool JSAtomicsMutex::LockImpl(Isolate* requester, Handle<JSAtomicsMutex> mutex,
base::Optional<base::TimeDelta> timeout,
LockSlowPathWrapper slow_path_wrapper) {
bool JSAtomicsMutex::Lock(Isolate* requester, Handle<JSAtomicsMutex> mutex,
base::Optional<base::TimeDelta> timeout) {
DisallowGarbageCollection no_gc;
// First try to lock an uncontended mutex, which should be the common case. If
// this fails, then go to the slow path to possibly put the current thread to
Expand All @@ -165,22 +164,14 @@ bool JSAtomicsMutex::LockImpl(Isolate* requester, Handle<JSAtomicsMutex> mutex,
std::memory_order_relaxed))) {
locked = true;
} else {
locked = slow_path_wrapper(state);
locked = LockSlowPath(requester, mutex, state, timeout);
}
if (V8_LIKELY(locked)) {
mutex->SetCurrentThreadAsOwner();
}
return locked;
}

// static
bool JSAtomicsMutex::Lock(Isolate* requester, Handle<JSAtomicsMutex> mutex,
base::Optional<base::TimeDelta> timeout) {
return LockImpl(requester, mutex, timeout, [=](std::atomic<StateT>* state) {
return LockSlowPath(requester, mutex, state, timeout);
});
}

bool JSAtomicsMutex::TryLock() {
DisallowGarbageCollection no_gc;
StateT expected = kUnlockedUncontended;
Expand Down

0 comments on commit 04c7ba1

Please sign in to comment.