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

implement thread-spawn #81

Merged
merged 8 commits into from
Nov 10, 2024
Merged

implement thread-spawn #81

merged 8 commits into from
Nov 10, 2024

Conversation

oligamiq
Copy link
Contributor

I have implemented a multi-thread accessible file system, which was a prerequisite for wasi-threads.

I haven’t created any tests as I’m not quite sure how to do that.

Features:
Usable across multiple workers
Create a WASIFarm and obtain a WASIFarmRef using .get_ref().
WASIFarmRef can be sent via postMessage.
Be cautious, as classes will also be sent as objects via postMessage.
Multiple WASIFarm can be used to create WASIFarmAnimal.
For Stdio, the WASIRef at the end takes precedence.
Files passed to WASIRef will be treated as if they exist.
WASIFarmAnimal can be accessed using .wasiImport.
Each WASIFarmAnimal can only have one .wasiImport.
When creating a WASIFarmAnimal, you can pass override_fd_maps to change the files accessible by each WASIFarmRef. This allows passing file descriptors to child processes.
For usage examples, please refer to the wasi_threads example.
I ported the example rustc and it works fine!
Issues:
Uses SharedArrayBuffer
Since the communication portion is separated, by understanding the polyfill found at https://github.com/WebReflection/sabayon, it should be possible to implement this in a non-SharedArrayBuffer environment.

Commands used across multiple files cannot yet be used across multiple Farms
I will address this eventually.

File pointers are not process-dependent
This could be achieved by using pread to implement read, but I’m not sure if it's correct to make file pointers process-dependent in the first place.

Parts that haven’t been tested may be unreliable
If 100% is the amount of code I can manage, then this project has reached 120%, so I may not be able to manage everything.

It should be correct, but atomics.waitAsync gives tsc error.

Motivation is running low

@oligamiq oligamiq changed the title Usable across multiple workers WASI threads-spwan Sep 27, 2024
@oligamiq oligamiq changed the title threads-spwan implement thread-spawn Sep 27, 2024
@oligamiq
Copy link
Contributor Author

oligamiq commented Sep 27, 2024

rustc on LLVM confirmed to work properly

image

image

@oligamiq
Copy link
Contributor Author

I try running wasm-ld using
https://github.com/YoWASP/clang

@bjorn3
Copy link
Owner

bjorn3 commented Sep 27, 2024

bjorn3/rust#8 links lld directly into the rustc.wasm binary such that it doesn't need to run an external linker executable.

@oligamiq
Copy link
Contributor Author

Have you reached the level of availability yet?

@bjorn3
Copy link
Owner

bjorn3 commented Sep 27, 2024

What do you mean?

@oligamiq
Copy link
Contributor Author

Does it work?

@bjorn3
Copy link
Owner

bjorn3 commented Sep 27, 2024

It works in wasmtime at least:

$ echo 'fn main() { println!("Hello World!"); }' | wasmtime run -Sthreads=y -Spreview2=n --dir tmp::/ --dir dist --env RUST_MIN_STACK=16777216 dist/bin/rustc.wasm - --sysroot dist --target wasm32-wasip1
Linking using LC_ALL="C" VSLANG="1033" "rust-lld" "-flavor" "wasm" [...]
$ wasmtime run tmp/rust_out.wasm
Hello World!

@oligamiq
Copy link
Contributor Author

ok, I'll try it after it's compiled on my server

Change to use the first stdio

fd_write can be async enabled

move but wasm throw error:
  memory access out of bounds
@oligamiq
Copy link
Contributor Author

Report on the result of using wasm-ld
Clang exits without any output. It might be caused by #80.
When passing a blob pattern to wasm-ld, it directly passes it to path_open, which causes an issue (should this be addressed?).
When passing proper arguments, it fails midway with a "move but wasm throw error: memory access out of bounds" error.

I should test wasm-ld with wasmtime, but first, I plan to try bjorn3/rust#8.

@bjorn3
Copy link
Owner

bjorn3 commented Sep 27, 2024

When passing a blob pattern to wasm-ld, it directly passes it to path_open, which causes an issue (should this be addressed?).

On Unix glob patterns are normally handled by the shell, not by the program itself.

@oligamiq
Copy link
Contributor Author

oligamiq commented Sep 27, 2024

Using bjorn3/rust#8, I successfully compiled and executed it directly on the web!

example: wasi_multi_threads_rustc

@bjorn3
Copy link
Owner

bjorn3 commented Sep 27, 2024

Great work! Just as a heads up: It will almost certainly take me at least a week before I have time to review this.

@oligamiq
Copy link
Contributor Author

I've written quite a lot of comments, but since there are many files and the structure is quite complex, feel free to ask if you have any concerns!

@oligamiq
Copy link
Contributor Author

I don't understand poll_oneoff at all, but since I'm implementing a file system that can be accessed by multiple workers, it's possible that I can implement poll_oneoff as well.

@bjorn3
Copy link
Owner

bjorn3 commented Sep 27, 2024

I don't understand poll_oneoff at all

It's similar to epoll on Linux, kqueue on BSD. Basically it allows you to ask a bunch of fd's at once if they have any data ready to ready from or can be written too and I believe also allows setting a timeout. This is what async rust internally uses on wasi. It is mostly useful for streams of data like network sockets or reading from stdin.

Copy link
Owner

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

This is a lot more code than I expected. I've left a couple of small comments, but I'm no way near done reviewing this. It will likely take at least a couple of weeks before I've fully reviewed this. Would it be possible to have the thread support in a separate package? If so if it works locally for me, I think it could be merged mostly as is with a disclaimer that it is less production ready than the rest of browser_wasi_shim.

src/wasi.ts Outdated Show resolved Hide resolved
src/fs_mem.ts Outdated Show resolved Hide resolved
test/run-testsuite.sh Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
.gitmodules Outdated Show resolved Hide resolved
@oligamiq
Copy link
Contributor Author

oligamiq commented Oct 4, 2024

We are dependent on classes such as Fd, File, and Directory. Since I have only modified src/wasi.ts and have isolated everything in the src/wasi_farm directory, I believe it is quite feasible to create a separate npm package that has @bjorn3/browser_wasi_shim as a dependency.

@oligamiq
Copy link
Contributor Author

oligamiq commented Oct 6, 2024

Regarding the wasm-ification of cargo:
For the time being, I’ve managed to run cargo run, so I’ll pause the work for now. I’ve disabled all git-related functionalities, so I can’t use any libraries. Due to the heavy use of threads, the process has become extremely slow. Therefore, I plan to wait for the implementation of thread pools, fd_filestat_set_times, and further maturity of gitoxide, as well as WASI Preview 2 reaching Tier 2.

I won’t be submitting a pull request because the current state of cargo is too problematic. However, in the process, I did discover a bug related to this pull request. Additionally, I implemented a special function to run wasm within virtual files, and I’d like to add that as well. Would that be acceptable?

image

@bjorn3
Copy link
Owner

bjorn3 commented Oct 6, 2024

Additionally, I implemented a special function to run wasm within virtual files, and I’d like to add that as well. Would that be acceptable?

If you add it outside of the wasi_snapshot_preview1 module import, it should be possible for the users of browser_wasi_shim to add it without direct browser_wasi_shim support, right? I'm fine with adding it to one of the examples, but I'm hesitant to add extensions to wasi to the core of browser_wasi_shim.

@oligamiq
Copy link
Contributor Author

oligamiq commented Oct 6, 2024

Indeed. I'll submit the bug fix only.

@oligamiq
Copy link
Contributor Author

oligamiq commented Oct 6, 2024

This is a lot more code than I expected. I've left a couple of small comments, but I'm no way near done reviewing this. It will likely take at least a couple of weeks before I've fully reviewed this. Would it be possible to have the thread support in a separate package? If so if it works locally for me, I think it could be merged mostly as is with a disclaimer that it is less production ready than the rest of browser_wasi_shim.

What shall we do?

…itten fd_renumber. add .vscode on .gitignore. rm unused change. Pass fd_map as is to child threads. Change .gitmodules to tabs. reset run-testsuite.sh Minor modifications to the code.

patch about null equal undefined on parse JSON.
@bjorn3
Copy link
Owner

bjorn3 commented Oct 6, 2024

What shall we do?

Please split the thread support into a new package that depends on the main browser_wasi_shim package.

@oligamiq
Copy link
Contributor Author

oligamiq commented Oct 6, 2024

Would you prefer to go with a monorepo, or should I set up a separate repository?

@bjorn3
Copy link
Owner

bjorn3 commented Oct 6, 2024

Having it in this repo is fine.

@oligamiq
Copy link
Contributor Author

oligamiq commented Oct 6, 2024

Ok, I try monorepo.
However, it will take time because I don't know the publishing system of this repository.

@oligamiq
Copy link
Contributor Author

oligamiq commented Oct 6, 2024

By the way, do you have a good package name?

@bjorn3
Copy link
Owner

bjorn3 commented Oct 7, 2024

Maybe browser_wasi_shim-threads?

@oligamiq
Copy link
Contributor Author

oligamiq commented Oct 8, 2024

I splited, so check please.

Copy link
Owner

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

Couple minor changes and I will merge it.

.gitmodules Show resolved Hide resolved
threads/README.md Show resolved Hide resolved
@@ -0,0 +1,21 @@
# A pure javascript shim for WASI Preview 1 threads

This project is implement threads on browser_wasi_shim
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
This project is implement threads on browser_wasi_shim
This project implement threads on browser_wasi_shim

nit

@bjorn3
Copy link
Owner

bjorn3 commented Nov 10, 2024

(did you forget to push?)

@oligamiq
Copy link
Contributor Author

Oh, I see. I couldn’t pull because I resolved it without committing. My apologies and thank you so much!

@bjorn3 bjorn3 merged commit 3f705d1 into bjorn3:main Nov 10, 2024
1 check passed
@bjorn3
Copy link
Owner

bjorn3 commented Nov 10, 2024

Thanks a lot for working on this!

@oligamiq
Copy link
Contributor Author

Oh, my apologies, I just finished making the corrections, so it was a bit too early to merge.

@bjorn3
Copy link
Owner

bjorn3 commented Nov 10, 2024

My bad. I fixed the typo you hadn't resolved yet when I merged this PR myself, but for any other changes please open another PR.

@oligamiq
Copy link
Contributor Author

ok.

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