Bug 149093 - Implement calls to JavaScript functions in WebAssembly
Summary: Implement calls to JavaScript functions in WebAssembly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 146064
  Show dependency treegraph
 
Reported: 2015-09-12 13:45 PDT by Sukolsak Sakshuwong
Modified: 2015-09-15 13:00 PDT (History)
7 users (show)

See Also:


Attachments
Patch (25.90 KB, patch)
2015-09-12 15:20 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (20.38 KB, patch)
2015-09-15 04:18 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-09-12 13:45:42 PDT
Implement calls to JavaScript functions in WebAssembly
Comment 1 Sukolsak Sakshuwong 2015-09-12 15:20:55 PDT
Created attachment 261064 [details]
Patch
Comment 2 Filip Pizlo 2015-09-14 18:19:04 PDT
Comment on attachment 261064 [details]
Patch

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

> Source/JavaScriptCore/wasm/WASMModuleParser.cpp:53
> +    : m_exec(exec)

We should avoid doing this.  ExecState* really means a frame pointer, so it's only valid during execution, and using it here can cause three other problems:
1) Impossible to make this code run concurrently.
2) Too easy to introduce user-observable effects.  We usually only pass around ExecState* when we're in a context where it's OK to have user-observed effects.
3) Too easy to make the parser impure - i.e. somehow dependent on the current state of the world.
Comment 3 Yusuke Suzuki 2015-09-14 22:54:32 PDT
Comment on attachment 261064 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WASMModuleParser.cpp:53
>> +    : m_exec(exec)
> 
> We should avoid doing this.  ExecState* really means a frame pointer, so it's only valid during execution, and using it here can cause three other problems:
> 1) Impossible to make this code run concurrently.
> 2) Too easy to introduce user-observable effects.  We usually only pass around ExecState* when we're in a context where it's OK to have user-observed effects.
> 3) Too easy to make the parser impure - i.e. somehow dependent on the current state of the world.

Currently, in asm.js[1] and our WebAssembly implementation, we use the simple object as the set of the exported names.
So to look up the property from it, we need ExecState*... (I don't think adding new method getOwnPropertySlot(VM& ... ) to a lot of JSCell's derived class is good.)
But seeing the asm.js spec, it says that "all property access must resolve to data properties;".
So getOwnPropertySlot does not cause any observable effect if we check the slot with `slot.isValue()` and retrieve the value.
What do you think of

1. Not storing ExecState* to the member field. It's transient value, so storing to the member is not good. Passing it as the parameter is better.
2. Using getOwnPropertySlot and ExecState* and writing the comment about "The imported binding should be data property. We only retrieve the data property. So it does not cause any observable effect".
3. We should not use ExecState* except for getImportedValue.

And I believe this code will be replaced with the other one in the future, because

1. I believe that WebAssembly will be integrated into ES6 module loader. At that time, we don't use the object to link the functions. (currently, nothing about WebAssembly + Module is defined yet)
2. When the proxy is introduced, even `getOwnPropertySlot` becomes observable (Even [[HasProperty]] operation can throw errors!). So asm.js' linking semantics will be recasted. Or WebAssembly will replace  this semantics.

[1]: http://asmjs.org/spec/latest/
Comment 4 Sukolsak Sakshuwong 2015-09-15 04:18:38 PDT
Created attachment 261186 [details]
Patch
Comment 5 Sukolsak Sakshuwong 2015-09-15 04:45:10 PDT
Thanks for the comments. I have made the following changes:

- Use only data properties. This should not cause any user-observable effect.

- Pass ExecState* as a parameter. Since we use only data properties, we shouldn't have to use ExecState*. But this seems to require a lot of code changes and is probably not worth the effort as there are no details about how WASM imports work yet.

- The second argument to loadWebAssembly() is not required if the module doesn't import any value.
Comment 6 WebKit Commit Bot 2015-09-15 13:00:38 PDT
Comment on attachment 261186 [details]
Patch

Clearing flags on attachment: 261186

Committed r189822: <http://trac.webkit.org/changeset/189822>
Comment 7 WebKit Commit Bot 2015-09-15 13:00:42 PDT
All reviewed patches have been landed.  Closing bug.