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

Inconsistent DEFAULT_MIN_STACK_SIZE Across Platforms Causes Unexpected Issues #126027

Closed
namse opened this issue Jun 5, 2024 · 7 comments · Fixed by #126059
Closed

Inconsistent DEFAULT_MIN_STACK_SIZE Across Platforms Causes Unexpected Issues #126027

namse opened this issue Jun 5, 2024 · 7 comments · Fixed by #126059
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@namse
Copy link

namse commented Jun 5, 2024

The DEFAULT_MIN_STACK_SIZE is the default stack size assigned when creating a thread. This value varies across different platforms. The issue is that there isn't enough explanation as to why these values are different, and users might not realize that the stack size can vary.

Recently, I was running existing Windows code on a WASI target, and I kept encountering font decoding failures. Since it worked correctly on Windows, I suspected WASI's instability and spent considerable time debugging. After several days, I discovered that the root cause was the stack size: WASI has a very small stack size of 4KB, while Windows has a much larger stack size of 2MB. Once I forcibly increased the thread's stack size, all issues were resolved.

I felt quite frustrated that such a simple reason caused the problem.

Although this is my personal experience, I believe that rather than setting different default stack sizes for different platforms without a compelling reason, it would be better to have a consistent stack size across all platforms in Rust.

However, I acknowledge that this suggestion might not be very effective because the required memory can vary depending on the instruction set or architecture.

I would like to discuss this matter further.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 5, 2024
@workingjubilee
Copy link
Member

Different platforms have different useful minimum stack sizes.

I have no idea why only 4KiB is reserved on wasi.

@workingjubilee
Copy link
Member

workingjubilee commented Jun 5, 2024

In my opinion, this isn't an issue of "inconsistency" this is an issue of a target having an absurdly low value, period. Please open a PR to raise it.

@workingjubilee workingjubilee added the O-wasm Target: WASM (WebAssembly), http://webassembly.org/ label Jun 5, 2024
@namse
Copy link
Author

namse commented Jun 6, 2024

Different platforms have different useful minimum stack sizes.

What are the criteria for determining that? Since I don't know, I can't tell whether the 4KB for WASI is inappropriate or not, and I also can't open a PR because I don't know the appropriate value.

@workingjubilee
Copy link
Member

...a zillion insanely low-level minutiae like how big the registers are, mostly. an article that might help: https://ariadne.space/2021/06/25/understanding-thread-stack-sizes-and-how-alpine-is-different/

as that article explains, the smallest thread stack size of all platforms that I'm aware of, aside from the ones in our repo that only ask for 4096 bytes, is 64KiB. the current 4096 bytes boundary has caused you significant pain so it seems sensible to raise it to at least that much, then fool around and see how it works.

@workingjubilee
Copy link
Member

workingjubilee commented Jun 6, 2024

having done some review of our platform code, I am pretty sure 4096 bytes was just picked as the "unsupported" value and platforms that initially lacked thread support, like wasm32-wasi, just inherited that value.

@namse
Copy link
Author

namse commented Jun 6, 2024

I understand. The differences between the platforms are indeed huge..!

Before proceeding, I would like to hear the opinion or history from Alex Crichton, the main maintainer of the wasi target. @alexcrichton , could you please share your thoughts on this issue?

@alexcrichton
Copy link
Member

I commented a bit on #126059 but I've no recollection for the current value for wasi. I suspect it was a number of historical accidents that led to this, and stack sizes should definitely be larger than 4k by default on wasi. Thanks for the issue @namse!

fmease added a commit to fmease/rust that referenced this issue Jun 6, 2024
…r, r=ChrisDenton

Raise `DEFAULT_MIN_STACK_SIZE` to at least 64KiB

Prevent copy-paste errors from producing new starved-for-resources threaded platforms by raising `DEFAULT_MIN_STACK_SIZE` from 4096 bytes to at least 64KiB.

Two platforms "affected" by this have no actual threads:
- UEFI
- "unsupported"

Platforms that this actually affects:
- wasm32-wasi with "atomics" enabled
- wasm32-wasi-p1-threads

Two exceptions:
- SGX: a "secure code execution" platform, stays at 4096B
- TEEOS: also a "secure code execution" platform, stays at 8192B

I believe either of these may have sufficiently "interesting" semantics around threads, or significant external library support. Either would mean making any choices here for them is suspect.

Fixes rust-lang#126027 which is a bug report about `DEFAULT_MIN_STACK_SIZE` being too low on wasi.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 6, 2024
…r, r=ChrisDenton

Raise `DEFAULT_MIN_STACK_SIZE` to at least 64KiB

Prevent copy-paste errors from producing new starved-for-resources threaded platforms by raising `DEFAULT_MIN_STACK_SIZE` from 4096 bytes to at least 64KiB.

Two platforms "affected" by this have no actual threads:
- UEFI
- "unsupported"

Platforms that this actually affects:
- wasm32-wasi with "atomics" enabled
- wasm32-wasi-p1-threads

Two exceptions:
- SGX: a "secure code execution" platform, stays at 4096B
- TEEOS: also a "secure code execution" platform, stays at 8192B

I believe either of these may have sufficiently "interesting" semantics around threads, or significant external library support. Either would mean making any choices here for them is suspect.

Fixes rust-lang#126027 which is a bug report about `DEFAULT_MIN_STACK_SIZE` being too low on wasi.
@bors bors closed this as completed in 68c57de Jun 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 7, 2024
Rollup merge of rust-lang#126059 - workingjubilee:stack-nothing-higher, r=ChrisDenton

Raise `DEFAULT_MIN_STACK_SIZE` to at least 64KiB

Prevent copy-paste errors from producing new starved-for-resources threaded platforms by raising `DEFAULT_MIN_STACK_SIZE` from 4096 bytes to at least 64KiB.

Two platforms "affected" by this have no actual threads:
- UEFI
- "unsupported"

Platforms that this actually affects:
- wasm32-wasi with "atomics" enabled
- wasm32-wasi-p1-threads

Two exceptions:
- SGX: a "secure code execution" platform, stays at 4096B
- TEEOS: also a "secure code execution" platform, stays at 8192B

I believe either of these may have sufficiently "interesting" semantics around threads, or significant external library support. Either would mean making any choices here for them is suspect.

Fixes rust-lang#126027 which is a bug report about `DEFAULT_MIN_STACK_SIZE` being too low on wasi.
@saethlin saethlin added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants