-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JSC] Cache softStackLimit in Wasm::Instance #14267
[JSC] Cache softStackLimit in Wasm::Instance #14267
Conversation
EWS run on previous version of this PR (hash 45858db) |
45858db
to
b6592ad
Compare
@@ -1086,6 +1094,9 @@ class VM : public ThreadSafeRefCounted<VM>, public DoublyLinkedListNode<VM> { | |||
Ref<Waiter> m_syncWaiter; | |||
|
|||
Vector<Function<void()>> m_didPopListeners; | |||
#if ENABLE(WEBASSEMBLY) | |||
ThreadSafeWeakHashSet<Wasm::Instance> m_wasmInstances; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is WeakHashSet. So Wasm::Instance will be removed automatically when it dies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: there is a 1 to 1 correspondence between VM and WasmInstance, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. WasmInstance -> VM. But no VM -> WasmInstance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The part I want to make sure of is that a WasmInstance
can only be associated with 1 VM, correct? It is never associated with more than 1 VM at the same time, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. Yusuke pointed out offline that the commit msg already addressed this: "Wasm::Instance is strictly tied to one VM."
EWS run on current version of this PR (hash b6592ad) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
https://bugs.webkit.org/show_bug.cgi?id=257230 rdar://109740794 Reviewed by Justin Michaud. Let's store m_softStackLimit in Wasm::Instance directly. Wasm::Instance is strictly tied to one VM. This patch associates Wasm::Instance to VM, and we update this field of Instance when VM's m_softStackLimit changes (this is very rare). So we do not need to do double-load to stack overflow check in each prologue of wasm functions. * Source/JavaScriptCore/jit/AssemblyHelpers.cpp: (JSC::AssemblyHelpers::checkWasmStackOverflow): * Source/JavaScriptCore/llint/WebAssembly.asm: * Source/JavaScriptCore/runtime/VM.cpp: (JSC::VM::updateStackLimits): (JSC::VM::registerWasmInstance): * Source/JavaScriptCore/runtime/VM.h: * Source/JavaScriptCore/wasm/WasmBBQJIT.cpp: (JSC::Wasm::BBQJIT::addTopLevel): (JSC::Wasm::BBQJIT::addLoopOSREntrypoint): * Source/JavaScriptCore/wasm/WasmInstance.cpp: (JSC::Wasm::Instance::Instance): * Source/JavaScriptCore/wasm/WasmInstance.h: (JSC::Wasm::Instance::offsetOfSoftStackLimit): (JSC::Wasm::Instance::updateSoftStackLimit): Canonical link: https://commits.webkit.org/264531@main
b6592ad
to
5be4a1f
Compare
Committed 264531@main (5be4a1f): https://commits.webkit.org/264531@main Reviewed commits have been landed. Closing PR #14267 and removing active labels. |
5be4a1f
b6592ad