[webkit-reviews] review granted: [Bug 198157] [WASM-References] Add support for Funcref in parameters and return types : [Attachment 372170] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jun 16 00:16:28 PDT 2019

Yusuke Suzuki <ysuzuki at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 198157: [WASM-References] Add support for Funcref in parameters and return

Attachment 372170: Patch


--- Comment #8 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 372170
  --> https://bugs.webkit.org/attachment.cgi?id=372170

View in context: https://bugs.webkit.org/attachment.cgi?id=372170&action=review

r=me with several fixes.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:86
> +    return m_functionWrappers.contains(i) ? m_functionWrappers.get(i).get()
: jsNull();

`contains(i)` and `get(i)` calls are duplicate basically. So we are looking up
hash table twice.
Can we just use `get(i)`? I think it will return empty WriteBarrier thing.
Then, we can convert JSEmpty to JSNull, and we can do this function without
using `contains`.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:98
> +	  

Ah, I missed it. Why not just iterating the hashmap of wrappers? I think # of
wrappers can be much smaller than the total number of function space size. And
it also avoids repeated calls of `contains()` / `get()` in getFunctionWrapper.
And I think the hashtable can be modified by the user's setFunctionWrapper. So
locking is necessary.

More information about the webkit-reviews mailing list