Bug 170478 – WebAssembly: ModuleInformation should be a ref counted thing that can be shared across threads.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

WebAssembly: ModuleInformation should be a ref counted thing that can be shared across threads.

[Keith Miller]

Created attachment 306222 [details]
Patch

[JF Bastien]

Does this address https://bugs.webkit.org/show_bug.cgi?id=170296 ?

[Keith Miller]

(In reply to JF Bastien from comment #2)
> Does this address https://bugs.webkit.org/show_bug.cgi?id=170296 ?

We pass the names.wast.js test. So I believe so but we should still double check.

[JF Bastien]

Comment on attachment 306222 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmModuleInformation.h:32
> +#include <wtf/Vector.h>

Missing include for optional.

> Source/JavaScriptCore/wasm/WasmModuleParser.h:40
> +    ModuleParser(const uint8_t* sourceBuffer, size_t sourceLength, ModuleInformation& info)

I'd rather keep returning the ModuleInformation instead of passing in a reference to it: on failure it was literally impossible to have a ModuleInformation from the previous code. Now the reference can be half-valid.

[JF Bastien]

(In reply to Keith Miller from comment #3)
> (In reply to JF Bastien from comment #2)
> > Does this address https://bugs.webkit.org/show_bug.cgi?id=170296 ?
> 
> We pass the names.wast.js test. So I believe so but we should still double
> check.

The new test hasn't been committed yet:
  https://github.com/WebAssembly/spec/pull/450

So let's leave the other bug open for now. I don't think we need to resolve it any time soon.

[Keith Miller]

Comment on attachment 306222 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmModuleInformation.h:32
>> +#include <wtf/Vector.h>
> 
> Missing include for optional.

WasmFormat already has optional. If anything I can just delete the vector include.

>> Source/JavaScriptCore/wasm/WasmModuleParser.h:40
>> +    ModuleParser(const uint8_t* sourceBuffer, size_t sourceLength, ModuleInformation& info)
> 
> I'd rather keep returning the ModuleInformation instead of passing in a reference to it: on failure it was literally impossible to have a ModuleInformation from the previous code. Now the reference can be half-valid.

The reason I did it this way is that the plan needs to maybe sometimes own the source until the ModuleInformation is initialized when it no longer does. Also, if the ModuleParser returns the ModuleInformation it's a lot easier to accidentally re-parse the module and make two copies of the module information.

[Saam Barati]

Comment on attachment 306222 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        is likely, not a problem because:

no comma

> Source/JavaScriptCore/wasm/WasmModuleInformation.h:31
> +#include <wtf/RefCounted.h>

I think you want ThreadSafeRefCounted here. Or I guess you can remove the include since it looks like it's doing nothing that you need.

> Source/JavaScriptCore/wasm/WasmModuleInformation.h:47
> +        ? importFunctionSignatureIndices[functionIndex]
> +        : internalFunctionSignatureIndices[functionIndex - importFunctionSignatureIndices.size()];

Please indent.

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:100
> +    return { };

Why this change?

> Source/JavaScriptCore/wasm/WasmParser.h:166
> +    memcpy(result.data(), source() + m_offset, stringLength);

nit: you can use string start here, right?

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:170
> +    Ref<Plan> plan = adoptRef(*new Plan(vm, makeRef(const_cast<Wasm::ModuleInformation&>(module->moduleInformation())), Plan::FullCompile, [promise, instance, module, entries] (Plan& p) {

why not just return non const instead of us lying about const-ness?

[Saam Barati]

also, looks like you need to edit cmake info.

[Keith Miller]

Comment on attachment 306222 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:13
>> +        is likely, not a problem because:
> 
> no comma

fixed.

>> Source/JavaScriptCore/wasm/WasmModuleInformation.h:31
>> +#include <wtf/RefCounted.h>
> 
> I think you want ThreadSafeRefCounted here. Or I guess you can remove the include since it looks like it's doing nothing that you need.

removed.

>> Source/JavaScriptCore/wasm/WasmModuleInformation.h:47
>> +        : internalFunctionSignatureIndices[functionIndex - importFunctionSignatureIndices.size()];
> 
> Please indent.

fixed. I'm surprised the style checker didn't catch this.

>> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:100
>> +    return { };
> 
> Why this change?

See the discussion with JF below.

>> Source/JavaScriptCore/wasm/WasmParser.h:166
>> +    memcpy(result.data(), source() + m_offset, stringLength);
> 
> nit: you can use string start here, right?

fixed.

>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:170
>> +    Ref<Plan> plan = adoptRef(*new Plan(vm, makeRef(const_cast<Wasm::ModuleInformation&>(module->moduleInformation())), Plan::FullCompile, [promise, instance, module, entries] (Plan& p) {
> 
> why not just return non const instead of us lying about const-ness?

I didn't want other places to accidentally modify the ModuleInformation but we need to change the ref count here so I have to do a const_cast :/

[Keith Miller]

Created attachment 306231 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 306231 [details]:

media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html bug 168317 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 306231 [details]
Patch for landing

Clearing flags on attachment: 306231

Committed r214919: <http://trac.webkit.org/changeset/214919>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.