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

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
types
https://bugs.webkit.org/show_bug.cgi?id=198157

Attachment 372170: Patch

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




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

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
> +	  
visitor.appendUnbarriered(thisObject->instance().getFunctionWrapper(i));

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.