Discussion: Non Zero Stackalloc

[Drawaes]

Motivation

Stackalloc is often used in performance critical code where every operation counts. Today you have to pay the price of zeroing the memory even if you know you will overwrite the entire space, paying the memory write price 2x.

Current Situation

While the current documentation for stackalloc states you should not rely on it being zero'd I imagine that plenty of code has been written relying on it. As this code often has few checks and balances for performance reasons I don't think the current behaviour can be changed without risk.

Proposed Solution

A keyword could be added to modify the stackalloc command. This should be simple to parse because it will only occur after the stackalloc keyword. This would leave existing behaviour as it and allow an opt in to not zeroing the memory.
e.g.
byte* = stackalloc dirty byte[16];

If a runtime couldn't support this or hadn't been updated it could just be ignored and the zeroing would occur with no harm (other than performance).

Related issues

CoreClr 1279
CorerClr 8542

Potential implementation

If any method contains a dirty stackalloc the init flag will be left unset and the compiler would emit manual initialization for all other local vars and stackallocs in the method.

This would not require a CLR change, or extra instructions and as the method contains unsafe anyway it is already not verifiable.

Updates

1. Changed proposed keyword
2. Added potential implementation

[tannergooding]

I'm definitely in favor of the proposal, but not the nonzero keyword (although I don't have a better proposal either). Maybe we could just reuse fixed: byte* x = fixed byte[16].

[Drawaes]

I am not hardline on any particular name. The only thing it needs to convey is that you aren't getting what most would consider "normal" behaviour in .net. So it needs to be clear. If you turn the data into a struct for instance you could get anything... (ahh found c memories) :P I am not sure fixed conveys that but I don't thing nonzero is right either..

Naming is hard ;)

[jnm2]

raw? uninitialized, dirty? garbage or junk?

I kinda like dirty. Or noinit.

I can't see reusing a keyword here, none make any sense. :-(
With a contextual keyword there would be a super high risk of collision, but you could take the same tack as with _ I guess. (Edit: duh, forgot about the the keyword stackalloc.)

[jnm2]

And maybe one day we'll get to use var x = new noinit T[n]; in unsafe contexts :D

[Drawaes]

I was thinking rather than what your are or are not doing a description is better... actually dirty is what I thought of as well... plus there would be a small smile on my face to write

Byte* b = stackalloc dirty byte [10]

[HaloFour]

The bigger challenge might be coming up with the name of the IL modifier required to make this possible. Currently stackalloc is just emitted as the IL opcode localloc which does both the allocation and the zero-init. With that CLR change there really isn't anything that the C# compiler can do.

[Drawaes]

Collisions should be rare and it's pretty clear that it's an extra word there isn't more possible modifiers in a stackalloc case. If there is a better way I am all ears!

[tannergooding]

@jnm2, even with contextual, I don't think the collision risk exists.

stackalloc <context-keyword> type[] isn't allowed today, so it should always be unambiguous.

In either case, I think the bigger question is, how is this expected to work on the CoreCLR side, without breaking other expected behavior in C#.

Currently (at least according to the ECMA spec), localloc will always initialize to 0 if the init flag is set on the .locals declaration for the method. Otherwise, the memory is undefined... Does the CLR guarantee it won't initialize the memory to 0 if the init flag isn't set?

Currently (as best as I can tell) C# always sets the init flag, even though it also tries to error if you haven't initialized the field first.

[tannergooding]

@HaloFour, there shouldn't need to be a CLR change here (unless it currently sometimes initializes memory to zero, even if the init flag isn't set) -- See above.

[Drawaes]

Take a look at the second issue at the bottom of my initial comment (sorry on phone now or I would sort a link) but that's exactly what they are discussing. So if they sort it out there it would be good to have a way to access it rather than it being special magic.

[Drawaes]

First issue sorry.

[tannergooding]

