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

Fix pthread worker options usage with Vite bundler #22834

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jamsinclair
Copy link

@jamsinclair jamsinclair commented Nov 2, 2024

Overview

An attempt at solving #22394.

When code using threads is compiled with Emscripten and then bundled with Vite it does not bundle and throws an error.

This is due to a variable reference for worker options being used during worker construction. Vite needs static worker options to bundle the code successfully.

As discussed in the issue, the easiest solution would be to switch to using an object literal for each worker construction.

tools/link.py Outdated Show resolved Hide resolved
@@ -426,14 +407,14 @@ var LibraryPThread = {
createScriptURL: (ignored) => new URL("{{{ TARGET_JS_NAME }}}", import.meta.url)
}
);
worker = new Worker(p.createScriptURL('ignored'), workerOptions);
worker = new Worker(p.createScriptURL('ignored'), {{{ WORKER_OPTIONS }}});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I simpler way to do this would be to define a new macro here at the top of this file. e.g.

{{{
globalThis.WORKER_OPTIONS = {
   ...
};
null;
}}}

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much! I wasn't too sure how the templating works in these files 😅 . I'll update the PR with this approach when I get a chance.

src/library_pthread.js Outdated Show resolved Hide resolved
@jamsinclair
Copy link
Author

jamsinclair commented Dec 12, 2024

I'm still tinkering around. The learnings so far are

  • Emscripten sets a dynamic worker name value when ASSERTIONS are configured. This complicates how we can define the static worker options
  • To keep the code backwards compatible with the above, it looks like we'll need to define worker options per use of new Worker(...)
  • To also support the dynamic worker name values we'll need to output a /* @vite-ignore */ comment before each worker options definition

Please let me know if anyone has any other suggestions or cleaner ways we could solve this!

Edit: I do wonder if we can work with Vite to allow dynamic values and only require the worker type option to be static 🤔 . Looking through Vite's source code that appears to be sole reason why they require static values.

Edit 2: Proposed changes to Vite are likely to be accepted that would allow us support dynamic name fields (See: vitejs/vite#19010). I'll update this merge request with this in mind.

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.

2 participants