Slices: Small code quality regression

[omariom]

@KrzysztofCwalina @jkotas @adamsitnik @atsushikan

Contract's methods where changed recently from throwing to calling ThrowHelper's methods.
The older JITs don't recognize those throw helpers as never return methods and keep the call in the hot area. That affects the portable Span.

This is how RequiresInRange looked before the change:

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void RequiresInRange(int start, uint length)
{
    if ((uint)start >= length)
    {
        throw NewArgumentOutOfRangeException();
    }
}

Now it does this:

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void RequiresInRange(int start, uint length)
{
    if ((uint)start >= length)
    {
        ThrowHelper.ThrowArgumentOutOfRangeException();
    }
}

This method compiled on .NET Framework 4.6.1..

[MethodImpl(MethodImplOptions.NoInlining)]
static byte SpanIndexer(Span<byte> slice, int index)
{
    return slice[index];
}

generates the following code:

sub         rsp,28h  
mov         eax,dword ptr [rcx+10h]
cmp         edx,eax  
jb          000007FE980138A8  
; next 3 lines should be located in the cold area  at the end of the method
call        000007FE98012390   
mov         rcx,rax  
call        000007FEF77B1C00  
mov         rax,qword ptr [rcx]  
mov         rcx,qword ptr [rcx+8]  
movsxd      rdx,edx  
add         rax,rcx  
movzx       eax,byte ptr [rax+rdx] 
add         rsp,28h  
ret  

[benaadams]

The previous approach did bring the exception construction and throw machinery into the method so regardless of hot/cold code its a much heavier inlined method.

Could #ifdef [MethodImpl(MethodImplOptions.NoInlining)] on the throw helper methods for full framework?

/cc @mikedn @AndyAyersMS

[omariom]

The previous approach did bring the exception construction and throw machinery into the method so regardless of hot/cold code its a much heavier inlined method.

No, the exception construction code was located in the cold area and that allows some optimizations.

[KrzysztofCwalina]

If we believe this is an issue worth fixing, we could ship three spans in the package: fast span for new coreclr, slow-span for old corclr, and slow-span' for clr.

[AndyAyersMS]

From what I understand the jit changes discussed here are going to show up in desktop CLR in 4.6.3 (at least for x64).

I doubt the "bad" block placement makes enough of a perf difference to justify complicating the source code. While it certainly looks better to move cold code to the end of the method the cycle benefits are typically small.

[ahsonkhan]

@omariom, is this issue still of concern?

[omariom]

No