-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
gh-117645: Increase WASI stack size from 512 KiB to 8 MiB #117674
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
Conversation
Increase also the initial memory from 10 MiB to 20 MiB. Reenable test_dynamic on WASI release build.
!buildbot wasi |
The 8 MiB limit (8 388 608 bytes) is already used by the test.pythoninfo:
The value is set at:
I'm not sure of the difference between the linker With this change, test_dynamic works as expected (on a release build) with 8 MiB stack. |
@brettcannon: Would you mind to review this change? Are you ok to increase the initial stack from 512 KiB to 8 MiB? |
test_dynamic pass again on "buildbot/wasm32 WASI 8Core PR" (built in release mode). Previously, test_dynamic was failing on this buildbot. |
Note: I also tried to leave the initial stack unchanged (512 KiB), and only increase the memory size (to 20 MiB): |
I'm not convinced that wasmtime can grow the stack and/or the total memory size. So I changed the configure.ac comment. |
I think this definitely needs a WASI expert. It looks a bit like tuning generic build settings for the specific buildbot. |
I did another test:
test_dynamic fails with:
It confirms that the But it can grow the "total" memory (address space). Example on a build with 40 MiB initial memory:
I can allocate 60 MiB without any issue. |
It seems like nobody is available to review this change. I plan to merge my PR at the end of the week, unless someone comes and wants to review it :-) The change reenables a test_dynamic test which is currently always skipped on WASI. IMO it will unlock other use cases which are currently limited by the maximum stack memory. IMO a stack of 512 KiB is too small for Python, and sadly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for doing this! I've left one comment, but I'll leave it to you discretion.
Merged, thanks for the review @ericsnowcurrently. |
…on#117674) Increase also the initial memory from 10 MiB to 20 MiB. Reenable test_dynamic on WASI build.
Increase also the initial memory from 10 MiB to 20 MiB.
Reenable test_dynamic on WASI release build.