-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Change stack size on all desktop platforms to at least 1.5MB #98007
Conversation
- Change stack size to 4MB on 64-bit desktop - Add tests to check stack size
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
(force push & empty commit were to workaround #98009) |
...aries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaderBuilder.cs
Outdated
Show resolved
Hide resolved
As I have mentioned in #87879 (comment), I think that we should only fix macOS. Bumping the stack size on Windows is likely to introduce as many problems as it fixes. |
- Fix test definition - Fix NAOT apps stack size - Add `<UseAppHost>false</UseAppHost>` version of non-NAOT tests
@janvorli what are your thoughts on the stack sizes we should change? I currently have them all at 4MB for 64-bit desktop platforms, but this is mainly for testing (not that I would complain if we ended up on this number 😆). Would you be happy with 2MB for all desktop platforms?
@jkotas would you accept 2MB also? It is only marginally higher than the current Windows value (1.5MB). I think it is a good middle ground, and should be enough to account for the ever-expanding usage of the stack that I mentioned in #87879 (comment) for the time being (and we could obviously re-visit it in the future if we wanted to increase it again). |
I've found these 3 disabled tests due to macOS stack size, are there any others which I should re-enable with this fixed? |
I don't see a good reason for changing the Windows stacksize. We would need to file a breaking change notice for it and it would be hard to come up with a good justification. |
src/tests/Regressions/coreclr/GitHub_87879_AppHost/test87879.csproj
Outdated
Show resolved
Hide resolved
src/tests/Regressions/coreclr/GitHub_87879_AppHost/test87879.csproj
Outdated
Show resolved
Hide resolved
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 otherwise. Thank you!
- Remove the NAOT specific test, since NAOT runs for the main test also
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Should runtime-extra-platforms be run also @jkotas? (I can't run it, but I would think it makes sense, right?) |
I just wanted to double check that the new test is running as part of runtime-nativeaot-outerloop (confirmed - it does). |
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.
Thank you!
Fixes #87879
Fixes #2084
Fixes #1417
Changes the stack size on desktop platforms to a certain minimum.
Specific changes we've landed on:
IlcDefaultStackSize
to be used to specify the stack size for NAOT on all platformsPlan:
(Already reverted) Note: I've currently made some changes to System.Reflection.Metadata's API, I don't plan to keep these - I had noticed that NAOT seemed to only be giving 1MB on Windows though when I built it a test console app, so I am seeing if it fixes that before doing a more proper fix.