Conversation
3093556 to
6a99541
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
Unclear on the bundler magic for some sutff 🫠
@IceTDrinker reviewed 23 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).
Makefile line 1419 at r1 (raw file):
cd $(WASM_PAR_MQ_TEST_DIR) && \ RUSTFLAGS="$(WASM_RUSTFLAGS)" wasm-pack build --target=web --out-dir pkg && \ find pkg/snippets -type f -iname worker_helpers.js -exec sed -i 's|import("../../..")|import("../../../wasm_par_mq_web_tests.js")|g' {} \;
is this missing a json patch with jq like from the location you copied ?
unclear to me if the trailing ; is ok but seems a bit suspicious
utils/wasm-par-mq/examples/msm/index.js line 1 at r1 (raw file):
import init, * as wasm_bindgen from "./pkg/wasm_par_mq_example_msm.js";
copy pasted from the html ? (and API calls updated)
utils/wasm-par-mq/js/worker_helpers.js line 27 at r1 (raw file):
// Compute worker if (self.name === WORKER_NAME) {
how is the name set ?
utils/wasm-par-mq/src/pool.rs line 178 at r1 (raw file):
// Send a start message to trigger the worker bootstrap. worker .post_message(&JsValue::UNDEFINED)
maybe could be worth having something else than undefined ? can be a magic constant if needed, just wondering
unless this is recommended
utils/wasm-par-mq/web_tests/index.js line 1 at r1 (raw file):
import init, * as wasm from "./pkg/wasm_par_mq_web_tests.js";
copy pasted from the html ? (and API calls updated)
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).
Makefile line 1419 at r1 (raw file):
Previously, IceTDrinker wrote…
is this missing a json patch with jq like from the location you copied ?
unclear to me if the trailing ; is ok but seems a bit suspicious
The ; is standard with find -exec: https://unix.stackexchange.com/questions/389705/understanding-the-exec-option-of-find.
I don't think the jq is needed here, since there is no "files" in this test package.json. But it will certainly be needed when I update tfhe package in the next pr.
utils/wasm-par-mq/examples/msm/index.js line 1 at r1 (raw file):
Previously, IceTDrinker wrote…
copy pasted from the html ? (and API calls updated)
yes
utils/wasm-par-mq/js/worker_helpers.js line 27 at r1 (raw file):
Previously, IceTDrinker wrote…
how is the name set ?
The async flow, (no sync executor so it is a bit simpler) is like that:
This file is first imported from the main thread, meaning self.name is "".
So we skip both branches and define the functions below.
Then at some point createComputeWorker is called, which will create a worker with the name set as WORKER_NAME, using this file as entrypoint. So we go again through this file, but this time since it's from the worker we just created, self.name is set to WORKER_NAME.
In the sync mode, the main thread will load the sync executor instead of the worker. This is the same pattern, self.name is "", then we call createSyncExecutor, then the file is executed again from the sync executor with self.name set to SYNC_EXECUTOR_NAME. Except this time it's double use, it will at the same time start the worker but also define the createComputeWorker. This time the function will be called from the SyncExecutor, but like before will create a worker with WORKER_NAME. So we will execute the file a third time with self.name = WORKER_NAME.
utils/wasm-par-mq/src/pool.rs line 178 at r1 (raw file):
Previously, IceTDrinker wrote…
maybe could be worth having something else than undefined ? can be a magic constant if needed, just wondering
unless this is recommended
This is used to send an "empty" message to the worker to start it. The worker ignore this value, it is implemented as a lambda function that takes no argument (in utils/wasm-par-mq/js/worker_helpers.js line 30). So I guess UNDEFINED fits this purpose. On the opposite the sync executor takes an argument
utils/wasm-par-mq/web_tests/index.js line 1 at r1 (raw file):
Previously, IceTDrinker wrote…
copy pasted from the html ? (and API calls updated)
yes
6a99541 to
7078221
Compare
7078221 to
7f72a09
Compare
closes: please link all relevant issues
Goal
Simplifies a bit the API/DX of wasm-par-mq:
coordinator.jsis still neededFor the end user, it means that (taken from #3339):
becomes:
General idea
The main idea that makes it work has been inspired by wasm-bindgen-rayon: https://github.com/RReverser/wasm-bindgen-rayon/blob/main/src/workerHelpers.js.
The idea is to have a file that is at the same time an entry point for the worker and module that can be imported from rust as an
externblock. That way, the js will be copied at a known location (in thesnippetdir) by wasm-bindgen, removing the need for user to manually copy them and to give their path as an argument.Basically, this js file is:
Another trick is to create the worker from JS and use the syntax
new Worker(new URL("./worker_helpers.js", import.meta.url), ...);. This syntax is valid when executed directly in the browser, but is also recognized like a "keyword" by bundlers (like webpack) that will automatically fix the url and make it work even if the file is bundled and renamed. This could not work previously because the worker creation was done in rust and was not understood by bundlers.The
await import("../../..");syntax is also a similar trick from wasm-bindgen-rayon. It is also recognized by workers and converted by the path to the parent js package entrypoint. However it means that in the--target=web, case this path should be manually patched (using sed) like we already do for wasm-bindgen-rayon.I also simplified the way the coordinator is instantiated. Before users had to copy coordinator.js and create a sw.js file that would import it. This was a bit useless, now only copying coordinator.js is needed.
Misc
This pr also has a fmt of the js code in wasm-par-mq, but this is a separate commit. I also refactored the example/test js to move it to its own js file.
This change is