Skip to content

Always fixup custom __proxy attributes in library_html5_webgl.js #20135

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 28, 2023
Merged

Always fixup custom __proxy attributes in library_html5_webgl.js #20135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5ed028
Always fixup custom `__proxy` attributes in library_html5_webgl.js
sbc100 Aug 25, 2023
d7b7d5a
Update src/library_html5_webgl.js
sbc100 Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/jsifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,12 +225,12 @@ function(${args}) {
i53ConversionDeps.forEach((d) => deps.push(d))
}

if (SHARED_MEMORY) {
const proxyingMode = LibraryManager.library[symbol + '__proxy'];
if (proxyingMode) {
if (proxyingMode !== 'sync' && proxyingMode !== 'async') {
throw new Error(`Invalid proxyingMode ${symbol}__proxy: '${proxyingMode}' specified!`);
}
const proxyingMode = LibraryManager.library[symbol + '__proxy'];
if (proxyingMode) {
if (proxyingMode !== 'sync' && proxyingMode !== 'async') {
throw new Error(`Invalid proxyingMode ${symbol}__proxy: '${proxyingMode}' specified!`);
}
if (SHARED_MEMORY) {
const sync = proxyingMode === 'sync';
if (PTHREADS) {
snippet = modifyJSFunction(snippet, (args, body, async_, oneliner) => {
Expand Down
12 changes: 9 additions & 3 deletions src/library_html5_webgl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,13 +514,13 @@ var LibraryHtml5WebGL = {
emscripten_webgl_get_parameter_i64v: (param, dst) => writeI53ToI64(dst, GLctx.getParameter(param)),
};

#if PTHREADS
function handleWebGLProxying(funcs) {
#if SHARED_MEMORY
// Process 'sync_on_webgl_context_handle_thread' and 'sync_on_current_webgl_context_thread' pseudo-proxying modes
// to appropriate proxying mechanism, either proxying on-demand, unconditionally, or never, depending on build modes.
// 'sync_on_webgl_context_handle_thread' is used for function signatures that take a HTML5 WebGL context handle
// object as the first argument. 'sync_on_current_webgl_context_thread' is used for functions that operate on the
// implicit "current WebGL context" as activated via emscripten_webgl_make_current() function.
function handleWebGLProxying(funcs) {

function listOfNFunctionArgs(func) {
var args = [];
Expand Down Expand Up @@ -586,10 +586,16 @@ function handleWebGLProxying(funcs) {
delete funcs[i + '__proxy'];
}
}
#else
// In single threaded mode just delete our custom __proxy addributes, otherwise
// they will causes errors in the JS compiler.
for (var i in funcs) {
delete funcs[i + '__proxy'];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awkward, no? Wouldn't all other libraries besides library_htm5_webgl.js want to run this functionality as well to ignore proxy attributes?

Ideally for Wasm Workers, the __proxy mechanism should be ignored if function is called from main thread, and in assertions builds, if a proxied JS function is called from a Wasm Worker, the code would raise an error, and in release builds simply truck through?

That is what this code

emscripten/src/jsifier.js

Lines 247 to 255 in 3a55fe1

} else if (WASM_WORKERS && ASSERTIONS) {
// In ASSERTIONS builds add runtime checks that proxied functions are not attempted to be called in Wasm Workers
// (since there is no automatic proxying architecture available)
snippet = modifyJSFunction(snippet, (args, body) => `
function(${args}) {
assert(!ENVIRONMENT_IS_WASM_WORKER, "Attempted to call proxied function '${mangled}' in a Wasm Worker, but in Wasm Worker enabled builds, proxied function architecture is not available!");
${body}
}\n`);
}
attempts to do, so the above change should be redundant if the linked code works as intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code only runs if !SHARED_MEMORY so when building with wasm workers or pthreads this code doesn't run and the attributes are not ignored.

I agree this could probably use a comment. The issue is that library_htm5_webgl.js uses custom __proxy addribute sync_on_webgl_context_handle_thread which needs to get either re-written (above), or removed (here) before the library is processed, otherwise we get an Invalid proxyingMode jsifier.

Copy link
Collaborator Author

@sbc100 sbc100 Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I did already include a comment as to why this done. Does it make sense now, given my explanation?

My initial version of this attempted to relax the __proxy checking but @kripken suggested we always remove / rewrite the custom values instead: #20135 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code only runs if !SHARED_MEMORY

I am confused. Taking a closer look, the code runs if

if (SHARED_MEMORY) {
  if (PTHREADS) {
    // ...
  } else if (WASM_WORKERS && ASSERTIONS) {
    // emit "assert(!ENVIRONMENT_IS_WASM_WORKER"
  }
}

but I think I wrote that when I still had the assumption that PTHREADS and WASM_WORKERS would always be mutually exclusive and you'd only build targeting one or the other - but turns out targeting both at the same time made sense.

So that above code should instead be

if (SHARED_MEMORY) {
  if (PTHREADS) {
    // ...
  }
  if (WASM_WORKERS && ASSERTIONS) {
    // emit "assert(!ENVIRONMENT_IS_WASM_WORKER"
  }
}

so that in PTHREADS + WASM_WORKERS builds the assertion is also added. Would that appropriately resolve the issue?

I still think no library_x.js file should need to mutate the __proxy fields depending on the build mode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see what you are saying. We should probably not erase the __proxy attributes in the wasm workers build. I will create a followup to do that.

I agree the custom proxy stuff in library_html5_webgl.js is unusual but it looks like you were the one that created this mechanism back in #11356, and I think its OK as long as it get locally handled/erased.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I mis-remembered, I already did do that in this PR, that __proxy handling is enabled here for all SHARED_MEMORY builds.

I think updating jsifier.js in way you suggest sounds fine to me. That seems like a pre-existing issue that is somewhat unrelated to this PR and would effect all libraries, right?

Are you happy with this PR in the way it deals with library_html5_webgl.js?

Copy link
Collaborator

@juj juj Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a pre-existing issue that is somewhat unrelated to this PR and would effect all libraries, right?

Yeah, I think it would be very preferable to have that affect all libraries, so that any developer who is calling a __proxy: sync function inside a Wasm Worker would get an assertion firing on them. That is because I don't think any function with that label would work properly when called in the context of a Wasm Worker.

}
#endif // SHARED_MEMORY
}

handleWebGLProxying(LibraryHtml5WebGL);
#endif // PTHREADS

#if LibraryManager.has('library_webgl.js')
autoAddDeps(LibraryHtml5WebGL, '$GL');
Expand Down