Remove special case JS function proxying from library_pthread.c #16405

kleisauke · 2022-03-02T10:01:48Z

The normal __proxy method for JS functions works fine in these cases
and avoiding the special cases reduces the dependencies of
library_pthread.c which is included in all threaded programs.

sbc100 · 2022-03-03T02:18:18Z

system/lib/pthread/library_pthread.c

-    case EM_PROXIED_CREATE_CONTEXT:
-      q->returnValue.i = emscripten_webgl_create_context(q->args[0].cp, q->args[1].vp);
-      break;
+#ifdef __EMSCRIPTEN_OFFSCREEN_CANVAS__


Creating another libc variant purely for this purpose seems rather unfortunate.

Can we leave this in unconditionally for now, and submit PR that just removes the EM_PROXIED_CREATE_CONTEXT? Then we can figure out how to somehow
incject this handler only when OFFSCREEN_CANVAS is enabled as a followup?
Perhaps we can do it via some kind of dependency injections of extension mechanism
for _do_call?

Reverted for now with commit 815f592. I'm not sure what the best way is to handle this.

sbc100 · 2022-03-03T02:20:54Z

src/library_pthread.js

-#if !MINIMAL_RUNTIME
-    '$handleException',
-#endif
-  ],


Is this unrelated?

This NFC change (commit 0f1095f) was found while working on this, but is indeed unrelated. Let me know if you want to revert that too (for now).

But this code calls the _exit C function so I don't understand how removing the dependency can be correct?

In any case please split this out into a separate change.

These dependencies are also defined in $PThread__deps, so it's just de-duplicating the deps here.

emscripten/src/library_pthread.js

Lines 9 to 18 in 0f1095f

$PThread__deps: ['_emscripten_thread_init',

'$killThread',

'$cancelThread', '$cleanupThread', '$zeroMemory',

'$ptrToString', '$spawnThread',

'_emscripten_thread_free_data',

'exit',

#if !MINIMAL_RUNTIME

'$handleException',

#endif

],

I'll revert this change for now.

sbc100 · 2022-03-03T15:51:26Z

tests/other/metadce/minimal_main_Oz_USE_PTHREADS_PROXY_TO_PTHREAD.jssize

@@ -1 +1 @@
-47521


This is a pretty nice size reduction!

I managed to reduce it even more with commit b3beb21.

system/lib/pthread/library_pthread.c

src/library_pthread.js

sbc100

Can you give this PR the correct title and description and then I think we can land this.

kleisauke · 2022-03-04T10:16:46Z

PR description and title were updated as requested.

sbc100

Nice!

src/library_html5.js

tools/deps_info.py

The normal `__proxy` method for JS functions works fine in these cases and avoiding the special cases reduces the dependencies of `library_pthread.c` which is included in all threaded programs.

i.e. when building with `-sOFFSCREENCANVAS_SUPPORT=0` (default)

…pported" This reverts commit 670eecd.

The dependency wrangling here was a workaround for a problem that was solved in #16405 (see the command that is being deleted). Pretty much all the functions in this depend either directly or indirectly on the global GL object, so it makes sense to use autoAddDeps rather than try to add individual deps.

This family of function was not used anywhere can not currently usable externally. The core function `emscripten_sync_run_in_main_thread` was not usable since the its first argument is `em_queued_call` which is not struct that is declared externally. The `emscripten_sync_run_in_main_thread_N` function all take a function index as arg0 which is a index in to the JS function table which is not known statically, and is an implementation of detail of JS library proxying. It looks like these function were once used by JS library code but the last remaining use of them was removed in #16405.

This family of function was not used anywhere and is not currently usable externally. The core function `emscripten_sync_run_in_main_thread` was not usable since the its first argument is `em_queued_call` which is not struct that is declared externally. The `emscripten_sync_run_in_main_thread_N` function takes a function enum as arg0 which is not designed to be user facing. It looks like these function were once used by JS library code but the last remaining use of them was removed in #16405. See: #15756

…18580) This family of functions was not used anywhere within the codebase and is not currently usable externally. The core function `emscripten_sync_run_in_main_thread` was not usable since the its first argument is `em_queued_call` which is not struct that is declared externally. The `emscripten_sync_run_in_main_thread_N` functions takes a function enum as arg0 which is not designed to be user facing. It looks like these function were once used by JS library code but the last remaining use of them was removed in #16405. See: #15756

The dependency wrangling here was a workaround for a problem that was solved in #16405 (see the command that is being deleted). Pretty much all the functions in this depend either directly or indirectly on the global GL object, so it makes sense to use autoAddDeps rather than try to add individual deps.

