-
Notifications
You must be signed in to change notification settings - Fork 833
Fix excessive StackGuard thread jumps #18971
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
❗ Release notes required
|
I added some benchmark results to the description.
Those look like this with this PR:
|
b2666a8
to
abc4ae1
Compare
Another informal benchmark I did was to compare build times of IcedTasks.Tests, which are heavy with resumable state machine CEs: released compiler
this PR:
The optimizations phase is significantly faster (6.7 s vs 13.6 s) and there are no stack guard thread switches at all when there were thousands before. |
As usual I'm installing this in VS to also test the IDE behavior. The MacOS intermittent test fail (StackOverflowRepro.fs does not actually stack overflow) is a coincidence, I believe. It seems dotnet on MacOS has a larger stack now? |
Per Vlad's suggestion I made the old, throwing version conditional, only for netstandard2.0. |
Yes, this is an environmental change, not caused by you. |
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.
This is well isolated and brings huge compilation speed gains 🥇 , especially to optimizer phase.
It is already too late to make into the first GA releases of NET10/VS2026 respectively, but I am eager to port this to a subsequent release 👍 .
EDIT:
A bit more context on the process: While it is technically possible to convince the .NET team to accept last-minute additions or backport PRs, such changes would not benefit from any preview/RC1/RC2 testing.
The nature of this change directly affects the likelihood of stack overflows. For in-process editor tooling, that risk translates to crashing the entire IDE. In my view, this change improves the situation compared to the current approach of relying on hardcoded, estimated numbers. However, the API we are now using is not fully guaranteed. The documentation notes:
“The artificial stack limit is chosen by the common language runtime to ensure that enough space remains to throw an exception safely… [and that] stack space is large enough to execute the average .NET function.”
Having a preview period would allow us to gain much more confidence through real-world testing across F# programs and across different OSes and platforms.
I tested it for a day in VS. Works fine, no problems, good performance, no crashes. If anything, the built-in limit is in some cases way more conservative than what we had. if depth.Value < 40 || StackGuard.IsStackSufficient() then
... but that would be more risk for very little gain. There's a |
I would not do it, I prefer the change as it is 👍 . Btw. I will also remove the stackoverflow reproduction test (will keep the test which tests that we can avoid SO, will remove the one confirming SO in specific constelletation) |
Removed StackOverflow reproduction test due to host not crashing anymore reliably (which is not a bad thing).
by using
RuntimeHelpers.TryEnsureSufficientExecutionStack
instead of arbitrary guessed limits.Should improve #18970
ComputationExpressionBenchmarksbenchmark
withEmptyCache = true
:current main:
this PR:
Additionally, StackGuard stats are improved. If StackGuard does any thread switches, the minimal depth that triggered it is recorded. Visible in binlog when compiled with
--times
option:This effort is kindly sponsored by AmplifyingF# 🚀