Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Suggestion] [sokol_app] proxy javascript function accessing DOM to the main thread #999

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dvad
Copy link

@Dvad Dvad commented Feb 23, 2024

This is the PR mentioned in #997 and is the last change I had locally that I would like to suggest upstream.

What does this PR do?

This commit changes the javascript functions defined by sokol_app.h to allow proxying them to the main thread when necessary and enable the use of the emscripten proxy_to_pthread option. Specifically, it replaces a few call to the EM_JS macro to the EM_JS_MAIN_THREAD

What problem does it solve?

When compiled with proxy_to_pthread (generally recommended by emscripten documentation), the "main" app thread is running separately from the browser event loop. This guarantee the browser won't become unresponsive because of a deadlock. This option is particularly convenient when porting code that might block for any event on the main thread.

With this option, the main app thread is not able to access a few javascript features such as the DOM window object. The emscripten solution for that is called proxyinghttps://emscripten.org/docs/porting/pthreads.html#proxying), where javascript function calls are moved to the browser event loop and the return value eventually sent back to the worker thread.

Potential caveat

Since there is no utility EM_JS_MAIN_THREAD in emscripten (that would define a javascript function callable from c++ proxied to main), I had to define the corresponding function in pure C and make them call inline asm via the MAIN_THREAD_EM_ASM.
Example of such a change:

EM_JS(void, sapp_js_remove_dragndrop_listeners, (const char* canvas_name_cstr), {
...
}

becomes

void sapp_js_remove_dragndrop_listeners(const char* canvas_name_cstr) {
    MAIN_THREAD_EM_ASM({
      ...
    });
}

This means that the generated function in the javascript side won't be named with the name defined in C, instead they will have random name.

Hence, if those function were intended to be called from the javascript side, this PR would be a problem. I don't see any call like this so far, but the fact that they were not prefixed with an underscore, as other internal function, make me doubt.

## What does this PR do
This commit changes the javascript defined by sokol to allow proxying them when necessary and enable the use of the proxy_to_pthread option.
Specificallt, it replaces a few call to the `EM_JS` macro to the `EM_JS_MAIN_THREAD`

## What problem does it solve?
When compiled with `proxy_to_pthread` ([generally recommended](https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread) by emscripten documentation), the "main" app thread is running separately from the browser event loop. This guarantee the browser won't become unresponsive because of a deadlock.
This option is particularly convenient when porting code that might block for any event on the main thread.

With this option, the main app thread is not able to access a few javascript features such as the `window` object. The solution for that is called [proxying]()https://emscripten.org/docs/porting/pthreads.html#proxying), where javascript function calls are move to the main thread and proxied on the worker thread.

# Potential caveat
Since there is no utility `EM_JS_MAIN_THREAD` in emscripten (that would define a javascript function callable from c++ proxied to main), I had to define the corresponding function in C and make them call inline asm via the `MAIN_THREAD_EM_ASM`. This means that the generated function in the javascript side won't be named with the name defined in C, instead they will have random name.

Hence, if those function were intended to be called from the javascript side, this PR would be a problem. I don't see any call like this so far, but the fact that they were not profixed with an underscore as other internal function make me doubt.
@floooh
Copy link
Owner

floooh commented Feb 24, 2024

Hmm, at first glance this change is a bit too radical for my liking. Do you know if MAIN_THREAD_EM_ASM intoduces overhead if the C side is already running on the main thread?

@Dvad
Copy link
Author

Dvad commented Feb 25, 2024

I suspected this would require a bit more discussion, hence why I kept this PR for the end. :)

I think the ability to run with the proxy to pthread option is valuable as it actually eliminates a few nasty gotchas in multithreaded emscripten builds and make the behavior of the web version more similar to the native build.

Unfortunately, I don't have good answer for the overhead, and did not find anything on the emscripten github repo before posting the PR.

I find the emscripten comment on MAIN_THREAD_EM_ASM is informative :

// Runs the given JavaScript code synchronously on the main browser thread, and returns no value back.
// Call this function for example to access DOM elements in a pthread when building with -s USE_PTHREADS=1.
// Avoid calling this function in performance sensitive code, because this will effectively sleep the
// calling thread until the main browser thread is able to service the proxied function call. If you have
// multiple MAIN_THREAD_EM_ASM() code blocks to call in succession, it will likely be much faster to
// coalesce all the calls to a single MAIN_THREAD_EM_ASM() block. If you do not need synchronization nor
// a return value back, consider using the function MAIN_THREAD_ASYNC_EM_ASM() instead, which will not block.
// In single-threaded builds (including proxy-to-worker), MAIN_THREAD_EM_ASM*()
// functions are direct aliases to the corresponding EM_ASM*() family of functions.
#define MAIN_THREAD_EM_ASM(code, ...)

Another informative bit is looking at the generated javascript for the inline ASM call:

var runMainThreadEmAsm = (code, sigPtr, argbuf, sync) => {
 var args = readEmAsmArgs(sigPtr, argbuf);
 if (ENVIRONMENT_IS_PTHREAD) {
  return proxyToMainThread.apply(null, [ -1 - code, sync ].concat(args));
 }
 return ASM_CONSTS[code].apply(null, args);
};

So, In term of overhead I see two potential sources:

  1. From the proxying part: the fact that now the javascript part will start by checking if we are already on the main thread, if so it call the function directly in a sychronous way. This overhead is zero in non pthread build.
  2. From the inline ASM vs EM_JS part: the javascript function is now a field of the ASM_CONSTS table with a code number, so calling it introduce an extra indirection.

I think if 2 is a problem, another option could be to detect and dispatch on the C side instead and avoid inline ASM:

EM_JS(void, cfunc_impl, (void));

void cfunc(void) {
  if (!emscripten_is_main_runtime_thread()) {
    cfunc_impl();
  } else {
    emscripten_sync_run_in_main_runtime_thread(SIG, cfunc_impl, args)
  }
}

but honestly I don't know enough about the internal of emscripten and javascript to see if that could lead to a measurable difference overhead.

Note that I realized my PR is incomplete, but my guess was that the functions needing to be changed were not performance sensitive:

void sapp_js_add_beforeunload_listener()
void sapp_js_remove_beforeunload_listener() 
void sapp_js_add_clipboard_listener()
void sapp_js_remove_clipboard_listener()
void sapp_js_write_clipboard(const char* c_str)
void sapp_js_add_dragndrop_listeners(const char* canvas_name_cstr)
void sapp_js_remove_dragndrop_listeners(const char* canvas_name_cstr)
void sapp_js_init(const char* c_str_target)

although my current implementation is likely not the most efficient and could be improved either with MAIN_THREAD_ASYNC_EM_ASM when correct or with the c version above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants