Skip to content

Need to pull pinning via &MemoryMarshal.GetReference #27308

Closed

Description

@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

Activity

  1. danmoseley commented on Sep 4, 2018

    @danmoseley
    Contributor

    @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?

  2. jkotas commented on Sep 4, 2018

    @jkotas
    Member

    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.

  3. JeremyKuhne commented on Sep 4, 2018

    @JeremyKuhne
    MemberAuthor

    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.

  4. AlexRadch commented on Oct 9, 2018

    @AlexRadch
    Contributor

    @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()

  5. joperezr commented on Jun 5, 2019

    @joperezr
    Member

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

  6. stephentoub commented on Jun 5, 2019

    @stephentoub
    Member

    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.)
  7. JeremyKuhne commented on Jun 5, 2019

    @JeremyKuhne
    MemberAuthor

    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?

  8. joperezr commented on Jun 5, 2019

    @joperezr
    Member

    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?

  9. JeremyKuhne commented on Jun 5, 2019

    @JeremyKuhne
    MemberAuthor

    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.

  10. joperezr commented on Jun 21, 2019

    @joperezr
    Member

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

  11. transferred this issue fromdotnet/corefxon Jan 31, 2020
  12. added this to the Future milestone on Jan 31, 2020
  13. removed
    untriagedNew issue has not been triaged by the area owner
    on Jun 25, 2020
  14. 2 remaining items

  15. dotnet-policy-service commented on Jun 14, 2025

    @dotnet-policy-service
    Contributor

    Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

    This process is part of our issue cleanup automation.

  16. dotnet-policy-service commented on Jun 28, 2025

    @dotnet-policy-service
    Contributor

    This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

  17. removed this from the Future milestone on Jun 28, 2025
  18. locked and limited conversation to collaborators on Jul 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area-MetaenhancementProduct code improvement that does NOT require public API changes/additionshelp wanted[up-for-grabs] Good issue for external contributorstenet-reliabilityReliability/stability related issue (stress, load problems, etc.)

Type

No type

Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions