Multithreading 6/N: Proxy calls to JavaScript functions from pthreads to main browser thread by juj · Pull Request #5531 · emscripten-core/emscripten

juj · 2017-08-26T19:10:44Z

This PR implements a mechanism where JavaScript library functions can be attributed with foo__proxy: 'mode', where mode is one of 'main', 'async', 'main_gl' or 'async_gl'. This allows a pthread running in a web worker to either synchronously or asynchronously call out to JavaScript on the main browser thread.

The _gl variants decide proxying based on the currently active WebGL context for the calling thread. If the calling thread owns the GL context, then the call is not proxied. If the calling thread instead has activated a proxied context that is owned by the main browser thread, then the call is proxied. (This machinery is expanded on along with tests in a later PR)

This PR requires #5527 to land first in order to work.

etienne02 · 2017-09-06T12:26:20Z

src/library_pthread.js

+            var returnValue;
+            var funcTable = (e.data.func >= 0) ? proxiedFunctionTable : ASM_CONSTS;
+            var funcIdx = (e.data.func >= 0) ? e.data.func : (-1 - e.data.func);
+            PThread.currentProxiedOperationCallerThread = worker.pthread.threadInfoStruct; // Sometimes we need to backproxy events to the calling thread (e.g. HTML5 DOM events handlers such as emscripten_set_mousemove_callback()), so keep track in a globally accessible variable about the thread that initiated the proxying.


Here I have an exception : worker.pthread is undefined, when a proxy call is lanch but the worker terminate and is clean.

Any chance you'd be able to make a small test case?

I test to reproduce it in a small test case but for now, i'm not able. I will continue to try.

kripken

Are the 4 proxying modes documented? the names suggest what they do, but I'm not 100% sure.

kripken · 2017-09-06T22:38:52Z

src/jsifier.js

+
+    // This is ad-hoc numbering scheme to map signatures to IDs, and must agree with call handler in src/library_pthread.js.
+    // Once proxied function calls no longer go through postMessage()s but instead in the heap, this will need to change, since int vs float will matter.
+    var functionCallOrdinal = sig.length + (sizeofReturn == 8 ? 20 : 0);


what is magical about 8 and 20?

sizeOfReturn being 8 equals the case of a double, and the ID 20 is where functions with double return type start. The comment refers to the matching IDs at src/library_pthread.js: https://github.com/kripken/emscripten/pull/5531/files#diff-db41bea94577c2dd9b0eef0308b06cf9R272

Changed sizeOfReturn == 8 test to sig[0] == 'd' to be more self-documenting.

juj · 2017-09-07T08:50:50Z

I'll need to write a full set of documentation about the updated pthreads support. I'd like to defer that to once these PRs land, so that I can write a coherent update of the current doc page at http://kripken.github.io/emscripten-site/docs/porting/pthreads.html

kripken · 2017-09-11T18:42:25Z

src/jsifier.js

+
+    // This is ad-hoc numbering scheme to map signatures to IDs, and must agree with call handler in src/library_pthread.js.
+    // Once proxied function calls no longer go through postMessage()s but instead in the heap, this will need to change, since int vs float will matter.
+    var functionCallOrdinal = sig.length + (sig[0] == 'd' ? 20 : 0);


i still think the 20 is bad - couldn't that become invalid if we change memory layout? or am i missing something?

at minimum lets put a big comment with XXX FIXME on this.

The number 20 is more like a magic ID, it only needs to match in this spot in src/jsifier.js and then in src/pthread-main.js (in https://github.com/kripken/emscripten/pull/5531/files#diff-db41bea94577c2dd9b0eef0308b06cf9R272). There is nothing about the memory layout itself that would interact with this. Only limit to number 20 is if there's a function that needs proxying that would take in more than 20 input parameters, then we'd have to bump up the number.

Basically this is turning all these signature strings 'vii', 'viii', 'iii', 'dii' into ID numbers, so that the receiving code knows what kind of function signature it should be calling.

I see, so it just cares about the # of params and whether the return value is float? maybe add a comment with that?

also, how about storing 20 in a const, and also asserting that the sig.length is shorter than it?

Good suggestions, improved this. Also while reading, found a yet unimplemented case in handling there, fixed that, and also changed the number from 20 to 32 to be more programmer-friendly :)

kripken · 2017-09-11T18:43:07Z

Bigger docs later sounds ok, but just for reviewing this PR, I could use some short explanations :)

juj · 2017-09-13T09:43:17Z

Edited the PR to remove the main_gl and async_gl proxying modes for now. This is because I'm looking at redoing the WebGL proxying via a more efficient method, by creating a WebGL thread. Depending on how that goes, I'll look at reintroducing main_gl and async_gl, whichever makes more sense.

juj · 2017-09-13T09:45:25Z

Bigger docs later sounds ok, but just for reviewing this PR, I could use some short explanations :)

The main proxying mode queues the function call on the main browser thread, and the calling thread then synchronously waits for the function call to finish on the main thread, and gets the return value back.

The async proxying mode queues the function call on the main browser thread, but immediately continues to execute code without waiting for the function call to happen, or the return value, if the function had any.

kripken · 2017-09-13T19:19:25Z

I see, thanks. Why not call it sync and async, then, instead of main and async?

…ction calls to main browser thread.

… a value back.

…in thread.

…e.data' to 'd' since it is accessed awfully many times in library_pthread.js.

juj · 2017-09-22T09:36:41Z

I see, thanks. Why not call it sync and async, then, instead of main and async?

Hmm, yeah, I first started with main, and then added main_async, and then thought that's too long, and shortened it to async. But sync and async sound like better, I'll change to that.

juj · 2017-09-22T10:47:01Z

Updated the PR, how does it look now?

kripken

lgtm with one style nit

kripken · 2017-09-22T18:11:44Z

src/jsifier.js

+
+    function sizeofType(t) {
+      switch(t) {
+        case 'd':


please do case bodies on same line, or {,}

juj added the multithreading label Aug 26, 2017

This was referenced Aug 26, 2017

Multithreading 7/N: Add support to calling SDL 1.x from pthreads #5532

Merged

Multithreading 8/N: Add support to calling OpenAL from pthreads #5533

Merged

Multithreading 9/N: MAIN_THREAD_EM_ASM() #5544

Merged

etienne02 reviewed Sep 6, 2017

View reviewed changes

kripken reviewed Sep 6, 2017

View reviewed changes

juj force-pushed the proxy_to_main_thread branch 2 times, most recently from 4b8ddf2 to 6945de2 Compare September 8, 2017 08:50

kripken reviewed Sep 11, 2017

View reviewed changes

juj force-pushed the proxy_to_main_thread branch from 6945de2 to 7a2f474 Compare September 13, 2017 09:39

juj force-pushed the proxy_to_main_thread branch from 7a2f474 to cca54d2 Compare September 22, 2017 09:31

juj added 4 commits September 22, 2017 12:34

Implement a model for synchronously or asynchronously proxying JS fun…

865e19a

…ction calls to main browser thread.

Add test for synchronously proxying a call to main thread and getting…

198b2c6

… a value back.

Add a test for asynchronously proxying calls from a pthread to the ma…

0bf4a88

…in thread.

Improve handling of proxied function returning double, and contract '…

244d10d

…e.data' to 'd' since it is accessed awfully many times in library_pthread.js.

juj force-pushed the proxy_to_main_thread branch from cca54d2 to 244d10d Compare September 22, 2017 09:39

kripken approved these changes Sep 22, 2017

View reviewed changes

Apply style cleanup to switch-case in jsifier.js

8559efe

juj merged commit 5bcb75e into emscripten-core:incoming Sep 22, 2017

Conversation

juj commented Aug 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juj commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Sep 11, 2017

Uh oh!

juj commented Sep 13, 2017

Uh oh!

juj commented Sep 13, 2017

Uh oh!

kripken commented Sep 13, 2017

Uh oh!

juj commented Sep 22, 2017

Uh oh!

juj commented Sep 22, 2017

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants