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

[wasm-mt] Fixup C exports initialization after startup.ts refactoring #73158

Merged
merged 4 commits into from
Aug 2, 2022

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Aug 1, 2022

Fixup issues due to d7c8bc1:

  1. Add back the missing diagnostic server C wraps.
  2. Run init_c_exports lazily on pthread workers, always. Eager initialization doesn't work because at the time that our code runs, Emscripten hasn't fully initialized Module yet (in particular Module["cwraps"] hasn't been assigned yet).
  3. Call mono_wasm_pthread_worker_init directly from initializeImportsAndExports - calling it from preInit doesn't work because Emscripten doesn't run preInit in pthread workers. Also there's a technical change here to return a Promise<EmscriptenModule> - Emscripten is good with this because they use .then in worker.js and it's good for us because we can do async initialization at this point (for example waiting for the channel to the main browser thread to get established)

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned lambdageek Aug 1, 2022
@lambdageek lambdageek added area-VM-threading-mono arch-wasm WebAssembly architecture labels Aug 1, 2022
@ghost
Copy link

ghost commented Aug 1, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixup issues due to d7c8bc1:

  1. Add back the missing diagnostic server C wraps.
  2. Run init_c_exports lazily on pthread workers, always. Eager initialization doesn't work because at the time that our code runs, Emscripten hasn't fully initialized Module yet (in particular Module["cwraps"] hasn't been assigned yet).
  3. Call mono_wasm_pthread_worker_init directly from initializeImportsAndExports - calling it from preInit doesn't work because Emscripten doesn't run preInit in pthread workers. Also there's a technical change here to return a Promise<EmscriptenModule> - Emscripten is good with this because they use .then in worker.js and it's good for us because we can do async initialization at this point (for example waiting for the channel to the main browser thread to get established)
Author: lambdageek
Assignees: lambdageek
Labels:

arch-wasm, area-Threading-mono

Milestone: -

@lambdageek lambdageek added this to the 7.0.0 milestone Aug 1, 2022
@lambdageek
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek lambdageek force-pushed the wasm-mt-pthread-cwraps branch 2 times, most recently from 623a0bd to 1acad8d Compare August 2, 2022 01:51
@lambdageek
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1. Copy the mono_wasm_pre_init_essential code into
mono_wasm_pthread_worker_init in order to set up the C bindings
infrastructure in the workers.

2. Because `mono_wasm_pthread_worker_init` runs in the middle of
Emscripten's initialization (before all the Emscripten properties on
`Module` are assigned) always use lazy cwraps initialization (the
issue is `Module["cwrap"]` hasn't been set yet)
@lambdageek

This comment was marked as outdated.

@lambdageek lambdageek marked this pull request as draft August 2, 2022 13:41
@lambdageek lambdageek marked this pull request as ready for review August 2, 2022 13:41
@lambdageek
Copy link
Member Author

oops, I meant to do that to my other PR. this one is ok.

@lambdageek lambdageek merged commit 4c45d7a into dotnet:main Aug 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-VM-threading-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants