[webkit-reviews] review denied: [Bug 170488] WebAssembly: Make to a compilation API that allows for multi-VM concurrent compilations of Wasm Modules : [Attachment 306433] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 6 16:38:41 PDT 2017
Keith Miller <keith_miller at apple.com> has denied Saam Barati
<sbarati at apple.com>'s request for review:
Bug 170488: WebAssembly: Make to a compilation API that allows for multi-VM
concurrent compilations of Wasm Modules
https://bugs.webkit.org/show_bug.cgi?id=170488
Attachment 306433: patch
https://bugs.webkit.org/attachment.cgi?id=306433&action=review
--- Comment #6 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 306433
--> https://bugs.webkit.org/attachment.cgi?id=306433
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.
More information about the webkit-reviews
mailing list