Also, regardless of whether removing the init flag guarantees uninitialized memory on the CLR side, I'm wondering if removing the init flag will be considered "breaking", even in this case.

Today, the following code is disallowed in C#, due to the assigned/unassigned tracking:

unsafe static void Main(string[] args)
{
    int x;
    Method(out x);
}

static void Method(out int x)
{
    if (x == 0)
    {
    }
}

However, regardless of C# saying 'x' is unassigned in Method, the CLR will have initialized it to 0 (due to the .locals init flag). So, if Method was (hypotheticaly) a method in some other language, it could be dependent on the fact that x defaults to 0 (it may also not know or care about C# out, since it is really just [Out] ref at the IL level).

[Drawaes]

Is the init locals for an entire method only?

As a quick aside the ecma spec says stackalloc memory contents is undefined

[tannergooding]

As a quick aside the ecma spec says stackalloc memory contents is undefined

This is for C#:

While for CLI, it depends on the state of the .locals [init] flag:

Is the init locals for an entire method only?

Looking at the spec still, but it looks like the metadata table only has room for one .locals entry

[tannergooding]

It may be interesting to note the following:

localloc is always unverifiable, but this is likely why C# includes it by default

[tannergooding]

how would that work

@Drawaes, it would be treated as intrinsic by the runtime and be emitted as equivalent to a localloc instruction, but with appropriate initialization semantics.

In any case, for normal C# semantics (where variables have to be initialized before use), you can probably just strip the init flag from the methods. This shouldn't be a problem unless you have some code that is dependent on it being zero-initialized today.

If you want to strip the flags, here is a simple program to do so:

private static void StripLocalsInit(IEnumerable<string> assemblyPaths)
{
    foreach (var assemblyPath in assemblyPaths)
    {
        var assembly = AssemblyDefinition.ReadAssembly(assemblyPath);

        Console.WriteLine($"Stripping the locals `init` flag for {assembly.FullName}");

        foreach (var module in assembly.Modules)
        {
            if (module.HasTypes)
            {
                foreach (var type in module.Types)
                {
                    Process(type);
                }
            }
        }

        assembly.Write(assemblyPath + ".opt");
    }
}

private static void Process(TypeDefinition type)
{
    if (type.HasNestedTypes)
    {
        foreach (var nestedType in type.NestedTypes)
        {
            Process(nestedType);
        }
    }

    if (type.HasMethods)
    {
        foreach (var method in type.Methods)
        {
            Process(method);
        }
    }

    if (type.HasProperties)
    {
        foreach (var property in type.Properties)
        {
            Process(property.GetMethod);
            Process(property.SetMethod);

            if (property.HasOtherMethods)
            {
                foreach (var otherMethod in property.OtherMethods)
                {
                    Process(otherMethod);
                }
            }
        }
    }

    if (type.HasEvents)
    {
        foreach (var @event in type.Events)
        {
            Process(@event.AddMethod);
            Process(@event.InvokeMethod);
            Process(@event.RemoveMethod);

            if (@event.HasOtherMethods)
            {
                foreach (var otherMethod in @event.OtherMethods)
                {
                    Process(otherMethod);
                }
            }
        }
    }
}

private static void Process(MethodDefinition method)
{
    if ((method != null) && method.HasBody)
    {
        var body = method.Body;

        if (body.InitLocals)
        {
            Console.WriteLine($"    {method.FullName}");
            body.InitLocals = false;
        }
    }
}

Should just need to add a dependency on Mono.Cecil and it should work.

[HaloFour]

@tannergooding

I don't think that's a good idea. Since this is a method-wide flag that impacts both local initialization and stack allocation indiscriminately I think that appearing as an API would only make it really confusing as to what kind of impact the pseudo-call would have on other code. It would have to be illegal to combine both stackalloc and Unsafe.StackAlloc<T> since the compiler can't mix semantics but it would be far from obvious as to why that is the case.

[Drawaes]

