Stack size override #24532
Stack size override #24532
Conversation
sdmaclea
force-pushed
the
StackSizeOverride
branch
from
3071c9c to
edb1e4a
Compare
sdmaclea
force-pushed
the
StackSizeOverride
branch
from
edb1e4a to
b981414
Compare
|
I'm curious about the property name: I guess what I'm saying is, why not |
src/vm/threads.cpp
Outdated
|
|
||
| if ((errno == ERANGE) || (valueStr == end) || (end == nullptr) || (end[0] != 0) || (value >= max4KPages)) | ||
| { | ||
| ThrowMessage("OVERRIDE_DEFAULT_STACK_SIZE_4KPAGES value parse error"); |
There was a problem hiding this comment.
I don't know much about exceptions in the runtime... so maybe this is correct, but I find it weird that ThrowMessage is not used anywhere else as far as I can tell. And also then there's this comment:
Line 481 in e0fd735
Another consideration is the error message, I would think it should be localized (as in coming from resources somewhere?).
There was a problem hiding this comment.
#define ThrowMessage "Don't use this in the VM directory"
That is a terrible worthless define. It should be a static assert at least. Silently ignoring the error is just silly. More technical debt...
I didn't expect this to be a user facing error. I think the message will get lost anyway. It will likely become one of those E_FAIL startup errors that are so nice to debug....
Looks like this might have to become a COMPlusThrowHR...
It is arbitrary. I started with bytes, then decided 4K was the absolute minimum stack size we could ever support, and a 4K granularity was the minimum that would ever make sense. I considered bytes, 4K, 64K, 1M. I could switch it to bytes and set a minimum stack size. I would probably arbitrarily chose 64K. |
|
I think bytes would be better. |
|
Similar to the UseEntryPoint feature, I think it makes sense to have an option to have an environment variable to set the stack size. It's significantly easier for a user to set it as it doesn't require using runtime properties. Thoughts? |
src/vm/corhost.cpp
Outdated
| @@ -742,6 +742,12 @@ HRESULT CorHost2::_CreateAppDomain( | |||
| pwzAppNiPaths = pPropertyValues[i]; | |||
| } | |||
| else | |||
| if (wcscmp(pPropertyNames[i], W("OVERRIDE_DEFAULT_STACK_SIZE")) == 0) | |||
There was a problem hiding this comment.
Nit: Make it just DEFAULT_STACK_SIZE ? `
|
It looks like (from the commit history) we have an environment variable 🎉. So just to confirm, the |
Yes. |
|
It should be |
Right |
|
Thanks! |
Add ability to override stack size on all platforms * Add DEFAULT_STACK_SIZE property * Make environment variable affect all platforms
Add ability to override stack size on all platforms * Add DEFAULT_STACK_SIZE property * Make environment variable affect all platforms Commit migrated from dotnet/coreclr@6a85ca5
Add code to override process default stack size for VM from the property bag
Fixes #21450
/cc @jkotalik