Need to pull pinning via &MemoryMarshal.GetReference

[JeremyKuhne]

C# 7.3 allows pinning spans directly. Pinning empty behaves differently than pinning the ref returned from MemoryMarshal.GetReference.

static unsafe void PinMe()
{
    int[] emptyArray = new int[0];
    Span<int> emptySpan = new Span<int>(emptyArray);
    fixed (int* ea = emptyArray)
    fixed (int* es = emptySpan)
    fixed (int* esr = &MemoryMarshal.GetReference(emptySpan))
    {
        Console.WriteLine($"Array: 0x{(ulong)ea:X}, Span: 0x{(ulong)es:X}, Address of ref span: 0x{(ulong)esr:X}");
    }
}

Yields something like:

Array: 0x0, Span: 0x0, Address of ref span: 0x27D97CD1588

dotnet/corefx#32072 (comment)

cc: @danmosemsft, @jkotas

[danmoseley]

@JeremyKuhne I tried doing this and get a bunch of test failures. Was this what you intended:
https://github.com/dotnet/corefx/compare/master...danmosemsft:memorymarshal?expand=1

Failures like

  Assertion Failed

     at System.IO.Compression.Deflater.SetInput(Byte* inputBufferPtr, Int32 count) in C:\git\corefx\src\System.IO.Compression\src\System\IO\Compression\DeflateZLib\Deflater.cs:line 111
     at System.IO.Compression.DeflateStream.WriteCore(ReadOnlySpan`1 buffer) in C:\git\corefx\src\System.IO.Compression\src\System\IO\Compression\DeflateZLib\DeflateStream.cs:line 539
     at System.IO.Compression.DeflateStream.Write(Byte[] array, Int32 offset, Int32 count) in C:\git\corefx\src\System.IO.Compression\src\System\IO\Compression\DeflateZLib\DeflateStream.cs:line 508

        System.ArgumentNullException : Array cannot be null.
        Parameter name: chars
        Stack Trace:
          E:\A\_work\104\s\src\System.Private.CoreLib\shared\System\Text\UTF8Encoding.cs(177,0): at System.Text.UTF8Encoding.GetByteCount(Char* chars, Int32 count)
          C:\git\corefx\src\Common\src\System\Security\Cryptography\AsnWriter.cs(1570,0): at System.Security.Cryptography.Asn1.AsnWriter.WriteCharacterStringCore(Asn1Tag tag, Encoding encoding, ReadOnlySpan`1 str)

Any idea what I did wrong?

[jkotas]

You have likely discovered real product bugs where the code does not handle empty spans with null pointers correctly. E.g. in the deflater case, I expect that you would see the same problem if you passed in Span.Empty with null pointer into the API.

[JeremyKuhne]

Any idea what I did wrong?

Likely nothing. This is the behavior difference- &MemoryMarshal.GetReference would return a valid pointer. & on an array of zero length returns null. There are likely a few places where we should bail out when we have nothing to do.

[AlexRadch]

@JeremyKuhne MemoryMarshal.GetReference does not normalize the pointer for empty Span to null. I don't know if this is a bug or a behavior.

You can use emptySpan them self or you can use hidden method emptySpan.GetPinnableReference()

[joperezr]

@JeremyKuhne is this something we want to do for 3.0? If not I would like to move it to future.

[stephentoub]

I'm not entirely sure what this issue is tracking.

• If there's a case where we're using MemoryMarshal.GetReference but it's causing (or has the potential to cause) problems because the resulting pointer is being passed to something that requires a null pointer for 0 length inputs, then there's a bug to be fixed and ideally we'd fix it for 3.0.
• Otherwise, MemoryMarshal.GetReference is more typing/code but slightly faster than the default GetPinnableReference used for pinning. Any changes moving between the two are definitely not necessary for 3.0, and changes moving from GetReference to the implicit GetPinnableReference would ideally come with perf numbers to indicate that the extra checks don't regress anything (unless it's in code where performance doesn't matter, non-hot paths, etc.)

[JeremyKuhne]

MemoryMarshal.GetReference is more typing/code but slightly faster than the default GetPinnableReference used for pinning

I did not realize it was slightly faster. If it was equivalent there is no reason to use it as it isn't as safe.

Maybe a better issue would be to ensure we have tests that cover "unused" inputs when using GetReference?

[joperezr]

From the original issue, I thought the original idea was to do this work for a better perf, but seems like when @danmosemsft attempted it, he flushed out some potential product bugs, so I guess I'm also confused @stephentoub . @JeremyKuhne what is the work that this is really tracking?

[JeremyKuhne]

what is the work that this is really tracking?

Not using an API that has an unexpected behavior as it is was causing bugs. Pinning empty arrays always returned null, as does directly pinning an empty span. GetReference does not. Some native APIs require that null is passed for empty arrays (see the linked bug in the description).

It was never about performance, I just assumed it wasn't going to negatively impact perf to just pin the span.

[joperezr]

I see. @JeremyKuhne do you plan on working on this? If not and this is not critical, we can move this to Future.

[xtqqczze]

In #45969, all instances of fixed (var a = &b.GetPinnableReference()) were replaced with fixed (var a = b).