GetPinnableReference on String.cs

[simplejackcoder]

This is a feature improvement for https://github.com/dotnet/coreclr/issues/23359

Please let me know if there is more to do this seem too easy!

[GrabYourPitchforks]

@jkotas It's typical that we'd put [EditorBrowsable(EditorBrowsableState.Never)] on such a method, correct? This pattern is after all primarily intended for compiler use.

[jkotas]

Yes, this should follow the pattern used for Span.

Also, this should be marked as [NonVersionable] to match OffsetToStringData and avoid regressing R2R code quality.

[GrabYourPitchforks]

@simplejackcoder - For reference to what Jan and I were discussing above, see specifically these two lines for the appropriate attribute patterns:

coreclr/src/System.Private.CoreLib/shared/System/ReadOnlySpan.Fast.cs

Line 172 in 2f4a756

[EditorBrowsable(EditorBrowsableState.Never)]

coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs

Line 106 in a593024

[System.Runtime.Versioning.NonVersionable]

[GrabYourPitchforks]

Prefer fully-qualifying these here rather than introducing using aliases at the top of the file.

[jkotas]

This is regressing codegen for number of fixed statements in CoreLib. For example: Convert:FromBase64String(ref):ref. Before: 93 bytes. After: 97 bytes. The problem seems to be unnecessary stack spills:

Before:

       mov      edx, dword ptr [rcx+8]
       mov      rcx, rax
       call     [Convert:FromBase64CharPtr(long,int):ref]

After:

       mov      qword ptr [rsp+20H], rdx // unnecessary stackspill
       mov      edx, dword ptr [rcx+8]
       mov      rcx, qword ptr [rsp+20H]
       call     [Convert:FromBase64CharPtr(long,int):ref]

@dotnet/jit-contrib Thoughts?

[AndyAyersMS]

Possibly a consequence of #15706. The jit is very conservative now when it sees a mixture of calls and references to pinned locals.

What does the IL look like?

[GrabYourPitchforks]

@jkotas You might remember that same stack-spill pattern with the recent ASCIIEncoding work I was doing. In particular, I had to write this code:

public unsafe int GetCharCount(ReadOnlySpan<byte> bytes)
{
    fixed (byte* pBytes = &MemoryMarshal.GetReference(bytes))
    {
        return GetCharCountFast(pBytes, bytes.Length);
    }
}

Like this instead to avoid the unnecessary stack spills:

public unsafe int GetCharCount(ReadOnlySpan<byte> bytes)
{
    ref byte rBytes = ref MemoryMarshal.GetReference(bytes);
    int length = bytes.Length;

    // Don't reference 'pBytes' once it's pinned, as all accesses read from the stack rather than registers
    fixed (byte* pBytes = &rBytes)
    {
        // rBytes and length are both still enregistered here
        return GetCharCountFast((byte*)Unsafe.AsPointer(ref rBytes), length);
    }
}

[jkotas]

What does the IL look like?

.method public hidebysig static uint8[]  FromBase64String(string s) cil managed
{
  // Code size       44 (0x2c)
  .maxstack  2
  .locals (char* V_0,
           char& pinned V_1)
  IL_0000:  ldarg.0
  IL_0001:  brtrue.s   IL_000e
  IL_0003:  ldstr      "s"
  IL_0008:  newobj     instance void System.ArgumentNullException::.ctor(string)
  IL_000d:  throw
  IL_000e:  ldarg.0
  IL_000f:  brtrue.s   IL_0015
  IL_0011:  ldc.i4.0
  IL_0012:  conv.u
  IL_0013:  br.s       IL_001e
  IL_0015:  ldarg.0
  IL_0016:  call       instance char& modreq(System.Runtime.InteropServices.InAttribute) System.String::GetPinnableReference()
  IL_001b:  stloc.1
  IL_001c:  ldloc.1
  IL_001d:  conv.u
  IL_001e:  stloc.0
  IL_001f:  ldloc.0
  IL_0020:  ldarg.0
  IL_0021:  callvirt   instance int32 System.String::get_Length()
  IL_0026:  call       uint8[] System.Convert::FromBase64CharPtr(char*,
                                                                 int32)
  IL_002b:  ret
}

I had to write this code, like this instead

Did you happen to open a JIT bug on this? Nobody should be writing convoluted code like this to get good codegen for this.

[GrabYourPitchforks]

Did you happen to open a JIT bug on this? Nobody should be writing convoluted code like this to get good codegen for this.

No. The ultimate decision was that it wasn't worth the complexity to save three or four instructions, and we backed out the change and just kind of forgot about it.

[GrabYourPitchforks]

I opened #23437 to track the underlying JIT issue.

[simplejackcoder]

Does it mean that this PR will regress performance and should not currently be accepted?

[GrabYourPitchforks]

@simplejackcoder There's nothing wrong with the PR in my opinion. It's very straightforward and is a useful API addition. I'd recommend leaving the PR open for now while we investigate the JIT behavior on our end.

[AndyAyersMS]

Left some notes over in #23437, but basically this is a register allocator issue. We have conflicting lifetimes and the allocator spills the pinned value. Diff from the "before" picture is largely that we are now pinning the byref instead of the string itself; this alters the lifetimes the allocator sees and presents a different interference graph.

Diffs will be most prominent in a tiny method like FromBase64String that takes a string as arg, pins it, and passes a span like pair to a callee. This particular case can be improved by reordering the arg list in FromBase64CharPtr so it is (length, ptr) -- this will reduce the conflict set.

For this bit of framework code, we might also consider hoisting the span creation up so that FromBase64CharPtr and related accept a span, this would avoid pinning and pointers all together.

I'm going to look into why the allocator isn't anti-preferencing RDX as the target register; doing so might encourage the allocator to use an extra register to get around the conflict like it does in the before case. But at this point it looks like there might not be an easy fix on the jit side.

We could also address this via load CSEs of the DNER pin slot (and that would have many other benefits) but that is too risky/ambitious for 3.0. Or (similar) teach the allocator that there is no point spilling a copy of a DNER local as the value can just be reloaded at its use point.

[AndyAyersMS]

I'm going to look into why the allocator isn't anti-preferencing RDX as the target register;

Should have been obvious, but since we pin the byref via a local and then use the local, and the local is in memory as DNER (do not enregister), there's no way for the allocator to connect the byref creation with its ultimate use at the call.

So I don't see any short-term fix in the jit for this. @CarolEidt feel free to jump in, as I may have overlooked the ability of the allocator to anticipate the need to spill here and do something more clever (see notes on the "string case" over in #23437).

I think the diffs from this change are generally a wash. There are a few ugly cases like the above, and other cases where codegen is somewhat better (redundant null checks are removed). Perf for Convert:FromBase64String is unlikely to be impacted much, if any, as most of the heavy lifting happens in the helper called after the string is pinned.

[jkotas]

Ok, let's take this