Add SkipLocalsInit

[JamesNK]

Addresses #26586

Enable SkipLocalsInit almost everywhere in Kestrel, HTTP, hosting, routing. Selected because they're the "hot path" assemblies when it comes to startup time and per-request performance.

Test to see what breaks.

[davidfowl]

I'm not sure we'd easily be able to tell what breaks. I'd hate to cause an unexpected security bug because we enabled it assembly wide.

[JamesNK]

Stackallocs review. These are the stackallocs used in the assemblies with the attribute applied. I haven't reviewed any from code that is synced with runtime. They are already working with attribute applied.

• aspnetcore/src/Http/Headers/src/HeaderUtilities.cs

  Line 566 in c029cdf

  char* charBuffer = stackalloc char[_int64MaxStringLength];

  - every char is set in the used span portion. Note: The stackalloc length is a static readonly int. Why not a const?

• aspnetcore/src/Http/Routing/src/Matching/DfaMatcher.cs

  Line 46 in c029cdf

  Span<PathSegment> buffer = stackalloc PathSegment[_maxSegmentCount];

  - every PathSegment is set in FastTokenizer, then trimmed to used length

• aspnetcore/src/Http/WebUtilities/src/FormPipeReader.cs

  Line 336 in c029cdf

  Span<byte> buffer = stackalloc byte[(int)ros.Length];

  - CopyTo will set all bytes in the buffer. Note: should this stackalloc be a const size and then sliced?

• aspnetcore/src/Servers/IIS/IIS/src/Core/IO/AsyncWriteOperationBase.cs

  Line 40 in c029cdf

  var chunks = stackalloc HttpApiTypes.HTTP_DATA_CHUNK[chunkCount];

  - stackalloc will be the length of chunks in sequence. Each sequence is then updated using values in memory. This stackalloc is a struct - HttpApiTypes.HTTP_DATA_CHUNK - and only some fields on the struct are updated. Need to change this logic to set the entire struct value to zero out unset fields

• aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs

  Line 107 in c029cdf

  Span<char> decodedChars = stackalloc char[20];

  - Encoding.GetChars returns the number of chars updated, and that is sliced before passed to TryParse. Need to double check Encoding.GetChars always set all values.

• aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs

  Line 376 in c029cdf

  ? stackalloc byte[MaxPathBufferStackAllocSize].Slice(0, pathSegment.Length)

  - stackalloc is a const size, then sliced to path segment length. All values are set from path segment

• aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs

  Line 700 in c029cdf

  ? stackalloc byte[MaxPathBufferStackAllocSize].Slice(0, pathSegment.Length)

  - exact same code as Http2Stream. Need to reduce duplication

• aspnetcore/src/Shared/HttpSys/NativeInterop/SocketAddress.cs

  Line 248 in c029cdf

  ushort* rawShorts = stackalloc ushort[NumberOfIPv6Labels];

  - I believe this is fine, and each ushort is set from the buffer, but should have a closer look

• aspnetcore/src/Shared/WebEncoders/WebEncoders.cs

  Line 357 in c029cdf

  ? stackalloc char[bufferSize]

  - stackalloc char values are set with Convert.TryToBase64Chars and then sliced to returned length. Note: should this stackalloc be a const size and then sliced?

[davidfowl]

cc @GrabYourPitchforks Can you help us out here with an audit?

[GrabYourPitchforks]

TBH I was actually going to try to pull back on our use of SkipLocalsInit within the runtime repo, targeting specifically crypto and serialization and other "sensitive" assemblies.

[Tratcher]

Hosting.Abstractions is not on a perf-critical code path and from your review it doesn't contain any stackallocs. Why did you mark this assembly? The same goes for Server.Abstractions and likely some of the others.

[JamesNK]

I added it to all projects referenced by Kestrel.Core. Want them removed?

[Tratcher]

What does it get us if we're not using stackallocs in these assemblies?

[JamesNK]

@stephentoub Is there much benefit to SkipLocalsInit outside of stackalloc usages?

[stephentoub]

The benefit isn't limited to stackalloc, but is proportional to how much state is on the stack that needs to be cleared (and where clearing can be skipped). As such, it generally is much more impactful with stackalloc, but it can show up as a peanut butter cost elsewhere.

[SkipLocalsInit]
[Benchmark]
public unsafe int WithSkipLocal()
{
    Guid g1, g2, g3, g4, g5, g6;

    // Do something cheap that let's the benchmark compile and stuff not get optimized away
    Unsafe.SkipInit(out g1);
    Unsafe.SkipInit(out g2);
    Unsafe.SkipInit(out g3);
    Unsafe.SkipInit(out g4);
    Unsafe.SkipInit(out g5);
    Unsafe.SkipInit(out g6);
    return *(int*)&g1 + *(int*)&g2 + *(int*)&g3 + *(int*)&g4 + *(int*)&g5 + *(int*)&g6;
}

[Benchmark]
public unsafe int WithoutSkipLocal()
{
    Guid g1, g2, g3, g4, g5, g6;

    // Do something cheap that let's the benchmark compile and stuff not get optimized away
    Unsafe.SkipInit(out g1);
    Unsafe.SkipInit(out g2);
    Unsafe.SkipInit(out g3);
    Unsafe.SkipInit(out g4);
    Unsafe.SkipInit(out g5);
    Unsafe.SkipInit(out g6);
    return *(int*)&g1 + *(int*)&g2 + *(int*)&g3 + *(int*)&g4 + *(int*)&g5 + *(int*)&g6;
}

Method	Mean	Error	StdDev	Code Size
WithSkipLocal	0.2983 ns	0.0324 ns	0.0303 ns	33 B
WithoutSkipLocal	0.7273 ns	0.0015 ns	0.0013 ns	79 B

If the goal was biggest bang for the buck, though, it's definitely where larger stackallocs are being employed.

[stephentoub]

Another place it can show up impactfully is with fixed-size buffers, since as with stackallocs, they can often represent large regions of stack space that need to be cleared, e.g.

[SkipLocalsInit]
[Benchmark]
public unsafe int WithSkipLocal()
{
    Data d;
    Unsafe.SkipInit(out d);
    return d.Ptr[0];
}

[Benchmark]
public unsafe int WithoutSkipLocal()
{
    Data d;
    Unsafe.SkipInit(out d);
    return d.Ptr[0];
}

unsafe struct Data
{
    public fixed byte Ptr[1024];
}

Method	Mean	Error	StdDev	Code Size
WithSkipLocal	0.5009 ns	0.0047 ns	0.0041 ns	64 B
WithoutSkipLocal	14.8256 ns	0.0411 ns	0.0343 ns	132 B

[Tratcher]

Given it comes with a degree of risk, it seems better not to add it unless here's a demonstrated benefit. I also assume you'd be targeting the hot paths like per-request processing. Hosting doesn't have many of those.

[davidfowl]

I agree with @Tratcher but we also don't clear the array from the array pool so I don't see how this is different. Still I'd prefer doing this to some key placed in Kestrel rather than the entire assembly. That'll build more confidence.

[davidfowl]

@JamesNK are we doing this PR?

[JamesNK]

Do you want it?

I investigated how we're using stackalloc to make sure they're safe, but it sounds like a deeper review is required and there is little enthusiasm to do it.

[davidfowl]

I don't want it assembly wide, I want to put it on a few hot methods in kestrel to start.

[JamesNK]

Replaced by explicit PR.