Skip to content

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

Merged
merged 3 commits into from
Apr 16, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 9, 2024

Increase also the initial memory from 10 MiB to 20 MiB.

Reenable test_dynamic on WASI release build.

Increase also the initial memory from 10 MiB to 20 MiB.

Reenable test_dynamic on WASI release build.
@vstinner
Copy link
Member Author

vstinner commented Apr 9, 2024

!buildbot wasi

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 2736f25 🤖

The command will test the builders whose names match following regular expression: wasi

The builders matched are:

  • wasm32-wasi PR
  • wasm32-wasi Non-Debug PR
  • wasm32 WASI 8Core PR

@vstinner
Copy link
Member Author

vstinner commented Apr 9, 2024

The 8 MiB limit (8 388 608 bytes) is already used by the wasmtime command:

test.pythoninfo:

sysconfig[HOSTRUNNER]: wasmtime run --wasm max-wasm-stack=8388608 --wasi preview2 --env PYTHONPATH=/$(shell realpath --relative-to /opt/buildbot/kushaldas-wasm/3.x.kushaldas-wasi.wasi.nondebug/build/build_oot/host/../.. /opt/buildbot/kushaldas-wasm/3.x.kushaldas-wasi.wasi.nondebug/build/build_oot/host)/$(shell cat pybuilddir.txt):/Lib --dir ../..::/

The value is set at:

$ grep 8388608 Tools/wasm/ -R
Tools/wasm/wasi.py:                        # The 8388608 value comes from `ulimit -s` under Linux
Tools/wasm/wasi.py:                        "--wasm max-wasm-stack=8388608 "
Tools/wasm/wasm_build.py:            "--wasm max-wasm-stack=8388608 "

I'm not sure of the difference between the linker -z stack-size=8388608 option and wasmtime option max-wasm-stack=8388608. According to the issue, the stack doesn't seem to grow from 512 KiB to 8 MiB automatically: #117645 (comment)

With this change, test_dynamic works as expected (on a release build) with 8 MiB stack.

@vstinner
Copy link
Member Author

vstinner commented Apr 9, 2024

@brettcannon: Would you mind to review this change? Are you ok to increase the initial stack from 512 KiB to 8 MiB?

@vstinner vstinner marked this pull request as ready for review April 9, 2024 12:03
@vstinner
Copy link
Member Author

vstinner commented Apr 9, 2024

test_dynamic pass again on "buildbot/wasm32 WASI 8Core PR" (built in release mode). Previously, test_dynamic was failing on this buildbot.

@vstinner
Copy link
Member Author

vstinner commented Apr 9, 2024

Note: I also tried to leave the initial stack unchanged (512 KiB), and only increase the memory size (to 20 MiB): -z stack-size=524288 -Wl,--stack-first -Wl,--initial-memory=20971520. test_dynamic fails in this case (release build). We need to increase the stack size in the linker if we want test_dynamic to pass: what this PR does.

@erlend-aasland erlend-aasland removed their request for review April 9, 2024 17:13
@vstinner
Copy link
Member Author

I'm not convinced that wasmtime can grow the stack and/or the total memory size. So I changed the configure.ac comment.

@encukou
Copy link
Member

encukou commented Apr 10, 2024

I think this definitely needs a WASI expert. It looks a bit like tuning generic build settings for the specific buildbot.
@ericsnowcurrently, do you want to take a look?

@vstinner
Copy link
Member Author

I did another test:

  • 512 KiB initial stack
  • 40 MiB initial memory

test_dynamic fails with:

    2: memory fault at wasm address 0x1000005b0 in linear memory of size 0x2800000
    3: wasm trap: out of bounds memory access

It confirms that the wasmtime cannot grow the stack: we have to set it to 8 MiB directly.

But it can grow the "total" memory (address space). Example on a build with 40 MiB initial memory:

# cross-build/wasm32-wasi/python.sh 
Python 3.13.0a5+ (heads/wasi_stack-dirty:a69ffa75eb, Apr 15 2024, 13:36:08) [Clang 16.0.0 ] on wasi
Type "help", "copyright", "credits" or "license" for more information.
>>> x=b'x' * (1024 * 1024 * 60)
>>> len(x)
62914560

I can allocate 60 MiB without any issue.

@vstinner
Copy link
Member Author

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 --wasm max-wasm-stack=8388608 option to wasmtime doesn't work: it fails to grow the stack.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a 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.

@vstinner vstinner merged commit 9197847 into python:main Apr 16, 2024
@vstinner vstinner deleted the wasi_stack branch April 16, 2024 21:26
@vstinner
Copy link
Member Author

Merged, thanks for the review @ericsnowcurrently.

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…on#117674)

Increase also the initial memory from 10 MiB to 20 MiB.

Reenable test_dynamic on WASI build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants