Bug 170478

Summary: WebAssembly: ModuleInformation should be a ref counted thing that can be shared across threads.
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, jfbastien, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Keith Miller 2017-04-04 15:30:23 PDT
WebAssembly: ModuleInformation should be a ref counted thing that can be shared across threads.
Comment 1 Keith Miller 2017-04-04 15:40:14 PDT
Created attachment 306222 [details]
Patch
Comment 2 JF Bastien 2017-04-04 16:05:17 PDT
Does this address https://bugs.webkit.org/show_bug.cgi?id=170296 ?
Comment 3 Keith Miller 2017-04-04 16:06:49 PDT
(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.
Comment 4 JF Bastien 2017-04-04 16:20:35 PDT
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.
Comment 5 JF Bastien 2017-04-04 16:21:55 PDT
(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.
Comment 6 Keith Miller 2017-04-04 16:38:13 PDT
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.
Comment 7 Saam Barati 2017-04-04 16:40:39 PDT
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?
Comment 8 Saam Barati 2017-04-04 16:41:06 PDT
also, looks like you need to edit cmake info.
Comment 9 Keith Miller 2017-04-04 17:01:53 PDT
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 :/
Comment 10 Keith Miller 2017-04-04 17:02:17 PDT
Created attachment 306231 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2017-04-04 17:55:16 PDT
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: graouts@apple.com)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2017-04-04 17:55:58 PDT
Comment on attachment 306231 [details]
Patch for landing

Clearing flags on attachment: 306231

Committed r214919: <http://trac.webkit.org/changeset/214919>
Comment 13 WebKit Commit Bot 2017-04-04 17:56:00 PDT
All reviewed patches have been landed.  Closing bug.