Agreed on that. The example I have above (calling schannel) is the prime place that could catch you out where you have an array of structs elsewhere

[tannergooding]

I think that appearing as an API would only make it really confusing as to what kind of impact the pseudo-call would have on other code

How so, since it is an intrinsic, rather than an IL instruction it can have whatever semantics its contract defines. If it states that it behaves as the C/C++ alloca method and zeros the memory, or not, based on some initialize parameter it takes, it can do exactly that.

It would require runtime support, but it would be completely non-breaking and would be no different from calling alloca yourself as a P/Invoke (which is only unreasonable today due to the overhead of a P/Invoke, and possible any security checks that may or may not prevent this, not sure on that end).

[tannergooding]

To be clear, I am stating this would be a runtime intrinsic, not a compiler intrinsic. The compiler would continue emitting the init flag, as it always has, and would not break any existing code.

The runtime would recognize the Unsafe.StackAlloc call and optimize it away to just reserve the stack space (same as the alloca C method).

[HaloFour]

@tannergooding

To be clear, I am stating this would be a runtime intrinsic, not a compiler intrinsic. The compiler would continue emitting the init flag, as it always has, and would not break any existing code.

I see. I assumed that the compiler would recognize the call and omit the init from the .locals block as to achieve the behavior on the existing CLR without modification.

Are there any precedents for such "runtime intrinstics" like this?

[tannergooding]

The hardware intrinsics proposal is somewhat similar, although it is adding runtime intrinsics for Intel SIMD instructions.

There have always been some functions treated as intrinsic by the runtime (System.Math.Sqrt is treated as an intrinsic for sqrtsd on x86 hardware).

Some other members of the System.Runtime.CompilerServices.Unsafe class are treated as intrinsic as well. Some have IL implementations, but some also have no implementation and just have the execution engine output the appropriate code.

[tannergooding]

So, runtime intrinsics aren't really new, and are still being used in places, but I am not sure if there is something "exactly" like this.

[Drawaes]

FYI I like your "temp" fix above and will probably be using it 👍

So to give an example of a simple method

static unsafe void TestMethod()
{
    var b = stackalloc dirty byte[3];
    var c = stackalloc byte[4];
}

It's unverifiable today so we win/lose nothing by breaking verifiability (is that a word?)

So if we modify the IL like below then there should be no change in existing behavior and thus little risk

Most of the values we need are in the stack anyway (size and address) but might need a little shuffle.. not sure anyway it should be cheap as we would have payed it for the second stackalloc anyway (and the first).

[tannergooding]

That sounds about right.

As an aside, I've been wanting a tool that does post IL optimization on an assembly for sometime and I just created a repo to do this: https://github.com/tannergooding/ilopt. Right now it just exposes a StripLocalsInit option and should allow patching all methods or only against those that match a given pattern.

It does not currently insert initblk statements, but an option to do that shouldn't be crazy hard.

[Drawaes]

I think you are trying to tempt me into doing this aren't you :p

[tannergooding]

Only if you have time 😄; Otherwise, I'll take a look tonight or this weekend. I have a few simple optimizations I want to add anyways (things that the C# compiler doesn't do and that can be done trivially before the JIT gets it, like shortcutting comparisons, or using shift to multiply, etc) and this will be my long weekend project.

[Drawaes]

I will see u on gitter;)

[jcouv]

Tagging @VSadov @OmarTawfik

[KevinCathcart]

This proposal is almost completely obsoleted by the already approved #1738. This will add an attribute named SkipLocalsInitAttribute. This attribute can be assembly, class, or member level. And causes any stackalloc to skip zeroing, by omitting the init on .locals.

This is technically not a change in the language, only a change in the runtime, and in the compiler.
It has one minor downside vs this proposal, namely that is is not possible to specify one stackalloc in a method is zero initialed, while another is not.

I don't think that is actually a problem in practice. I feel like this can be closed as rejected in favor of #1738.