Bug 147222 - Implement WebAssembly modules
Summary: Implement WebAssembly modules
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 146064
  Show dependency treegraph
 
Reported: 2015-07-23 01:01 PDT by Sukolsak Sakshuwong
Modified: 2015-07-23 19:37 PDT (History)
5 users (show)

See Also:


Attachments
Patch (13.65 KB, patch)
2015-07-23 01:06 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (14.33 KB, patch)
2015-07-23 12:05 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Remove m_arrayBuffer for now as per discussion. (11.54 KB, patch)
2015-07-23 12:52 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Use jsCast instead of static_cast. (11.53 KB, patch)
2015-07-23 13:11 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (1.47 KB, patch)
2015-07-23 16:47 PDT, Sukolsak Sakshuwong
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff
Fix ChangeLog (1.53 KB, patch)
2015-07-23 18:27 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Fix ChangeLog (1.38 KB, patch)
2015-07-23 18:32 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sukolsak Sakshuwong 2015-07-23 01:01:28 PDT
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
Comment 1 Sukolsak Sakshuwong 2015-07-23 01:06:09 PDT
Created attachment 257342 [details]
Patch
Comment 2 Sukolsak Sakshuwong 2015-07-23 12:05:16 PDT
Created attachment 257359 [details]
Patch
Comment 3 Mark Lam 2015-07-23 12:19:09 PDT
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?
Comment 4 Mark Lam 2015-07-23 12:45:04 PDT
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.
Comment 5 Sukolsak Sakshuwong 2015-07-23 12:52:01 PDT
Created attachment 257366 [details]
Remove m_arrayBuffer for now as per discussion.
Comment 6 Mark Lam 2015-07-23 12:59:12 PDT
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);
Comment 7 Sukolsak Sakshuwong 2015-07-23 13:11:01 PDT
Created attachment 257370 [details]
Use jsCast instead of static_cast.
Comment 8 Mark Lam 2015-07-23 13:38:16 PDT
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.
Comment 9 WebKit Commit Bot 2015-07-23 14:28:03 PDT
Comment on attachment 257370 [details]
Use jsCast instead of static_cast.

Clearing flags on attachment: 257370

Committed r187254: <http://trac.webkit.org/changeset/187254>
Comment 10 WebKit Commit Bot 2015-07-23 14:28:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Geoffrey Garen 2015-07-23 16:23:13 PDT
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.
Comment 12 Sukolsak Sakshuwong 2015-07-23 16:47:00 PDT
Reopening to attach new patch.
Comment 13 Sukolsak Sakshuwong 2015-07-23 16:47:02 PDT
Created attachment 257407 [details]
Patch
Comment 14 Geoffrey Garen 2015-07-23 16:50:09 PDT
Comment on attachment 257407 [details]
Patch

r=me

It looks like you need to fixup the changelog entry.
Comment 15 Mark Lam 2015-07-23 16:50:37 PDT
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.
Comment 16 Sukolsak Sakshuwong 2015-07-23 18:27:35 PDT
Created attachment 257416 [details]
Fix ChangeLog
Comment 17 Sukolsak Sakshuwong 2015-07-23 18:32:13 PDT
Created attachment 257417 [details]
Fix ChangeLog
Comment 18 WebKit Commit Bot 2015-07-23 19:36:59 PDT
Comment on attachment 257417 [details]
Fix ChangeLog

Clearing flags on attachment: 257417

Committed r187283: <http://trac.webkit.org/changeset/187283>
Comment 19 WebKit Commit Bot 2015-07-23 19:37:03 PDT
All reviewed patches have been landed.  Closing bug.