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 ability to set stack size for spawned threads #3995

Merged
merged 1 commit into from
Jul 28, 2024

Conversation

daxpedda
Copy link
Collaborator

@daxpedda daxpedda commented Jun 20, 2024

This does a couple of things:

  • Changes the signature of default() and initSync() to take an object and moving all parameters into that object. The old signature still works, but is deprecated. This lets us add additional parameters without breaking changes.
  • Increase the default stack size of spawned threads from 1 to 2 MB. This follows Std's default for tier 1 targets, but I'm unsure what to go by here.
  • Add the property thread_stack_size to default() and initSync(), to let the user set the thread stack size.
  • Modify __wbindgen_thread_destroy() to take an additional parameter specifying the thread stack size. This only comes into play when destroying other threads, when destroying the current thread, by passing no parameters, the correct thread stack size is determined internally.
  • Fix __wbindgen_thread_destroy() ignoring parameters. Though I'm not exactly sure how not setting the correct locals to be parameters when building the function actually played out in reality.

@@ -52,7 +52,7 @@ pub fn get_shadow_stack_pointer(module: &Module) -> Option<GlobalId> {
match candidates.len() {
0 => None,
// TODO: have an actual check here.
1 => Some(candidates[0].id()),
1 | 2 => Some(candidates[0].id()),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there some other/better way to find this now?

@daxpedda daxpedda force-pushed the stack-size branch 2 times, most recently from 8ff0ff8 to 96e5541 Compare June 21, 2024 20:27
@@ -442,7 +472,7 @@ fn inject_destroy(
},
);

let destroy_id = builder.finish(Vec::new(), &mut module.funcs);
let destroy_id = builder.finish(vec![tls_base, stack_alloc, stack_size], &mut module.funcs);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I only added the last parameter, the first two locals were just not assigned.
So I guess __wbindgen_thread_destroy() didn't work until now when called for another thread.

@daxpedda daxpedda marked this pull request as ready for review June 22, 2024 06:32
@daxpedda daxpedda requested a review from Liamolucko June 23, 2024 07:51
@daxpedda
Copy link
Collaborator Author

Changing the parameters on default() and initSync() is something I would appreciate your approval on @Liamolucko.

@daxpedda daxpedda merged commit 7ad1a27 into rustwasm:main Jul 28, 2024
25 checks passed
@fvirdia
Copy link

fvirdia commented Aug 13, 2024

Hi, I stumbled across this commit as consequence of await init() giving me a warning.
I understand the idea is to change the signature to allow passing multiple parameters as part of an object, with the example code using await init({module_or_path: "...."}). In my current setup, I get the path auto-generated by wasm_bindgen, which I find quite convenient. Will await init({}) be supported in the long term, or should I migrate to passing specifically the path?

@daxpedda
Copy link
Collaborator Author

You mean it is emitting a warning when you pass nothing?
That is unintentional, passing no argument is supported in the long term.

I will check whats going on exactly and make a PR to fix this.

@fvirdia
Copy link

fvirdia commented Aug 13, 2024

Yeah, I believe is due to this check here:

if (typeof module_or_path !== 'undefined' && Object.getPrototypeOf(module_or_path) === Object.prototype)

@phoenixsnake
Copy link

I am currently using wasm-pack --target no-modules with wasm_bindgen(ArrayBuffer_of_wasm_bg_binary);.

The reason I use --target no-modules is that I want to deploy just a single .js file, so I keep ArrayBuffer_of_wasm_bg_binary in a code variable. I'm not using the ESM version for this same reason—because I need a single .js file. Therefore, in my use case, there's no extra .wasm file either.

I received the deprecated warning, which led me to this page. Could we keep the legacy API (i.e., not deprecate it) and instead add new options in the second parameter? Since the new Initialize function favors a parameter-less approach, the parameter-less way might not be affected by this change.

can we overload default() parameter like this ( at lease for --target no-modules option )

wasm_bindgen(); //new initialize way
wasm_bindgne( {} ); //current way

//For legacy support, can we avoid deprecating this
wasm_bindgen( ArrayBuffer | Unit8Array );

//extend legacy support
wasm_bindgen( ArrayBuffer | Unit8Array, {} );

@daxpedda
Copy link
Collaborator Author

From your post I was unable to tell why the new API doesn't work for you.
There shouldn't be anything the new API is unable to do that the old API was able to do.

E.g. in your case:

// old API:
wasm_bindgen(your_array_buffer);
// new API:
wasm_bindgen({ module_or_path: your_array_buffer });

@phoenixsnake
Copy link

Thank you for your reply.

I am currently use this method
wasm_bindgen({ module_or_path: your_array_buffer });

Will module_or_path be the official name for this parameter object in wasm_bindgen?

I found the following documentation on this topic: Without a Bundler.
https://rustwasm.github.io/docs/wasm-bindgen/examples/without-a-bundler.html

However, I can’t find specific documentation on the correct way to initialize with wasm_bindgen( { parameter } ).

Almost every new example uses initialization without parameters.
I’m just worried about deprecated warnings and whether this style of initialization might become unsupported in the future.

@daxpedda
Copy link
Collaborator Author

I am currently use this method wasm_bindgen({ module_or_path: your_array_buffer });

Will module_or_path be the official name for this parameter object in wasm_bindgen?

It already is!

However, I can’t find specific documentation on the correct way to initialize with wasm_bindgen( { parameter } ).

Unfortunately documentation is pretty lacking on this right now, it should definitely be improved!

Almost every new example uses initialization without parameters.
I’m just worried about deprecated warnings and whether this style of initialization might become unsupported in the future.

This is a bug!
Initialization without parameters will definitely not be deprecated in the future.
This has been addressed in #4074 and a new version with this fix will be released soon.

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