Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Use Span to drop byte[1] allocations #15680

Merged
merged 4 commits into from
Jan 3, 2018
Merged

Use Span to drop byte[1] allocations #15680

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
663b7e6
Stream: stackalloc'd Span rather than byte array
benaadams Jan 2, 2018
679ed59
StubHelpers ConvertToManaged use stackalloc'd Span
benaadams Jan 2, 2018
fb4e6e2
feedback
benaadams Jan 3, 2018
6cd4db2
remove redundant assignment
benaadams Jan 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions src/mscorlib/shared/System/IO/PinnedBufferMemoryStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,12 @@ internal PinnedBufferMemoryStream(byte[] array)
{
Debug.Assert(array != null, "Array can't be null");

int len = array.Length;
// Handle 0 length byte arrays specially.
if (len == 0)
{
array = new byte[1];
len = 0;
}

_array = array;
_pinningHandle = GCHandle.Alloc(array, GCHandleType.Pinned);
// Now the byte[] is pinned for the lifetime of this instance.
// But I also need to get a pointer to that block of memory...
fixed (byte* ptr = &_array[0])
int len = array.Length;
fixed (byte* ptr = &MemoryMarshal.GetReference((Span<byte>)array))
Initialize(ptr, len, len, FileAccess.Read);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is span needed here? fixed works fine with zero-length arrays).

Copy link
Member

@jkotas jkotas Jan 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinning zero length array gives back null. Initialize method does not accept null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Though that could be addressed as well just by passing ptr != null ? ptr : (byte*)1 instead of ptr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there are several ways to address this. Another option in this case is to fix or delete the null check in the Initialize method.

  • (byte*)1 is a very unusual pointer value. If I saw it somewhere, I would be looking for memory corruption. It would be nice for the unmanaged pointers to either be null or point to valid memory.
  • The code size varies for the different options. I suspect that ptr != null ? ptr : (byte*)1 pattern is larger overall native code compared to MemoryMarshal.GetReference - we should measure.

Copy link
Member Author

@benaadams benaadams Jan 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As now (MemoryMarshal.GetReference) its

       call     GCHandle:InternalAlloc(ref,int):long
       or       rax, 1
       mov      qword ptr [rsi+96], rax
       lea      r8, bword ptr [rdi+16]
       mov      r9d, dword ptr [rdi+8]
       mov      bword ptr [rsp+28H], r8
       mov      rdx, r8
       mov      dword ptr [rsp+20H], 1
       movsxd   r8, ebx
       movsxd   r9, ebx
       mov      rcx, rsi
       call     UnmanagedMemoryStream:Initialize(long,long,long,int):this

Changing to

fixed (byte* ptr = array)
    Initialize(ptr != null ? ptr : (byte*)1, len, len, FileAccess.Read);

It becomes

       call     GCHandle:InternalAlloc(ref,int):long
       or       rax, 1
       mov      qword ptr [rsi+96], rax
       mov      gword ptr [rsp+28H], rdi
       mov      r8, gword ptr [rsp+28H]
       cmp      dword ptr [r8+8], 0
       jne      SHORT G_M3256_IG04
G_M3256_IG03:
       xor      r8, r8
       jmp      SHORT G_M3256_IG05
G_M3256_IG04:
       mov      r8, gword ptr [rsp+28H]
       cmp      dword ptr [r8+8], 0
       jbe      SHORT G_M3256_IG09
       mov      r8, gword ptr [rsp+28H]
       add      r8, 16
G_M3256_IG05:
       test     r8, r8
       jne      SHORT G_M3256_IG06
       mov      edx, 1
       jmp      SHORT G_M3256_IG07
G_M3256_IG06:
       mov      rdx, r8
G_M3256_IG07:
       mov      dword ptr [rsp+20H], 1
       movsxd   r9, ebx
       mov      r8, r9
       mov      rcx, rsi
       call     UnmanagedMemoryStream:Initialize(long,long,long,int):this

Which seems more involved?

}

Expand Down
3 changes: 2 additions & 1 deletion src/mscorlib/src/System/StubHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ unsafe static internal byte ConvertToNative(char managedChar, bool fBestFit, boo

static internal char ConvertToManaged(byte nativeChar)
{
byte[] bytes = new byte[1] { nativeChar };
Span<byte> bytes = new Span<byte>(ref nativeChar, 1);
bytes[0] = nativeChar;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytes[0] = nativeChar; is redundant

string str = Encoding.Default.GetString(bytes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in corelib. Presumably it can just use the Span(ref T, int) ctor. Or it could use DangerousCreate that just calls it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to figure out why DangerousCreate wasn't showing up in intellisense:

[EditorBrowsable(EditorBrowsableState.Never)]
public static Span<T> DangerousCreate(object obj, ref T objectData, int length);

This totally trolled me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KrzysztofCwalina, @ahsonkhan, why is this marked as EditorBrowsableState.Never? We seem to be too quick to do that. It's already named Dangerous... why do we need to hide it? Even if we wanted to hide it from the IntelliSense list, marking it as Never means that once you type out "DangerousCreate" and the open paren, you still don't get IntelliSense help with the argument list, making it difficult to use even after you've discovered it.

return str[0];
}
Expand Down