Perf: Investigate applying SkipLocalsInitAttribute

[JamesNK]

[SkipLocalsInit] can be used to improve performance - https://github.com/dotnet/roslyn/blob/master/docs/features/skiplocalsinit.md

Runtime applies this attribute on all their assemblies for 5.0 - https://github.com/dotnet/runtime/search?q=SkipLocalsInit

We should investigate whether there are performance gains in dotnet/aspnetcore, and if there are side effects when using the optimization. There are potential safety implications to not initializing locals to zero.

[davidfowl]

This might have a big impact on response writing, I remember @benaadams trying to reduce the stack size of ResponseHeaders.Append because of this.

[benaadams]

Anywhere that stackallocs Spans which don't contain references (which would be all as you can't current stackalloc spans with references); would be a good candidate; however the caveat is that no value can be assumed to be zero and every used item must be set.

[gfoidl]

no value can be assumed to be zero

That's why I wouldn't apply the attribute per assembly (module), rather apply it on a finer granularity (i.e. per method or class). So it's easier to verify that the code is still correct.
If after some iterations all classes are attributed with skip-locals-init, then one could change it to assembly level.

(I learned this at one project the harder way, because it's easy to forget the assumption of local being zero).

[JamesNK]

@stephentoub It would be great if you could share the process yall went through to apply the attribute across the runtime assemblies.

• Why did you decide to apply it on all assemblies?
• How did you do test and verify behavior?
• Worth it?
• What do you recommendation for aspnetcore?

[stephentoub]

The core libraries have been on this path for multiple releases now. For .NET Core 2.1, we used the IL linker to clear out locals init on around 15 assemblies; the goal at the time was to enable this for all of netcoreapp, but we only enabled it for those we successfully audited to help ensure we wouldn't regress anything. Then for .NET Core 3.0, we finished auditing the rest and enabled it for all assemblies in netcoreapp. For .NET 5.0, with the C# compiler gaining support for this and the addition of SkipLocalsInit, we simply switched the mechanism to use the compiler's built-in support rather than later stripping it out.

The auditing was primarily looking at every function that either a) did a stackalloc, b) took the address of a local, or c) called a P/Invoke involving an out parameter. For (a) and (b), code inspection was done to ensure that code wasn't reading the data and relying on it being zero (e.g. only filling in some of a span and skipping writing zeros because the span would have already contained zeros). For (c), it was examining either the native function being called or using its documentation to validate that the out would always be written in any situation where the call site was expecting it. And now moving forward, for libraries in netcoreapp, we just assume nothing is getting zerod automatically as part of the code we write. The auditing caught most issues, and tests caught a few stragglers.

Here's an example of the kind of issue you can hit:
https://github.com/dotnet/corefx/pull/26993/files#diff-71877b1591aaacae2b066a1b6a8ee439R505
The previous code was assuming the stackalloc would zero out everything, and so the code to fill in each EventData in the stackalloc'd space was only setting Ptr and Size, but not Reserved. Reserved also needed to be zero'd out.

And here's an example where when doing the initial auditing and testing, tests found an issue and we skipped enabling it for a particular build of the assembly; we then later went back to investigate, fixed the issue, and enabled it for this build as well:
dotnet/corefx#27411

As for impact, as Ben says, this has the greatest impact on stackalloc, or more generally, the more stack space is being used, the more impact it has. You can see this with a simple benchmarkdotnet repro:

[SkipLocalsInit]
[Benchmark]
public int With()
{
    Span<int> s = stackalloc int[256];
    return s[0];
}

[Benchmark]
public int Without()
{
    Span<int> s = stackalloc int[256];
    return s[0];
}

which on my machine produces:

Method	Mean	Error	StdDev
With	0.7513 ns	0.0228 ns	0.0202 ns
Without	30.0570 ns	0.5949 ns	0.5274 ns

but you can see the impact of it as peanut butter even without stackallocs.

stackalloc is used all over the place in dotnet/runtime, and the zero'ing ends up being a tax we just don't want to have to pay, nor have to annotate any method that does it with SkipLocalsInit (at the time we were deploying this, though, we didn't have an easy mechanism to even do it at a level finer-grained than assembly).

As for whether it's worth it for ASP.NET Core, I can all but guarantee you'll find places where it'll make a meaningful difference. And my guess is that once you do, you'll get a taste for it and decide to roll it out more broadly :) But in rolling it out to existing code, it does require effort to audit and test to ensure that garbage isn't slipping through.

[benaadams]

For .NET 5.0, with the C# compiler gaining support for this and the addition of SkipLocalsInit, we simply switched the mechanism to use the compiler's built-in support rather than later stripping it out.

Presumably this changed from it being enabled everywhere to only those annotated methods? (So methods using a lot of non-ref locals would start zero-init again?)

[stephentoub]

Presumably this changed from it being enabled everywhere to only those annotated methods?

We attribute at the module level.

[Pilchie]

Putting in area-servers because it seems like we should start with the lower level parts.

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.