Skip to content

Commit

Permalink
Revert "[explicit-resource-management] Avoid unnecessary awaits"
Browse files Browse the repository at this point in the history
This reverts commit f054729.

Reason for revert: https://issues.chromium.org/u/1/issues/365060988

Original change's description:
> [explicit-resource-management] Avoid unnecessary awaits
>
> This CL reduces unnecessary Awaits for nullish values in blocks containing `await using` and in `AsyncDisposableStack`
> tc39/proposal-explicit-resource-management#219
>
>
> Bug: 42203814
> Change-Id: Ia94755bd19a217512d308b5b4f524471b8321d63
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5819755
> Reviewed-by: Shu-yu Guo <[email protected]>
> Commit-Queue: Rezvan Mahdavi Hezaveh <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#95982}

Bug: 42203814
Change-Id: I932341e117fd02299bda224ab5ff694372006ac0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5840443
Bot-Commit: Rubber Stamper <[email protected]>
Owners-Override: Rezvan Mahdavi Hezaveh <[email protected]>
Commit-Queue: Rezvan Mahdavi Hezaveh <[email protected]>
Cr-Commit-Position: refs/heads/main@{#95998}
  • Loading branch information
rmahdav authored and V8 LUCI CQ committed Sep 6, 2024
1 parent 7d0e8c0 commit c4096b8
Show file tree
Hide file tree
Showing 10 changed files with 51 additions and 221 deletions.
12 changes: 0 additions & 12 deletions src/builtins/promise-abstract-operations.tq
Original file line number Diff line number Diff line change
Expand Up @@ -488,18 +488,6 @@ transitioning macro PerformPromiseThenImpl(
promise.SetHasHandler();
}

transitioning javascript builtin PerformPromiseThenFunction(
js-implicit context: NativeContext, receiver: JSAny)(onFulfilled: JSAny,
onRejected: JSAny): JSAny {
const jsPromise = Cast<JSPromise>(receiver) otherwise unreachable;
const callableOnFulfilled = Cast<Callable>(onFulfilled) otherwise unreachable;
const callableOnRejected = Cast<Callable>(onRejected) otherwise unreachable;

PerformPromiseThenImpl(
jsPromise, callableOnFulfilled, callableOnRejected, Undefined);
return Undefined;
}

// https://tc39.es/ecma262/#sec-performpromisethen
transitioning builtin PerformPromiseThen(
implicit context: Context)(promise: JSPromise,
Expand Down
4 changes: 0 additions & 4 deletions src/diagnostics/objects-printer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1977,8 +1977,6 @@ void JSDisposableStackBase::JSDisposableStackBasePrint(std::ostream& os) {
os << "\n - stack: " << Brief(stack());
os << "\n - length: " << length();
os << "\n - state: " << state();
os << "\n - needsAwait: " << needsAwait();
os << "\n - hasAwaited: " << hasAwaited();
os << "\n - error: " << error();
JSObjectPrintBody(os, *this);
}
Expand All @@ -1988,8 +1986,6 @@ void JSAsyncDisposableStack::JSAsyncDisposableStackPrint(std::ostream& os) {
os << "\n - stack: " << Brief(stack());
os << "\n - length: " << length();
os << "\n - state: " << state();
os << "\n - needsAwait: " << needsAwait();
os << "\n - hasAwaited: " << hasAwaited();
os << "\n - error: " << error();
JSObjectPrintBody(os, *this);
}
Expand Down
6 changes: 0 additions & 6 deletions src/init/bootstrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3272,12 +3272,6 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
isolate_, prototype, "then", Builtin::kPromisePrototypeThen, 2, kAdapt);
native_context()->set_promise_then(*promise_then);

DirectHandle<JSFunction> perform_promise_then =
InstallFunctionWithBuiltinId(isolate_, prototype, "performPromiseThen",
Builtin::kPerformPromiseThenFunction, 2,
kAdapt);
native_context()->set_perform_promise_then(*perform_promise_then);

InstallFunctionWithBuiltinId(isolate_, prototype, "catch",
Builtin::kPromisePrototypeCatch, 1, kAdapt);

Expand Down
2 changes: 1 addition & 1 deletion src/interpreter/bytecode-generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2732,7 +2732,7 @@ void BytecodeGenerator::BuildDisposeScope(WrappedFunc wrapped_func,

builder()
->StoreAccumulatorInRegister(result_register)
.LoadTrue()
.LoadUndefined()
.CompareReference(result_register);

loop_builder.BreakIfTrue(ToBooleanMode::kConvertToBoolean);
Expand Down
1 change: 0 additions & 1 deletion src/objects/contexts.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ enum ContextLookupFlags {
async_module_evaluate_internal) \
V(REFLECT_APPLY_INDEX, JSFunction, reflect_apply) \
V(REFLECT_CONSTRUCT_INDEX, JSFunction, reflect_construct) \
V(PERFORM_PROMISE_THEN_INDEX, JSFunction, perform_promise_then) \
V(PROMISE_THEN_INDEX, JSFunction, promise_then) \
V(PROMISE_RESOLVE_INDEX, JSFunction, promise_resolve) \
V(FUNCTION_PROTOTYPE_APPLY_INDEX, JSFunction, function_prototype_apply) \
Expand Down
139 changes: 50 additions & 89 deletions src/objects/js-disposable-stack.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,93 +70,65 @@ MaybeHandle<Object> JSDisposableStackBase::DisposeResources(
DisposeCallTypeBit::decode(stack_type_case);
DisposeMethodHint hint = DisposeHintBit::decode(stack_type_case);

// (TODO:rezvan):
// https://github.com/tc39/proposal-explicit-resource-management/pull/219
// d. If hint is sync-dispose and needsAwait is true and hasAwaited is
// false, then
// i. Perform ! Await(undefined).
// ii. Set needsAwait to false.

if (hint == DisposeMethodHint::kSyncDispose &&
disposable_stack->needsAwait() == true &&
disposable_stack->hasAwaited() == false) {
// i. Perform ! Await(undefined).
// ii. Set needsAwait to false.
disposable_stack->set_needsAwait(false);

return ResolveAPromiseWithValueAndReturnIt(
isolate, ReadOnlyRoots(isolate).undefined_value_handle());
// e. If method is not undefined, then
// i. Let result be Completion(Call(method, value)).
// ii. If result is a normal completion and hint is async-dispose, then
// 1. Set result to Completion(Await(result.[[Value]])).
// 2. Set hasAwaited to true.
// iii. If result is a throw completion, then
// Else,
// i. Assert: hint is async-dispose.
// ii. Set needsAwait to true.
// iii. NOTE: This can only indicate a case where either null or
// undefined was the initialized value of an await using declaration.
// 4. If needsAwait is true and hasAwaited is false, then
// a. Perform ! Await(undefined).
v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
try_catch.SetVerbose(false);
try_catch.SetCaptureMessage(false);

if (call_type == DisposeMethodCallType::kValueIsReceiver) {
result = Execution::Call(isolate, method, value, 0, nullptr);
} else if (call_type == DisposeMethodCallType::kValueIsArgument) {
result = Execution::Call(isolate, method,
ReadOnlyRoots(isolate).undefined_value_handle(),
1, argv);
}

// e. If method is not undefined, then
if (!IsUndefined(*method)) {
// i. Let result be Completion(Call(method, value)).
v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
try_catch.SetVerbose(false);
try_catch.SetCaptureMessage(false);

if (call_type == DisposeMethodCallType::kValueIsReceiver) {
result = Execution::Call(isolate, method, value, 0, nullptr);
} else if (call_type == DisposeMethodCallType::kValueIsArgument) {
result = Execution::Call(
isolate, method, ReadOnlyRoots(isolate).undefined_value_handle(), 1,
argv);
}
Handle<Object> result_handle;

Handle<Object> result_handle;
// ii. If result is a normal completion and hint is async-dispose, then
// 1. Set result to Completion(Await(result.[[Value]])).
// 2. Set hasAwaited to true.
if (result.ToHandle(&result_handle)) {
if (hint == DisposeMethodHint::kAsyncDispose) {
DCHECK_NE(resources_type, DisposableStackResourcesType::kAllSync);
disposable_stack->set_length(length);

disposable_stack->set_hasAwaited(true);

return ResolveAPromiseWithValueAndReturnIt(isolate, result_handle);
}
} else {
// iii. If result is a throw completion, then
// 1. If completion is a throw completion, then
// a. Set result to result.[[Value]].
// b. Let suppressed be completion.[[Value]].
// c. Let error be a newly created SuppressedError object.
// d. Perform CreateNonEnumerableDataPropertyOrThrow(error,
// "error", result). e. Perform
// CreateNonEnumerableDataPropertyOrThrow(error, "suppressed",
// suppressed). f. Set completion to ThrowCompletion(error).
// 2. Else,
// a. Set completion to result.
DCHECK(isolate->has_exception());
DCHECK(try_catch.HasCaught());
Handle<Object> current_error(isolate->exception(), isolate);
if (!isolate->is_catchable_by_javascript(*current_error)) {
return {};
}
HandleErrorInDisposal(isolate, disposable_stack, current_error);
if (result.ToHandle(&result_handle)) {
if (hint == DisposeMethodHint::kAsyncDispose) {
DCHECK_NE(resources_type, DisposableStackResourcesType::kAllSync);
disposable_stack->set_length(length);

Handle<JSFunction> promise_function = isolate->promise_function();
Handle<Object> argv[] = {result_handle};
Handle<Object> resolve_result =
Execution::CallBuiltin(isolate, isolate->promise_resolve(),
promise_function, arraysize(argv), argv)
.ToHandleChecked();
return Cast<JSReceiver>(resolve_result);
}
} else {
// Else,
// i. Assert: hint is async-dispose.
DCHECK_EQ(hint, DisposeMethodHint::kAsyncDispose);
// ii. Set needsAwait to true.
// iii. NOTE: This can only indicate a case where either null or
// undefined was the initialized value of an await using declaration.
disposable_stack->set_length(length);
disposable_stack->set_needsAwait(true);
// b. If result is a throw completion, then
DCHECK(isolate->has_exception());
DCHECK(try_catch.HasCaught());
Handle<Object> current_error(isolate->exception(), isolate);
if (!isolate->is_catchable_by_javascript(*current_error)) {
return {};
}
HandleErrorInDisposal(isolate, disposable_stack, current_error);
}
}

// 4. If needsAwait is true and hasAwaited is false, then
// a. Perform ! Await(undefined).
if (disposable_stack->needsAwait() == true &&
disposable_stack->hasAwaited() == false) {
disposable_stack->set_length(length);
disposable_stack->set_hasAwaited(true);

return ResolveAPromiseWithValueAndReturnIt(
isolate, ReadOnlyRoots(isolate).undefined_value_handle());
}

// 5. NOTE: After disposeCapability has been disposed, it will never be used
// again. The contents of disposeCapability.[[DisposableResourceStack]] can be
// discarded in implementations, such as by garbage collection, at this point.
Expand All @@ -174,18 +146,7 @@ MaybeHandle<Object> JSDisposableStackBase::DisposeResources(
isolate->Throw(*existing_error_handle);
return MaybeHandle<Object>();
}
return isolate->factory()->true_value();
}

Handle<JSReceiver> JSDisposableStackBase::ResolveAPromiseWithValueAndReturnIt(
Isolate* isolate, Handle<Object> value) {
Handle<JSFunction> promise_function = isolate->promise_function();
Handle<Object> argv[] = {value};
Handle<Object> resolve_result =
Execution::CallBuiltin(isolate, isolate->promise_resolve(),
promise_function, arraysize(argv), argv)
.ToHandleChecked();
return Cast<JSReceiver>(resolve_result);
return isolate->factory()->undefined_value();
}

Maybe<bool> JSAsyncDisposableStack::NextDisposeAsyncIteration(
Expand All @@ -208,7 +169,7 @@ Maybe<bool> JSAsyncDisposableStack::NextDisposeAsyncIteration(
Handle<Object> result_handle;

if (result.ToHandle(&result_handle)) {
if (!IsTrue(*result_handle)) {
if (!IsUndefined(*result_handle)) {
Handle<Context> async_disposable_stack_context =
isolate->factory()->NewBuiltinContext(
isolate->native_context(),
Expand Down Expand Up @@ -240,8 +201,8 @@ Maybe<bool> JSAsyncDisposableStack::NextDisposeAsyncIteration(
.Build();

Handle<Object> argv[] = {on_fulfilled, on_rejected};

Execution::CallBuiltin(isolate, isolate->perform_promise_then(),
// (TODO:rezvan): Add a wrapper function for PerformPromiseThen.
Execution::CallBuiltin(isolate, isolate->promise_then(),
Cast<JSPromise>(result_handle), arraysize(argv),
argv)
.ToHandleChecked();
Expand Down
2 changes: 0 additions & 2 deletions src/objects/js-disposable-stack.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,6 @@ class JSDisposableStackBase
Isolate* isolate, DirectHandle<JSDisposableStackBase> disposable_stack,
MaybeHandle<Object> maybe_continuation_error,
DisposableStackResourcesType resources_type);
static Handle<JSReceiver> ResolveAPromiseWithValueAndReturnIt(
Isolate* isolate, Handle<Object> value);
static void HandleErrorInDisposal(
Isolate* isolate, DirectHandle<JSDisposableStackBase> disposable_stack,
Handle<Object> current_error);
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

0 comments on commit c4096b8

Please sign in to comment.