Bug 147222 – Implement WebAssembly modules

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

[Sukolsak Sakshuwong]

Implement WebAssembly modules. See https://bugs.webkit.org/show_bug.cgi?id=146064#c2 and https://github.com/WebAssembly/design/blob/master/MVP.md#linear-memory

[Sukolsak Sakshuwong]

Created attachment 257342 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 257359 [details]
Patch

[Mark Lam]

Comment on attachment 257359 [details]
Patch

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

> Source/JavaScriptCore/wasm/JSWASMModule.h:69
> +    WriteBarrier<JSArrayBuffer> m_arrayBuffer;

What is this m_arrayBuffer for?  Is it the buffer to hold the WASM source that we load?  If so, does it really need to be a JS property is accessible from JS code?

[Mark Lam]

Comment on attachment 257359 [details]
Patch

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

The patch looks good to me in general, but I think we should remove the m_arrayBuffer field until we know how it will be used.

> Source/JavaScriptCore/ChangeLog:7
> +

Please add a short comment here like:
"Introducing the boiler plate data structure for the WebAssembly module.  WASM functionality will be added in a subsequent patch."

>> Source/JavaScriptCore/wasm/JSWASMModule.h:69
>> +    WriteBarrier<JSArrayBuffer> m_arrayBuffer;
> 
> What is this m_arrayBuffer for?  Is it the buffer to hold the WASM source that we load?  If so, does it really need to be a JS property is accessible from JS code?

I spoke with Sukol offline.  This buffer is potentially needed for some analog of the passed in buffer in the "Putting It All Together" section of the asm.js spec: http://asmjs.org/spec/latest/.  However, it isn't clear from the spec how this would really work yet.  I think it's better to not include this m_arrayBuffer for now until we more concrete details.  In contrast, the m_functions is fine because it is clear that we'll storing the instantiated WASM functions there.

[Sukolsak Sakshuwong]

Created attachment 257366 [details]
Remove m_arrayBuffer for now as per discussion.

[Mark Lam]

Comment on attachment 257366 [details]
Remove m_arrayBuffer for now as per discussion.

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

You should probably also make the corresponding project file changes to Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj and Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters.

> Source/JavaScriptCore/wasm/JSWASMModule.cpp:40
> +    JSWASMModule* thisObject = static_cast<JSWASMModule*>(cell);

Sorry I didn't catch this the first time.  You should use a jsCast here instead:
    JSWASMModule* thisObject = jsCast<JSWASMModule*>(cell);

[Sukolsak Sakshuwong]

Created attachment 257370 [details]
Use jsCast instead of static_cast.

[Mark Lam]

Comment on attachment 257370 [details]
Use jsCast instead of static_cast.

r=me.

We'll add the vcxproj changes later since Sukol doesn't have access to VS right now.

[WebKit Commit Bot]

Comment on attachment 257370 [details]
Use jsCast instead of static_cast.

Clearing flags on attachment: 257370

Committed r187254: <http://trac.webkit.org/changeset/187254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 257370 [details]
Use jsCast instead of static_cast.

You've created an object that requires a destructor, but you haven't arranged to run the destructor.

By default, GC objects do not run destructors. One way to do this is to inherit from JSDestructibleObject.

[Sukolsak Sakshuwong]

Reopening to attach new patch.

[Sukolsak Sakshuwong]

Created attachment 257407 [details]
Patch

[Geoffrey Garen]

Comment on attachment 257407 [details]
Patch

r=me

It looks like you need to fixup the changelog entry.

[Mark Lam]

Comment on attachment 257370 [details]
Use jsCast instead of static_cast.

Unobsolete old patch as the new patch is a follow up fix, not a replacement.

[Sukolsak Sakshuwong]

Created attachment 257416 [details]
Fix ChangeLog

[Sukolsak Sakshuwong]

Created attachment 257417 [details]
Fix ChangeLog

[WebKit Commit Bot]

Comment on attachment 257417 [details]
Fix ChangeLog

Clearing flags on attachment: 257417

Committed r187283: <http://trac.webkit.org/changeset/187283>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.