Replace small stackallocs with collection literals#128389
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces several very small stackalloc scratch buffers with C# collection expressions targeting Span<T> / ReadOnlySpan<T>, and removes unsafe modifiers that were only present to enable those small stackallocs.
Changes:
- Replaced tiny
stackallocbuffers (e.g., 1–4 elements) with collection expressions (e.g.,[0, 0, 0, 0],['\0', '\0']) across multiple libraries. - Removed
unsafefrom a number of methods / local functions where it was only needed for thosestackallocs. - Kept existing behavior by continuing to use stack-based scratch spans for transient buffers (subject to compiler lowering), while simplifying method signatures.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexNode.cs | Removes an unsafe local function and replaces tiny scratch stackalloc spans with collection expressions. |
| src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs | Replaces small stackalloc spans used for category / set-char probing with collection expressions. |
| src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.StringSegment.cs | Removes unsafe from leftover-handling helpers and replaces small combined-buffer stackallocs with collection expressions. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X500DistinguishedNameBuilder.cs | Removes unsafe from AddCountryOrRegion and uses a 2-char collection expression buffer. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/LiteHash.Windows.cs | Replaces a 1-byte stackalloc scratch buffer with [0]. |
| src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs | Removes unsafe from the BigInteger(decimal) ctor and uses a 4-int collection expression buffer. |
| src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/DecimalUtilities.cs | Removes unsafe from helpers and uses 4-int collection expression buffers. |
| src/libraries/System.Private.Uri/src/System/UriHelper.cs | Removes unsafe from EscapeStringToBuilder and replaces a 4-byte scratch stackalloc with a collection expression. |
| src/libraries/System.Private.Uri/src/System/IriHelper.cs | Removes unsafe and replaces a 4-byte scratch stackalloc with a collection expression. |
| src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBinaryWriter.cs | Removes unsafe from decimal write paths and replaces 4-int stackalloc with collection expressions. |
| src/libraries/System.Private.CoreLib/src/System/Text/Encoding.Internal.cs | Removes unsafe from a virtual helper and updates a disabled #if false sample to use collection expressions. |
| src/libraries/System.Private.CoreLib/src/System/Text/DecoderNLS.cs | Removes unsafe from leftover-drain helpers and replaces 4-byte combined-buffer stackallocs with collection expressions. |
| src/libraries/System.Private.CoreLib/src/System/SearchValues/Strings/Helpers/AhoCorasick.cs | Removes unsafe from a helper and replaces a 2-char destination stackalloc with a collection expression. |
| src/libraries/System.Private.CoreLib/src/System/SearchValues/SearchValues.cs | Replaces a 4-char stackalloc scratch buffer with a collection expression. |
| src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs | Removes unsafe from _LoadObjectV1 and replaces a 4-int stackalloc with a collection expression. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs | Removes unsafe from EscapeStringToBuilder and replaces a 4-byte scratch stackalloc with a collection expression. |
| src/libraries/System.Private.CoreLib/src/System/IO/UnmanagedMemoryAccessor.cs | Replaces a 4-int stackalloc scratch buffer with a collection expression. |
| src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.cs | Removes unsafe from AddTitlecaseLetter and replaces a few 2-char scratch stackallocs with collection expressions. |
| src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Compiler/ILGen.cs | Removes unsafe from EmitDecimal and replaces a 4-int stackalloc with a collection expression. |
| src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeReaderStream.cs | Removes unsafe from ReadByte and replaces a 1-byte stackalloc with [0]. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/Zstandard/ZstandardStream.Decompress.cs | Removes unsafe from ReadByte and replaces a 1-byte stackalloc with [0]. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Writer/CborWriter.Tag.cs | Replaces 4-int stackalloc scratch buffers with collection expressions. |
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs | Removes unsafe from Id-related members / ctor and replaces tiny stackalloc spans with collection expressions. |
| src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLDecimal.cs | Removes unsafe from SqlDecimal(decimal) and replaces a 4-int stackalloc with a collection expression. |
| src/libraries/Common/src/System/Number.Formatting.Common.cs | Replaces a 4-int stackalloc scratch buffer with a collection expression. |
| src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptEncryptDecrypt.RSA.cs | Replaces a 1-byte stackalloc scratch span with a collection expression. |
|
Tagging subscribers to this area: @dotnet/area-meta |
|
Out of my own curiosity, with |
It's actually an improvement according to diffs:
The downside is one instruction (in all these cases) to zero a couple of bytes on stack. But the main motivation was to remove unsafe |
|
PTAL @MihaZupan @tannergooding removes 26 unsafe contexts. Not more than 4 elements to keep it readable. Long terms we need either an opposite of SkipLocalsInit, or Buffer.Allocate (safe variant) API. Or inline arrays, but they need some polishment to work nicely (e.g. |
| BCryptEncryptFlags dwFlags) | ||
| { | ||
| // BCryptEncrypt does not accept null/0, only non-null/0. | ||
| ReadOnlySpan<byte> notNull = stackalloc byte[1]; |
There was a problem hiding this comment.
I don't understand how this relates to unsafe. The PR description states "since stackallocs require unsafe context", but that's not true when the target is a span.
There was a problem hiding this comment.
They are unsafe in the unsafe evolution world due to [SkipLocalsInit] leaving the values in an undefined state (no definite assignment exists for stackalloc)
There was a problem hiding this comment.
To clarify, it is the combination of stackalloc + [SkipLocalsInit] which makes it unsafe.
stackalloc without [SkipLocalsInit] will continue to be safe. However, as @EgorBo points out stackalloc has its own pessimizations whether safe or not. We will see better codegen with using collection expressions for trivially small cases or [InlineArray] for larger cases.
There was a problem hiding this comment.
These are bytes. Every value is valid. What's unsafe about it any more than random?
There was a problem hiding this comment.
These are bytes. Every value is valid. What's unsafe about it any more than random?
The fact that the value is a kind of undefined (unitialized). There should be no possability to access a non-initialized value in safe C#
There was a problem hiding this comment.
I think unitialized variables are generally considered memory unsafe pretty much in every definition/language
The variable here is notNull and it is fully initialized.
But only for basic primitives (not bool). GC.AllocUnitializedArray and Unsafe.SkipInit have no overload that would only support such primitives
We're spending many person years making changes to the runtime, libraries, and language for this effort. Current lack of an overload is easily rectified as a drop in the bucket.
There was a problem hiding this comment.
These are bytes. Every value is valid. What's unsafe about it any more than random?
Yes theoretically the primitive types and some subset of structs are "any-bit-pattern is legal" and so there isn't really "uninitialized" state
However, we have no distinguishing factor for different T today and no plans for such a thing in v1 of the feature.
But then even if we did, using stackalloc is still worse for the JIT and language in terms of codegen and other factors, in part because it by spec (both runtime and language) must have a lifetime for the entire remainder of the method regardless of where it is allocated. Collection expressions and InlineArray have no such restrictions and no pessimizations to codegen for other reasons.
There was a problem hiding this comment.
The variable here is notNull and it is fully initialized.
It would be if it was ReadOnlySpan<byte> notNull = stackalloc byte[1] { 0 }; (initializer syntax) with SkipLocalsInit. I think unexpected unitialized values are bad as in they quite often behave like they're always initialized with the same value if the call stack is the same (and most of the time the value is 0) but then suddently some security feature is introduced or call chain is changed and an app breaks in an unexpected way - exactly like here https://silentsblog.com/2025/04/23/gta-san-andreas-win11-24h2-bug/ - a windows update broke GTA just by chaning a value in an uninitialized variable due to introduction of Kernel-mode Hardware-enforced Stack Protection
There was a problem hiding this comment.
Then another factor is that whether or not "any-bit pattern is legal" is fine is very dependent. Technically any bit pattern is legal for int, but the vast majority of int values do not allow negatives and may error if they are encountered. So it is not necessarily fine to just go into an algorithm with an "uninitialized" or "randomly initialized" Span<int>.
Uninitialized memory is unsafe and nearly every safe language agrees on that. C# is largely no different except for stackalloc and even then it is only if you combine stackalloc + SkipLocalsInit that you can be in that space.
There was a problem hiding this comment.
using stackalloc is still worse for the JIT and language in terms of codegen and other factors
I'm not arguing against the change. I'm arguing against the justification for the change.
Replace very small stackallocs with collection literals (since stackallocs require unsafe context).