Should Vector<T>.Count be readonly?

[benaadams]

[JitIntrinsic]
public static int Count
{
    get
    {
        return s_count;
    }
}
private static int s_count = InitializeCount();

If hardware accelerated is false will s_count still be Jitted to a const even though its not readonly? Or should it be readonly?

[mellinoe]

Just from a style perspective we should probably change this to a readonly field. As for the JIT'd code, I'm not sure there's a guarantee it will be treated as a constant even with the change, although I would think it much more likely/possible in that case. Perhaps @CarolEidt can chime in. The special-case SIMD code-path surely has very special handling for this member in particular.

[CarolEidt]

Sadly, I don't believe that the JIT does anything special with readonly fields. This is something I think would be potentially quite useful to do - i.e. if the class constructor has been called, then the JIT could get the value and use it as a constant. Caveat - even though I'm currently the "keeper" of the spec, I haven't done the due diligence to validate that it would actually be legal to do so (i.e. I think, but don't know for certain, that readonly is more than just a "hint").

All that said, I agree that making it readonly is the right thing to do.

[stephentoub]

I don't believe that the JIT does anything special with readonly field

My understanding is that it actually does for readonly statics of primitive types, doing exactly what you cite: "if the class constructor has been called, then the JIT could get the value and use it as a constant".

[CarolEidt]

Excellent - I just found that in the importer code! All the more reason to make this readonly.

[benaadams]

Though various iterations aspnet/KestrelHttpServer#524 (comment) I've found the first access to the readonly static pays a cost in that the jit bakes in the initialization checks to that function. It mostly seems to be function backed inits; though had the same effect using BitConverter.IsLittleEndian so it might just be anything.

After the first access all the later jitted functions use consts. So; the trick seems to be to access the readonly statics in one off startup functions; rather than in a tight loop function so the static initialization checks are just run once rather than continuously executed.

[benaadams]

Would probably be better it the jitter just resolved the value/function and dropped the guard checks; as if its compiling the function it likely means its just about to be run; though I don't know if it has the ability to run the code its generating.

[stephentoub]

pays a cost in that the jit bakes in the initialization checks to that function. It mostly seems to be function backed inits

It sounds like you're talking about the differences implied by beforefieldinit, which the C# compiler will emit on types that lack an explicit cctor.

Would probably be better it the jitter just resolved the value/function and dropped the guard checks

Which guard checks? The JIT will use constant propagation and the like to eliminate unnecessary blocks based on (limited) compilation-time analysis.

[benaadams]

Which guard checks?

    83:                     case 102:
    84:                         return _bytesStatus102;
00007FFEA7A8DF42  mov         rcx,7FFEA7970B28h  
00007FFEA7A8DF4C  mov         edx,3Fh  
00007FFEA7A8DF51  call        00007FFF0757BC50  
00007FFEA7A8DF56  mov         rcx,20AED0D7BB0h  
00007FFEA7A8DF60  mov         rax,qword ptr [rcx]  
00007FFEA7A8DF63  jmp         00007FFEA7A8E749  
    85:                     case 200:
    86:                         return _bytesStatus200;
00007FFEA7A8DF68  mov         rcx,7FFEA7970B28h  
00007FFEA7A8DF72  mov         edx,3Fh  
00007FFEA7A8DF77  call        00007FFF0757BC50  
00007FFEA7A8DF7C  mov         rcx,20AED0D7BB8h  
00007FFEA7A8DF86  mov         rax,qword ptr [rcx]  
00007FFEA7A8DF89  jmp         00007FFEA7A8E749  
    87:                     case 201:
    88:                         return _bytesStatus201;
00007FFEA7A8DF8E  mov         rcx,7FFEA7970B28h  
00007FFEA7A8DF98  mov         edx,3Fh  
00007FFEA7A8DF9D  call        00007FFF0757BC50  
00007FFEA7A8DFA2  mov         rcx,20AED0D7BC0h  
00007FFEA7A8DFAC  mov         rax,qword ptr [rcx]  
00007FFEA7A8DFAF  jmp         00007FFEA7A8E749  

vs (the Constants. are the same as above e.g. readonly static just pre accessed)

    31:                     case 102:
    32:                         return Constants.HeaderBytesStatus102;
00007FFE9ED7E21A  mov         rax,1A2D68F7A48h  
00007FFE9ED7E224  mov         rax,qword ptr [rax]  
00007FFE9ED7E227  jmp         00007FFE9ED7E639  
    33:                     case 200:
    34:                         return Constants.HeaderBytesStatus200;
00007FFE9ED7E22C  mov         rax,1A2D68F7A50h  
00007FFE9ED7E236  mov         rax,qword ptr [rax]  
00007FFE9ED7E239  jmp         00007FFE9ED7E639  
    35:                     case 201:
    36:                         return Constants.HeaderBytesStatus201;
00007FFE9ED7E23E  mov         rax,1A2D68F7A58h  
00007FFE9ED7E248  mov         rax,qword ptr [rax]  
00007FFE9ED7E24B  jmp         00007FFE9ED7E639  

It sounds like you're talking about the differences implied by beforefieldinit

Yes, though I don't fully understand the issues was a little confused on by the advice around it; which seemed to be: cctor bad, beforefieldinit good - though with beforefieldinit the first function jitted seems to have these costs applied to it (didn't investigate the cctor to deeply as didn't know what I was looking for)

Might be a different scenario with readonly statics though?

[stephentoub]

Which guard checks?

I'm not seeing that. I grabbed the class from https://github.com/benaadams/KestrelHttpServer/blob/dc9f5c433b934bbfd75772e39013891137e77b24/src/Microsoft.AspNet.Server.Kestrel/Http/ReasonPhrases.cs and put it into a console app that just does Console.WriteLine(ReasonPhrases.ToStatusCode(1234)), and I get this:

                    case 102:
                        return _bytesStatus102;
00007FFD82D7133A  mov         rax,2306A5D59A8h  
00007FFD82D71344  mov         rax,qword ptr [rax]  
00007FFD82D71347  jmp         00007FFD82D71781  
                    case 200:
                        return _bytesStatus200;
00007FFD82D7134C  mov         rax,2306A5D59B0h  
00007FFD82D71356  mov         rax,qword ptr [rax]  
00007FFD82D71359  jmp         00007FFD82D71781  
                    case 201:
                        return _bytesStatus201;
00007FFD82D7135E  mov         rax,2306A5D59B8h  
00007FFD82D71368  mov         rax,qword ptr [rax]  
00007FFD82D7136B  jmp         00007FFD82D71781  

Am I misunderstanding?

[benaadams]

Coreclr x64? I did see different behaviour with assigning Vector<T>.Count to a readonly static on x86 (where it turned to a const) and coreclr x64 (where it had the checks); but I assumed that was due to it being a [JitIntrinsic] and IsHardwareAccelerated going down a different path but maybe not?

[benaadams]

Also might need to be an instance method rather than static method to exhibit the behaviour.

[stephentoub]

Coreclr x64?

Hmm, actually, I was using the full framework x64 (.NET 4.6.1). @CarolEidt, is there something that would make .NET 4.6.1 64-bit different from coreclr x64 in this regard? I just tried with the latest coreclr x64, and I'm seeing the worse result, whereas with .NET 4.6.1 I see the better result.

[benaadams]

@stephentoub @CarolEidt added coreclr issue https://github.com/dotnet/coreclr/issues/2526