Bug 170316 – WebAssembly: Ref count Signature and SignatureInformation should not care about VM

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

We can refcount individual ones. Or we can have VM refcount the entire table.

[Saam Barati]

Created attachment 305937 [details]
WIP

crashes in debug

[Saam Barati]

Created attachment 305943 [details]
patch

[JF Bastien]

Comment on attachment 305943 [details]
patch

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

I'm not sure all the `const Signature *` need to be RefPtr: their lifetime is already bound by their caller. The way I tried to set this up, raw pointers are just things you can access without thinking about who owns it: you don't, someone else up your call chain does. I a way this is the "dumb pointer" idiom.

I'll need to take another look when I'm not as tired :)

> Source/JavaScriptCore/wasm/WasmBinding.cpp:-422
> -    String signatureDescription = SignatureInformation::get(vm, signatureIndex)->toString();

I think Keith had made this conditional on debug because it was slow?

> 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. :(

> 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.

> 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.

> 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.

> 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.

[Keith Miller]

Comment on attachment 305943 [details]
patch

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

r- on some comments JF and I made.

> 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.

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

ditto.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:965
> +auto B3IRGenerator::addCallIndirect(RefPtr<Signature>&& signature, SignatureIndex signatureIndex, Vector<ExpressionType>& args, ExpressionType& result) -> PartialResult

ditto.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1086
> +static void createJSToWasmWrapper(CompilationContext& compilationContext, WasmInternalFunction& function, const RefPtr<Signature>& signature, const ModuleInformation& info)

ditto.

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:-422
>> -    String signatureDescription = SignatureInformation::get(vm, signatureIndex)->toString();
> 
> I think Keith had made this conditional on debug because it was slow?

Moving this into the macro is fine since that case of the macro only runs if you have the option set.

>> 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.

> 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.

[Saam Barati]

Comment on attachment 305943 [details]
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

[Saam Barati]

Comment on attachment 305943 [details]
patch

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

>>> 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.

Actually, Ima keep this as is. Our handling of an argument count limit should make tryMalloc here useless. We should enforce our own limit during module parsing.

[Saam Barati]

Comment on attachment 305943 [details]
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.

I misread your comment. Anyways, I still don't agree. We use this pattern all over the place in JSC.

[Saam Barati]

Comment on attachment 305943 [details]
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.
> 
> I misread your comment. Anyways, I still don't agree. We use this pattern all over the place in JSC.

Also, I think the const RefPtr<Signature>& does communicate something to us here. It says somebody calling us is ensuring the proper lifetime. I think it's an information regression to use Signature*. I can see an argument for using Signature&, but I like knowing when something is ref counted.

[Saam Barati]

Created attachment 306001 [details]
patch

Addressed most comments. I also switched to using Ref instead of RefPtr. There is no reason a signature should ever be null. This would be very wrong.

[Keith Miller]

Comment on attachment 306001 [details]
patch

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

r=me

> Source/JavaScriptCore/wasm/WasmSignature.cpp:72
> +    void* memory = Signature::operator new (allocatedSize(argumentCount));
> +    Signature* result = new (memory) Signature(argumentCount);

Why not use the NotNull flag?

[Saam Barati]

Created attachment 306012 [details]
patch

[Saam Barati]

Comment on attachment 306012 [details]
patch

This is the patch for landing.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/214691/webkit

[JF Bastien]

Comment on attachment 306012 [details]
patch

Looks good!