kleisauke mentioned this pull request Mar 2, 2022

Wasm workers #12833

Merged

kleisauke force-pushed the remove_special_case_proxying branch 2 times, most recently from bf4e1b0 to 0f1095f Compare March 2, 2022 15:24

sbc100 reviewed Mar 3, 2022

View reviewed changes

sbc100 reviewed Mar 4, 2022

View reviewed changes

src/library_pthread.js Outdated Show resolved Hide resolved

sbc100 approved these changes Mar 4, 2022

View reviewed changes

kleisauke changed the title ~~Rebased version of PR #14912~~ Ensure library_pthread.c does not include more JS dependencies than necessary Mar 4, 2022

kleisauke force-pushed the remove_special_case_proxying branch from 82751c5 to 495f493 Compare March 4, 2022 10:10

sbc100 reviewed Mar 4, 2022

View reviewed changes

src/library_html5.js Outdated Show resolved Hide resolved

tools/deps_info.py Show resolved Hide resolved

sbc100 and others added 7 commits March 6, 2022 12:59

Remove special case JS function proxying from library_pthread.c

930c23d

The normal `__proxy` method for JS functions works fine in these cases and avoiding the special cases reduces the dependencies of `library_pthread.c` which is included in all threaded programs.

Simplify preprocessor directive and update comments

bff66f4

Add missing dependency to emscripten_proxy_to_main_thread_js

6051290

Stub-out EM_PROXIED_RESIZE_OFFSCREENCANVAS case when not supported

670eecd

i.e. when building with `-sOFFSCREENCANVAS_SUPPORT=0` (default)

Incorporate review comment

ccc2d8e

Revert "Stub-out EM_PROXIED_RESIZE_OFFSCREENCANVAS case when not su…

74468ca

…pported" This reverts commit 670eecd.

Remove unused REQUIRED_EXPORTS

67bac98

kleisauke force-pushed the remove_special_case_proxying branch from 495f493 to 67bac98 Compare March 6, 2022 12:18

kleisauke changed the title ~~Ensure library_pthread.c does not include more JS dependencies than necessary~~ Remove special case JS function proxying from library_pthread.c Mar 6, 2022

sbc100 approved these changes Mar 6, 2022

View reviewed changes

sbc100 merged commit a18bbf7 into emscripten-core:main Mar 6, 2022

kleisauke deleted the remove_special_case_proxying branch March 6, 2022 16:42

kleisauke mentioned this pull request Mar 6, 2022

build with pthreads always includes library_webgl.js #11805

Closed

sbc100 mentioned this pull request Jan 23, 2023

Remove unused (and unusable) emscripten_sync_run_in_main_thread API. #18580

Merged

sbc100 mentioned this pull request Apr 18, 2023

Add explicit GL dependency to library_html5_webgl #19196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove special case JS function proxying from library_pthread.c #16405

Remove special case JS function proxying from library_pthread.c #16405

Uh oh!

kleisauke commented Mar 2, 2022 •

edited

Loading

Uh oh!

sbc100 Mar 3, 2022

Uh oh!

kleisauke Mar 3, 2022

Uh oh!

sbc100 Mar 3, 2022 •

edited

Loading

Uh oh!

kleisauke Mar 3, 2022

Uh oh!

sbc100 Mar 3, 2022

Uh oh!

kleisauke Mar 3, 2022

Uh oh!

sbc100 Mar 3, 2022

Uh oh!

kleisauke Mar 4, 2022

Uh oh!

Uh oh!

Uh oh!

sbc100 left a comment

Uh oh!

kleisauke commented Mar 4, 2022

Uh oh!

sbc100 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	$PThread__deps: ['_emscripten_thread_init',
	'$killThread',
	'$cancelThread', '$cleanupThread', '$zeroMemory',
	'$ptrToString', '$spawnThread',
	'_emscripten_thread_free_data',
	'exit',
	#if !MINIMAL_RUNTIME
	'$handleException',
	#endif
	],

		@@ -1 +1 @@
		47521

Remove special case JS function proxying from library_pthread.c #16405

Remove special case JS function proxying from library_pthread.c #16405

Uh oh!

Conversation

kleisauke commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

kleisauke Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

kleisauke Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

kleisauke Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

kleisauke commented Mar 4, 2022

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kleisauke commented Mar 2, 2022 •

edited

Loading

sbc100 Mar 3, 2022 •

edited

Loading