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

Implement setStackSize on Windows #10737

Merged
merged 5 commits into from
May 25, 2024

Conversation

poweredbypie
Copy link
Member

@poweredbypie poweredbypie commented May 18, 2024

Motivation

See #10540.

Context

This implements setStackSize for Nix using MinGW, with the strategy described in #10540 (SetThreadStackGuarantee). I've tested this on a Windows 10 LTSC VM through NixOS without any issues.

However, I'm not sure if this change is really required. It seems from a bit of skimming that Windows allocates 1 MB of stack space for each thread by default (specified here), and it doesn't seem like that number is modifiable after compilation. As a result, the setStackSize call in main doesn't work with the 64 MB of stack space requested, as on Unix systems.

I've opted to request 1 MB of stack space as a result, but I'm unsure if this is even a useful request, considering the default thread's stack size.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label May 18, 2024
@Ericson2314
Copy link
Member

Ericson2314 commented May 20, 2024

@poweredbypie Thanks for trying this, especially the VM testing! Seeing https://stackoverflow.com/questions/156510/increase-stack-size-on-windows-gcc, it looks like we can use that in conjunction with this function, and then do the same size size as on Unix?

@Ericson2314 Ericson2314 self-assigned this May 20, 2024
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

per my comment above

@poweredbypie
Copy link
Member Author

poweredbypie commented May 23, 2024

I've implemented the linker flag and removed the conditional compilation so both Unix and Windows guarantee 64 MB. However, I've set the linker flag to commit 128 MB of stack rather than 64, since it seems to fail when only set to 64. I've tried with the following tests:
image
This gives the following on my VM:
image
The last 3 fail, so I'm assuming the reservation is aligned somewhat and is ignoring individual byte variation. I can do a more conservative reservation of maybe 64.5 or 65 MB if requested, but I don't think this is an issue in the long run since it only reserves the memory and doesn't commit it until actually needed. (see here again.)

Makefile Outdated
Comment on lines 99 to 100
# Increase the default reserved stack size to 128 MB so Nix doesn't run out of space
GLOBAL_LDFLAGS += -Wl,--stack,134217728
Copy link
Member

@Ericson2314 Ericson2314 May 23, 2024

Choose a reason for hiding this comment

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

Can we do this per executable? So change src/nix/local.mk? The one with the unit tests can be adjusted too.

Also maybe do the trick in https://stackoverflow.com/questions/1926063/how-do-i-perform-arithmetic-in-a-makefile to make things easier to read?

Copy link
Member Author

@poweredbypie poweredbypie May 24, 2024

Choose a reason for hiding this comment

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

Can we do this per executable? So change src/nix/local.mk? The one with the unit tests can be adjusted too.

Done, I added the flag to nix and all the libnix*-tests binaries. However, I'm not sure if that's what you're referring to. Do you mean all the tests or just the libnixutil-tests binary?

Also maybe do the trick in https://stackoverflow.com/questions/1926063/how-do-i-perform-arithmetic-in-a-makefile to make things easier to read?

Done!

@Ericson2314
Copy link
Member

@poweredbypie Thanks for all your testing work! Yes it does seem that the "user portion" of the stack is a bit smaller. If you can make a 65 or 66 MiB work, I do agree that would be best!

@poweredbypie
Copy link
Member Author

@poweredbypie Thanks for all your testing work! Yes it does seem that the "user portion" of the stack is a bit smaller. If you can make a 65 or 66 MiB work, I do agree that would be best!

Sure. 65 MB seems to work fine:
image
I'm using CFF Explorer to check the PE header (it shows 0x4100000, which is 65 MB in hex)

This way we can commit the same amount of stack size (64 MB) without a conditional.
Includes nix, libnixexpr-tests, libnixfetchers-tests, libnixstore-tests, libnixutil-tests.
@Ericson2314
Copy link
Member

https://learn.microsoft.com/en-us/cpp/build/reference/stack-stack-allocations?view=msvc-170

says

Use this option only when you build an .exe file. The /STACK option is ignored when applied to .dll files.

That definitely matches my intuition. Can we remove this from the libraries, or did you find it does in fact make a difference when testing?

@poweredbypie
Copy link
Member Author

poweredbypie commented May 24, 2024

https://learn.microsoft.com/en-us/cpp/build/reference/stack-stack-allocations?view=msvc-170

says

Use this option only when you build an .exe file. The /STACK option is ignored when applied to .dll files.

That definitely matches my intuition. Can we remove this from the libraries, or did you find it does in fact make a difference when testing?

Sorry, I'm not sure what you mean. I added the flags to the test binaries, which are still exe files:
image
image
(SizeOfStackReserve is 0x4100000 for libnixexpr-tests.exe)
The support libraries do not get the flag:
image
(SizeOfStackReserve is 0x200000 for libnixexpr-tests-support.dll, the default)
I can still remove them, since I'm not sure if they need the extra stack space, but the flag is taking effect.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Sorry! I misread

@Ericson2314 Ericson2314 merged commit 5cfa75e into NixOS:master May 25, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants