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

Add example for wasm_threads usage #841

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Add example for wasm_threads usage #841

merged 7 commits into from
Aug 7, 2024

Conversation

9SMTM6
Copy link
Contributor

@9SMTM6 9SMTM6 commented Aug 4, 2024

Would be an example for #680.

@ctron
Copy link
Collaborator

ctron commented Aug 5, 2024

Thanks for the PR. That's awesome. I think we already have something for the headers on main. I'll try to document that. So we could add this too.

@ctron
Copy link
Collaborator

ctron commented Aug 5, 2024

Using trunk from main or the most recent -alpha release, you can add:

[serve]
headers = { "X-Foo" = "bar" }

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Aug 5, 2024

@ctron If you've got something else let me know. I think this is about as well documented as it can be, I hope to avoid issues being created about this (if you look at wasm_thread issues then sadly many people did not read the documentation properly).

And thank you for the hint about the .cargo/config.toml file working for this at all, and being respected by trunk. IDK if I had found this otherwise.

@ctron
Copy link
Collaborator

ctron commented Aug 5, 2024

Cool, thanks. One think that's missing, sorry for overlooking that before, is to add it to the list of CI example tests:

example:
- cargo-manifest
- cdylib
- hooks
- initializer
- leptos
- no-rust
- proxy
- seed
- target-path
- vanilla
- webworker
- webworker-gloo
- webworker-module
- yew
- yew-tailwindcss
- yew-tls

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Aug 5, 2024

right. that would've been to easy. The pipeline doesnt have nightly. My pipeline uses dtolnay/rust-toolchain@stable to get the toolchain, and that respects toolchain overrides. But here the CI is more complicated, doesn't look like that'd work. I'm uncertain if my example fits in there at all, with how you use trunk --config.

I won't be able to get to this until tomorrow evening at the soonest.

@ctron
Copy link
Collaborator

ctron commented Aug 5, 2024

No worries, take your time. I think it's close!

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Aug 6, 2024

Hmm the failure wasnt because no nightly was present, but because it just used stable. In other words, it ignored the toolchain file. The --config flag is probably the reason. @ctron, I see 3 paths forward.

  1. trunk --config is intended to act just as if trunk was invoked in said directory, which means that this would need fixing, since currently theres a difference whether I change to the example/wasm_threads directory and invoke trunk or call it with --config from root. In the first case it respects the tool override, in the other it does not.
  2. if that is as intended, or will be fixed later, we could either make a specific job to test --config with intended situations or drop it, and then switch to the example directory in the CI and run trunk build in there directly.
  3. I could make a separate CI for this example (or drop running it in CI, but considering its using nightly and all that it'd probably be a good idea to keep it).

In any case where we run this in the CI, we will probably have to further adjust the CI to download the correct toolchain (nightly with some additional components for this example, perhaps other modifications like additional modules for future examples). So far dtolnay/rust-toolchain has worked well for me, as I said, it automatically respected my toolchain override and downloaded the nightly together with the also specified rust-src component changed the toolchain file.

@ctron
Copy link
Collaborator

ctron commented Aug 7, 2024

So having a rust-toolchain.toml should be sufficient. At least, that would be my expectation. cargo would automatically download the correct rust version.

So I think the problem is with 1.) … that it's not respecting the base directory of the config file.

@ctron
Copy link
Collaborator

ctron commented Aug 7, 2024

Looks like that's more a cargo issue rust-lang/cargo#11036 (comment) … however, I think we need to fix it.

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Aug 7, 2024

At least, that would be my expectation. cargo would automatically download the correct rust version.

Yeah, you seem to be right, looking closer at the logs this seems to be what's happening.

Looks like that's more a cargo issue rust-lang/cargo#11036 (comment)

Okay, that would make sense if trunk invokes it like that.

So about that fix. Still, 3 ways.

  1. Fix the issue upstream. Not a real solution. Assuming we do manage to solve this, it'd take a while to propagate here, and anyone with an outdated version of cargo still has the issue.
  2. Always change the directory in the pipeline as a fix for this, while staying uncomplicated
  3. Apply a specific fix for this example, with a link to the reason. I think the best way would be to make the example list in the ci file into a list of dicts with optional field 'cd' or similar, and depending on that we either use trunk - - config or cd path && trunk.

How often do you expect that --config is used? The example readme already has a bunch of qualifiers, so if it's not used often I'd leave a notice about this issus out of there. People should be able to figure out a fix (I explicitly mention nightly as required, and the compiler complains that we do something only allowed on nightly). Otherwise I can also add it I guess.

@ctron
Copy link
Collaborator

ctron commented Aug 7, 2024

I think I've a PR for this: #844

@ctron
Copy link
Collaborator

ctron commented Aug 7, 2024

I merged this PR. So maybe you can just rebase and we see if this does the trick.

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Aug 7, 2024

Nice, so we were able to solve this ourselves. It worked locally, so I think this should be it ;-P.

@ctron ctron merged commit 0141157 into trunk-rs:main Aug 7, 2024
60 checks passed
@ctron
Copy link
Collaborator

ctron commented Aug 7, 2024

Thanks for finishing this up. That's a great example!

@Shapur1234
Copy link

Shapur1234 commented Aug 10, 2024

Thanks for working on this!

I've just tried the new example and it works... mostly.
Most thread finish as the should, but a too much recursion error also appears. (Happens on both Librewolf and Chromium, using trunk 0.20.3

(The same too much recursion error also appear with my more complex app, but it could be a me issue)

Console:

INFO src/main.rs:15 
Available parallelism: Ok(2) [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:29 
hi number 1 from the main thread ThreadId(1)! [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:29 
hi number 2 from the main thread ThreadId(1)! [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:22 
hi number 1 from the spawned thread ThreadId(3)! [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:35 
Start scope test on thread ThreadId(2) [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:22 
hi number 1 from the spawned thread ThreadId(4)! [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
InternalError: too much recursion
2 [wasm_threads-d4756fe60b235b5f_bg.wasm:338386:1](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f_bg.wasm)
INFO src/main.rs:42 
hello from the first scoped thread ThreadId(5) [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:44 
a = [1, 2, 3] [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:50 
a[0..2] = [1, 2] [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:59 
Hello from scope "main" thread ThreadId(2) inside scope. [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:53 
hello from the second scoped thread ThreadId(6) [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)
INFO src/main.rs:68 
Scope done x = 4, a.len() = 4 [wasm_threads-d4756fe60b235b5f.js:408:17](http://127.0.0.1:8080/wasm_threads-d4756fe60b235b5f.js)

Any ideas what's going on?

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Aug 10, 2024

I've seen this too at times. It went away reliably in release mode though. And since I've used this in my App which always compiles dependencies with optimizations and it works fine.

I'm not entirely sure how it comes to be, but I don't think that's got anything to do with trunk.

@Shapur1234
Copy link

Yes, compiling in release mode fixes it, weird.
I don't remember seeing this this error when compiling wasm_thread using wasm-bindgen directly (But it has been some time).

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Aug 11, 2024

It going away with optimizations is probably coming from something like tail recursion optimization.

I'm mostly surprised where that amount of recursion is coming from. But I can't really see this being caused by trunk. It going away with optimizations on rust code means its gotta be in rust code - unless IÄm missing something -, and AFAIK trunk doesn't add a lot, or nothing, of that to the runtime of the website, and that optimization setting doesn't affect trunk either.

I'm also not entirely sure whats the result of that error either.

So if you add this to the Cargo.toml it also goes away:

[profile.dev.package."*"]
opt-level = 2

This will make clean rebuilds slower tho.

I did not test wasm_thread on its own, to be clear. I did never clone their repo.

@Shapur1234
Copy link

Shapur1234 commented Aug 11, 2024

Yes, optimizing dependencies fixed it for me.
I might be worth noting this in the README.md of the example.

@9SMTM6
Copy link
Contributor Author

9SMTM6 commented Aug 11, 2024

I'll add it and improve grammar in a pr. I'll be terse, as I'm still not certain of whats really going on.

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.

3 participants