[Webkit-unassigned] [Bug 170316] WebAssembly: Ref count Signature and SignatureInformation should not care about VM

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=170316

--- Comment #5 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 305943
  --> https://bugs.webkit.org/attachment.cgi?id=305943
patch

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

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:412
>> +auto B3IRGenerator::addArguments(const RefPtr<Signature>& signature) -> PartialResult
> 
> style: Revert this or use Signature& here. It's an anti-pattern to use a ref to a container class when the function isn't going hold any reference anyway.

I don't agree. I made "const RefPtr<Signature>&" precisely because the function doesn't need to ref it.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:884
>> +auto B3IRGenerator::addCall(uint32_t functionIndex, RefPtr<Signature>&& signature, Vector<ExpressionType>& args, ExpressionType& result) -> PartialResult
> 
> ditto.

I think these should also be "const RefPtr<Signature>&"

>> Source/JavaScriptCore/wasm/WasmFormat.h:237
>> +    Vector<RefPtr<Signature>> usedSignatures;
> 
> Isn't it kinda weird to duplicate this information? We already have a list of indices we use above, but it's not nicely RAII refcount so we dup it here. :(

I think this is the price to pay for using a raw uint32_t for signature index. We could make this fancier, but I think that belongs in its own patch.

>> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:111
>> +    WASM_PARSER_FAIL_IF(!m_result.module->usedSignatures.tryReserveCapacity(count), "can't allocate enough memory for Type section's ", count, " entries");
> 
> The error message is misleading since this allocation isn't the same as above, but errors out the same.

To a user of web assembly, what's the difference?

>>> Source/JavaScriptCore/wasm/WasmSignature.cpp:74
>>> +    if (argumentCount > 1024 * 1024)
>> 
>> This is the wrong place to enforce a limit. We should do this in the parser, and in one shot as part of bug #165833.
> 
> Agreed.

Ok.

>> Source/JavaScriptCore/wasm/WasmSignature.cpp:77
>> +    void* memory = Signature::operator new (allocatedSize(argumentCount));
> 
> Other parts of the code use the try* allocators so that wasm can OOM without being sad. It was tryFastCalloc above, we should keep that property here.

Ok.

>> Source/JavaScriptCore/wasm/WasmSignature.cpp:151
>> +    if (!info.m_signatureMap.size()) {
> 
> nit: You could do info.m_signatureMap.isEmpty() I think it's a little easier to read.

ok.

>> Source/JavaScriptCore/wasm/WasmSignature.cpp:153
>> +        info.m_nextIndex = Signature::invalidIndex + 1;
> 
> Here and elsewhere, it would be nice to have a symbolic constant for "first valid index". This otherwise assumes that the invalid index is 0.

sgtm

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170331/a083e77e/attachment-0001.html>