Log Message
[JSC] DeferredWorkTimer should clear pending task after running https://bugs.webkit.org/show_bug.cgi?id=220888
Reviewed by Mark Lam. Wasm code assumes that scheduleWorkSoon clears pending dependencies. But DeferredWorkTimer is not clearing it, and instead, FinalizationRegistry etc. is clearing it explicitly. This semantics is problematic. We are putting cancelPendingWork in JSPromise::resolve / JSPromise::reject. But they do not work since this is C++ version of them, and JSPromise has JS version of them. And if JS version is called, cancelPendingWork is not called. And we do not want to complicate JSPromise's reject / resolve path since this is super hot, and we should keep them in JS. Instead, we should always clear pending task if it is called. * jsc.cpp: (JSC_DEFINE_HOST_FUNCTION): * runtime/DeferredWorkTimer.cpp: (JSC::DeferredWorkTimer::doWork): * runtime/DeferredWorkTimer.h: * runtime/JSFinalizationRegistry.cpp: (JSC::JSFinalizationRegistry::finalizeUnconditionally): * runtime/JSPromise.cpp: (JSC::JSPromise::resolve): Remove this work-around, and instead, we must call scheduleWorkSoon if addPendingWork is called. (JSC::JSPromise::reject): Ditto. * wasm/js/JSWebAssembly.cpp: (JSC::JSWebAssembly::webAssemblyModuleValidateAsync): (JSC::instantiate): Use instance for Ticket instead of promise to disambiguate the ticket scheduling from compileAndInstantiate's one easily. (JSC::compileAndInstantiate): There are path that we do not call scheduleWorkSoon while we call addPendingWork, this is wrong, and JSPromise's workaround is added to alleviate this situation. We should not do that: we must call scheduleWorkSoon at some point if addPendingWork is called. (JSC::JSWebAssembly::webAssemblyModuleInstantinateAsync):
Modified Paths
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/jsc.cpp
- trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp
- trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.h
- trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp
- trunk/Source/_javascript_Core/runtime/JSPromise.cpp
- trunk/Source/_javascript_Core/wasm/js/JSWebAssembly.cpp
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (271780 => 271781)
--- trunk/Source/_javascript_Core/ChangeLog 2021-01-23 22:08:45 UTC (rev 271780)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-01-23 22:11:28 UTC (rev 271781)
@@ -1,3 +1,33 @@
+2021-01-23 Yusuke Suzuki <ysuz...@apple.com>
+
+ [JSC] DeferredWorkTimer should clear pending task after running
+ https://bugs.webkit.org/show_bug.cgi?id=220888
+
+ Reviewed by Mark Lam.
+
+ Wasm code assumes that scheduleWorkSoon clears pending dependencies. But DeferredWorkTimer is not clearing it, and instead, FinalizationRegistry etc. is clearing
+ it explicitly. This semantics is problematic. We are putting cancelPendingWork in JSPromise::resolve / JSPromise::reject. But they do not work since this is C++
+ version of them, and JSPromise has JS version of them. And if JS version is called, cancelPendingWork is not called. And we do not want to complicate JSPromise's
+ reject / resolve path since this is super hot, and we should keep them in JS.
+ Instead, we should always clear pending task if it is called.
+
+ * jsc.cpp:
+ (JSC_DEFINE_HOST_FUNCTION):
+ * runtime/DeferredWorkTimer.cpp:
+ (JSC::DeferredWorkTimer::doWork):
+ * runtime/DeferredWorkTimer.h:
+ * runtime/JSFinalizationRegistry.cpp:
+ (JSC::JSFinalizationRegistry::finalizeUnconditionally):
+ * runtime/JSPromise.cpp:
+ (JSC::JSPromise::resolve): Remove this work-around, and instead, we must call scheduleWorkSoon if addPendingWork is called.
+ (JSC::JSPromise::reject): Ditto.
+ * wasm/js/JSWebAssembly.cpp:
+ (JSC::JSWebAssembly::webAssemblyModuleValidateAsync):
+ (JSC::instantiate): Use instance for Ticket instead of promise to disambiguate the ticket scheduling from compileAndInstantiate's one easily.
+ (JSC::compileAndInstantiate): There are path that we do not call scheduleWorkSoon while we call addPendingWork, this is wrong, and JSPromise's workaround is added to
+ alleviate this situation. We should not do that: we must call scheduleWorkSoon at some point if addPendingWork is called.
+ (JSC::JSWebAssembly::webAssemblyModuleInstantinateAsync):
+
2021-01-23 Xan Lopez <x...@igalia.com>
[JSC] Allow to build WebAssembly without B3
Modified: trunk/Source/_javascript_Core/jsc.cpp (271780 => 271781)
--- trunk/Source/_javascript_Core/jsc.cpp 2021-01-23 22:08:45 UTC (rev 271780)
+++ trunk/Source/_javascript_Core/jsc.cpp 2021-01-23 22:11:28 UTC (rev 271781)
@@ -2342,13 +2342,10 @@
// FIXME: We don't look at the timeout parameter because we don't have a schedule work later API.
vm.deferredWorkTimer->addPendingWork(vm, callback, { });
- vm.deferredWorkTimer->scheduleWorkSoon(callback, [callback] {
+ vm.deferredWorkTimer->scheduleWorkSoon(callback, [callback](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) {
JSGlobalObject* globalObject = callback->globalObject();
- VM& vm = globalObject->vm();
-
MarkedArgumentBuffer args;
call(globalObject, callback, jsUndefined(), args, "You shouldn't see this...");
- vm.deferredWorkTimer->cancelPendingWork(callback);
});
return JSValue::encode(jsUndefined());
}
Modified: trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp (271780 => 271781)
--- trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp 2021-01-23 22:08:45 UTC (rev 271780)
+++ trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp 2021-01-23 22:11:28 UTC (rev 271781)
@@ -67,11 +67,17 @@
suspendedTasks.append(std::make_tuple(ticket, WTFMove(task)));
continue;
case ScriptExecutionStatus::Stopped:
+ m_pendingTickets.remove(pendingTicket);
continue;
case ScriptExecutionStatus::Running:
break;
}
+ // Remove ticket from m_pendingTickets since we are going to run it.
+ // But we want to keep ticketData while running task since it ensures dependencies are strongly held.
+ auto ticketData = WTFMove(pendingTicket->value);
+ m_pendingTickets.remove(pendingTicket);
+
// Allow tasks we are about to run to schedule work.
m_currentlyRunningTask = true;
{
@@ -81,7 +87,7 @@
vm.finalizeSynchronousJSExecution();
auto scope = DECLARE_CATCH_SCOPE(vm);
- task();
+ task(ticket, WTFMove(ticketData));
if (Exception* exception = scope.exception()) {
auto* globalObject = ticket->globalObject();
scope.clearException();
@@ -146,17 +152,6 @@
return result.dependencies.contains(dependency);
}
-bool DeferredWorkTimer::cancelPendingWork(Ticket ticket)
-{
- ASSERT(ticket->vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket->vm().heap.worldIsStopped()));
- bool result = m_pendingTickets.remove(ticket);
-
- if (result)
- dataLogLnIf(DeferredWorkTimerInternal::verbose, "Canceling ticket: ", RawPointer(ticket));
-
- return result;
-}
-
void DeferredWorkTimer::scheduleWorkSoon(Ticket ticket, Task&& task)
{
LockHolder locker(m_taskLock);
Modified: trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.h (271780 => 271781)
--- trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.h 2021-01-23 22:08:45 UTC (rev 271780)
+++ trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.h 2021-01-23 22:11:28 UTC (rev 271781)
@@ -43,6 +43,11 @@
public:
using Base = JSRunLoopTimer;
+ struct TicketData {
+ Vector<Strong<JSCell>> dependencies;
+ Strong<JSObject> scriptExecutionOwner;
+ };
+
void doWork(VM&) final;
using Ticket = JSObject*;
@@ -49,7 +54,6 @@
void addPendingWork(VM&, Ticket, Vector<Strong<JSCell>>&& dependencies);
bool hasPendingWork(Ticket);
bool hasDependancyInPendingWork(Ticket, JSCell* dependency);
- bool cancelPendingWork(Ticket);
// If the script execution owner your ticket is associated with gets canceled
// the Task will not be called and will be deallocated. So it's important
@@ -56,7 +60,7 @@
// to make sure your memory ownership model won't leak memory when
// this occurs. The easiest way is to make sure everything is either owned
// by a GC'd value in dependencies or by the Task lambda.
- using Task = Function<void()>;
+ using Task = Function<void(Ticket, TicketData&&)>;
void scheduleWorkSoon(Ticket, Task&&);
void didResumeScriptExecutionOwner();
@@ -72,10 +76,6 @@
bool m_shouldStopRunLoopWhenAllTicketsFinish { false };
bool m_currentlyRunningTask { false };
Deque<std::tuple<Ticket, Task>> m_tasks;
- struct TicketData {
- Vector<Strong<JSCell>> dependencies;
- Strong<JSObject> scriptExecutionOwner;
- };
HashMap<Ticket, TicketData> m_pendingTickets;
};
Modified: trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp (271780 => 271781)
--- trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp 2021-01-23 22:08:45 UTC (rev 271780)
+++ trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp 2021-01-23 22:11:28 UTC (rev 271781)
@@ -148,11 +148,9 @@
if (!vm.deferredWorkTimer->hasPendingWork(this) && (readiedCell || deadCount(locker))) {
vm.deferredWorkTimer->addPendingWork(vm, this, { });
ASSERT(vm.deferredWorkTimer->hasPendingWork(this));
- vm.deferredWorkTimer->scheduleWorkSoon(this, [this] {
+ vm.deferredWorkTimer->scheduleWorkSoon(this, [this](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) {
JSGlobalObject* globalObject = this->globalObject();
- VM& vm = globalObject->vm();
this->runFinalizationCleanup(globalObject);
- vm.deferredWorkTimer->cancelPendingWork(this);
});
}
}
Modified: trunk/Source/_javascript_Core/runtime/JSPromise.cpp (271780 => 271781)
--- trunk/Source/_javascript_Core/runtime/JSPromise.cpp 2021-01-23 22:08:45 UTC (rev 271780)
+++ trunk/Source/_javascript_Core/runtime/JSPromise.cpp 2021-01-23 22:11:28 UTC (rev 271781)
@@ -168,7 +168,6 @@
callFunction(lexicalGlobalObject, globalObject->resolvePromiseFunction(), this, value);
RETURN_IF_EXCEPTION(scope, void());
}
- vm.deferredWorkTimer->cancelPendingWork(this);
}
void JSPromise::reject(JSGlobalObject* lexicalGlobalObject, JSValue value)
@@ -183,7 +182,6 @@
callFunction(lexicalGlobalObject, globalObject->rejectPromiseFunction(), this, value);
RETURN_IF_EXCEPTION(scope, void());
}
- vm.deferredWorkTimer->cancelPendingWork(this);
}
void JSPromise::rejectAsHandled(JSGlobalObject* lexicalGlobalObject, JSValue value)
Modified: trunk/Source/_javascript_Core/wasm/js/JSWebAssembly.cpp (271780 => 271781)
--- trunk/Source/_javascript_Core/wasm/js/JSWebAssembly.cpp 2021-01-23 22:08:45 UTC (rev 271780)
+++ trunk/Source/_javascript_Core/wasm/js/JSWebAssembly.cpp 2021-01-23 22:11:28 UTC (rev 271781)
@@ -169,9 +169,8 @@
dependencies.append(Strong<JSCell>(vm, globalObject));
vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
-
Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([promise, globalObject, &vm] (Wasm::Module::ValidationResult&& result) mutable {
- vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, globalObject, result = WTFMove(result), &vm] () mutable {
+ vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, globalObject, result = WTFMove(result), &vm](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable {
auto scope = DECLARE_THROW_SCOPE(vm);
JSValue module = JSWebAssemblyModule::createStub(vm, globalObject, globalObject->webAssemblyModuleStructure(), WTFMove(result));
if (UNLIKELY(scope.exception())) {
@@ -197,13 +196,14 @@
Vector<Strong<JSCell>> dependencies;
// The instance keeps the module alive.
- dependencies.append(Strong<JSCell>(vm, instance));
+ dependencies.append(Strong<JSCell>(vm, promise));
dependencies.append(Strong<JSCell>(vm, importObject));
- vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
+
+ vm.deferredWorkTimer->addPendingWork(vm, instance, WTFMove(dependencies));
// Note: This completion task may or may not get called immediately.
module->module().compileAsync(&vm.wasmContext, instance->memoryMode(), createSharedTask<Wasm::CodeBlock::CallbackType>([promise, instance, module, importObject, resolveKind, creationMode, &vm] (Ref<Wasm::CodeBlock>&& refCodeBlock) mutable {
RefPtr<Wasm::CodeBlock> codeBlock = WTFMove(refCodeBlock);
- vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, instance, module, importObject, resolveKind, creationMode, &vm, codeBlock = WTFMove(codeBlock)] () mutable {
+ vm.deferredWorkTimer->scheduleWorkSoon(instance, [promise, instance, module, importObject, resolveKind, creationMode, &vm, codeBlock = WTFMove(codeBlock)](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable {
JSGlobalObject* globalObject = instance->globalObject();
resolve(vm, globalObject, promise, instance, module, importObject, codeBlock.releaseNonNull(), resolveKind, creationMode);
});
@@ -214,12 +214,6 @@
{
auto scope = DECLARE_THROW_SCOPE(vm);
- JSCell* moduleKeyCell = identifierToJSValue(vm, moduleKey).asCell();
- Vector<Strong<JSCell>> dependencies;
- dependencies.append(Strong<JSCell>(vm, importObject));
- dependencies.append(Strong<JSCell>(vm, moduleKeyCell));
- vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
-
Vector<uint8_t> source = createSourceBufferFromValue(vm, globalObject, buffer);
if (UNLIKELY(scope.exception())) {
promise->rejectWithCaughtException(globalObject, scope);
@@ -226,8 +220,13 @@
return;
}
+ JSCell* moduleKeyCell = identifierToJSValue(vm, moduleKey).asCell();
+ Vector<Strong<JSCell>> dependencies;
+ dependencies.append(Strong<JSCell>(vm, importObject));
+ dependencies.append(Strong<JSCell>(vm, moduleKeyCell));
+ vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([promise, importObject, moduleKeyCell, globalObject, resolveKind, creationMode, &vm] (Wasm::Module::ValidationResult&& result) mutable {
- vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, importObject, moduleKeyCell, globalObject, result = WTFMove(result), resolveKind, creationMode, &vm] () mutable {
+ vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, importObject, moduleKeyCell, globalObject, result = WTFMove(result), resolveKind, creationMode, &vm](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable {
auto scope = DECLARE_THROW_SCOPE(vm);
JSWebAssemblyModule* module = JSWebAssemblyModule::createStub(vm, globalObject, globalObject->webAssemblyModuleStructure(), WTFMove(result));
if (UNLIKELY(scope.exception())) {
@@ -265,9 +264,8 @@
dependencies.append(Strong<JSCell>(vm, importObject));
dependencies.append(Strong<JSCell>(vm, globalObject));
vm.deferredWorkTimer->addPendingWork(vm, promise, WTFMove(dependencies));
-
Wasm::Module::validateAsync(&vm.wasmContext, WTFMove(source), createSharedTask<Wasm::Module::CallbackType>([promise, importObject, globalObject, &vm] (Wasm::Module::ValidationResult&& result) mutable {
- vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, importObject, globalObject, result = WTFMove(result), &vm] () mutable {
+ vm.deferredWorkTimer->scheduleWorkSoon(promise, [promise, importObject, globalObject, result = WTFMove(result), &vm](DeferredWorkTimer::Ticket, DeferredWorkTimer::TicketData&&) mutable {
auto scope = DECLARE_THROW_SCOPE(vm);
JSWebAssemblyModule* module = JSWebAssemblyModule::createStub(vm, globalObject, globalObject->webAssemblyModuleStructure(), WTFMove(result));
if (UNLIKELY(scope.exception())) {
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
