Bug 170488

Created attachment 306357 [details] WIP I wonder if it works.

Created attachment 306360 [details] WIP passes most tests. Looks like I got some concurrency issues.

Created attachment 306361 [details] WIP It passes all wasm tests, and runs wasm bench on a debug build. I need to clean some stuff up and then convince myself it's correct.

Attachment 306361 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:52: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:53: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:39: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:67: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:71: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:72: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:74: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:76: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/WasmPlan.cpp:200: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/WasmPlan.cpp:201: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:112: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/WasmBinding.h:42: The parameter name "signatureIndex" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 13 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 306433 [details] patch

Comment on attachment 306433 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306433&action=review r- on the CodeBlock being in it's own file and the callback to finalize a CodeBlock. > Source/JavaScriptCore/wasm/WasmModule.cpp:39 > + m_plan = adoptRef(*new Plan(vm, makeRef(moduleInformation), Plan::FullCompile, [this] (VM&, Plan&) { What happens if someone cancels this vm. Who initializes the callees? I think you need to make the plan aware of the CodeBlock it's compiling for. When the Plan finishes it should call some linking hook on the CodeBlock. That linking hook can remove the RefPtr to the Plan or something. > Source/JavaScriptCore/wasm/WasmModule.h:94 > +class CodeBlock : public ThreadSafeRefCounted<CodeBlock> { > +public: > + using AsyncCompilationCallback = std::function<void(VM&)>; > + static Ref<CodeBlock> create(VM& vm, MemoryMode mode, ModuleInformation& moduleInformation) > + { > + return adoptRef(*new CodeBlock(vm, mode, moduleInformation)); > + } > + > + void waitUntilFinished(); > + void compileAsync(VM&, AsyncCompilationCallback&&); > + > + bool compilationFinished() > + { > + auto locker = holdLock(m_lock); > + return !m_plan; > + } > + bool runnable() { return compilationFinished() && !m_errorMessage; } > + > + // Note, we do this copy to ensure it's thread safe to have this > + // called from multiple threads simultaneously. > + String errorMessage() > + { > + ASSERT(!runnable()); > + CString cString = m_errorMessage.ascii(); > + return String(cString.data()); > + } > + > + unsigned functionImportCount() const { return m_wasmToWasmExitStubs.size(); } > + > + Wasm::Callee& jsEntrypointCalleeFromFunctionIndexSpace(unsigned functionIndexSpace) > + { > + RELEASE_ASSERT(functionIndexSpace >= functionImportCount()); > + unsigned calleeIndex = functionIndexSpace - functionImportCount(); > + RELEASE_ASSERT(calleeIndex < m_calleeCount); > + return *m_callees[calleeIndex].get(); > + } > + Wasm::Callee& wasmEntrypointCalleeFromFunctionIndexSpace(unsigned functionIndexSpace) > + { > + RELEASE_ASSERT(functionIndexSpace >= functionImportCount()); > + unsigned calleeIndex = functionIndexSpace - functionImportCount(); > + RELEASE_ASSERT(calleeIndex < m_calleeCount); > + return *m_callees[calleeIndex + m_calleeCount].get(); > + } > + bool isSafeToRun(MemoryMode); > + > +private: > + CodeBlock(VM&, MemoryMode, ModuleInformation&); > + unsigned m_calleeCount; > + MemoryMode m_mode; > + Vector<RefPtr<Callee>> m_callees; > + Vector<MacroAssemblerCodeRef> m_wasmToWasmExitStubs; > + RefPtr<Plan> m_plan; > + String m_errorMessage; > + Lock m_lock; > +}; This should be in it's own file along with the code from the .cpp file above.

Comment on attachment 306433 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306433&action=review It might also be worthwhile making the std::functions into SharedTasks. The new code moves them around a lot which is expensive for std::function. > Source/JavaScriptCore/wasm/WasmModule.h:98 > + using ValidationResult = WTF::Expected<RefPtr<Module>, String>; Why not make this a Ref?

Comment on attachment 306433 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306433&action=review >> Source/JavaScriptCore/wasm/WasmModule.cpp:39 >> + m_plan = adoptRef(*new Plan(vm, makeRef(moduleInformation), Plan::FullCompile, [this] (VM&, Plan&) { > > What happens if someone cancels this vm. Who initializes the callees? > > I think you need to make the plan aware of the CodeBlock it's compiling for. When the Plan finishes it should call some linking hook on the CodeBlock. That linking hook can remove the RefPtr to the Plan or something. Yup, this is a bug. I'll fix it. I think there are only two reasons for failure here: 1. Cancellation 2. OOM Perhaps we should just always retry to compile on failure in the future if the module asks to get compiled again for a particular memory mode? >> Source/JavaScriptCore/wasm/WasmModule.h:98 >> + using ValidationResult = WTF::Expected<RefPtr<Module>, String>; > > Why not make this a Ref? I wanted to, but was getting weird compilation errors.

Created attachment 306480 [details] patch

Comment on attachment 306480 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306480&action=review Looks good. A few comments, and GTK is sad on isSafeToRun linking. > Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp:47 > + m_taskLock.lock(); Trying to understand this lock. IIUC you need it for: - timers - pending promises - task / drain micro tasks - runloop ? It just seems weird / brittle to manually lock + unlock like this. IIUC doing it manually allows you to lock what you need and join different things that can be locked together? > Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:44 > + // The JS API insures this invariant by keeping alive the module as it has "ensures" > Source/JavaScriptCore/wasm/WasmCodeBlock.h:59 > + // called from multiple threads simultaneously. Is the ASCII version likely to die? I'm not sure I understand the thread + lifetime issue here. > Source/JavaScriptCore/wasm/WasmModule.cpp:61 > + // It's worth retrying. How often do we retry though? As a separate patch we'll want to reduce the number of concurrent compilations if we OOM, so I guess we can just leave a FIXME here? > Source/JavaScriptCore/wasm/WasmPlan.cpp:362 > fail(locker, ASCIILiteral("WebAssembly Plan was canceled. If you see this error message please file a bug at bugs.webkit.org!")); We spell it "cancelled" elsewhere. Brit spelling.

Comment on attachment 306480 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306480&action=review >> Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp:47 >> + m_taskLock.lock(); > > Trying to understand this lock. IIUC you need it for: > > - timers > - pending promises > - task / drain micro tasks > - runloop > > ? > > It just seems weird / brittle to manually lock + unlock like this. IIUC doing it manually allows you to lock what you need and join different things that can be locked together? This lock is only used to protect access to m_tasks. The reason I release the lock is that scheduleWorkSoon() acquires the lock. If we didn't unlock here, and the task calls scheduleWorkSoon(), we'll deadlock. >> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:44 >> + // The JS API insures this invariant by keeping alive the module as it has > > "ensures" fixed. >> Source/JavaScriptCore/wasm/WasmCodeBlock.h:59 >> + // called from multiple threads simultaneously. > > Is the ASCII version likely to die? I'm not sure I understand the thread + lifetime issue here. The issue is this: 2 threads simultaneously call this. WTF::String ref counts the StringImpl it owns. This is not done atomically. So we need to make a copy here for each thing that asks for the error message. >> Source/JavaScriptCore/wasm/WasmModule.cpp:61 >> + // It's worth retrying. > > How often do we retry though? As a separate patch we'll want to reduce the number of concurrent compilations if we OOM, so I guess we can just leave a FIXME here? I'll file a FIXME to implement some backoff policy or something similar. >> Source/JavaScriptCore/wasm/WasmPlan.cpp:362 >> fail(locker, ASCIILiteral("WebAssembly Plan was canceled. If you see this error message please file a bug at bugs.webkit.org!")); > > We spell it "cancelled" elsewhere. Brit spelling. I'll fix it.

> >> Source/JavaScriptCore/wasm/WasmCodeBlock.h:59 > >> + // called from multiple threads simultaneously. > > > > Is the ASCII version likely to die? I'm not sure I understand the thread + lifetime issue here. > > The issue is this: > 2 threads simultaneously call this. WTF::String ref counts the StringImpl it > owns. This is not done atomically. So we need to make a copy here for each > thing that asks for the error message. Can't you return the const char* and avoid the issue entirely?

landed in: https://trac.webkit.org/changeset/215103/webkit

(In reply to JF Bastien from comment #12) > > >> Source/JavaScriptCore/wasm/WasmCodeBlock.h:59 > > >> + // called from multiple threads simultaneously. > > > > > > Is the ASCII version likely to die? I'm not sure I understand the thread + lifetime issue here. > > > > The issue is this: > > 2 threads simultaneously call this. WTF::String ref counts the StringImpl it > > owns. This is not done atomically. So we need to make a copy here for each > > thing that asks for the error message. > > Can't you return the const char* and avoid the issue entirely? Each caller would want to make a String out of it anyways (note there are only two callers, I believe). I think it's nice to just give the caller what they want in a safe way.