[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]

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.