feat: drop redundant fixed for already pinned array

[unsafePtr]

fixed is redundant here, as array is already pinned. Small asm comparison to justify the change:
https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLQGED2AbbEDGMAlpgHYAyRMECU2yANDCFMgLYA+AAgEwCMAWABQXAAwACLnwB0AJTilibCNICSimpgAOAZRoA3IvhQBuYcK4BmST3HpxAb2HiX4566viFyKJHHIYBDhCcQAhODBIBD0Ac2VFAApgAE9qACpxABNYKAZxIkVxPFIYmAALPJTqAG0AXQB+L1JMmmxkgpjwyJoASncXJyFXYclrJChMsjbxKogMgBEc8QBeLJyzIZGXT3HJ0mmCmHFyCBLylaLT0rKNre2x6D3p2brGgFVm1vaSrqiLhRaCDaHV+NFurgAvuZNi5+m4YaMmj4/KDohA4qcjgBBBKHGYRKI6IgALwgPUccOG+igCHx3VpqwA4uhpFjcJh8LAIB8ClQiHQSRBMliELRkgAeWYAPiSBJoRNJeS0BVIQpA4kCcDJG0prjARAAHkLxElUnM1jAoBdgHKEH0EcNBncRlwAOziVUAdzCtti8RgCWylsqvsFIfpPXBWyhCJjwzhnm8vggPvpfsxYVxhRtacF5Kdd2ptJzfyZLLZ2A5XJ5pD5AtJwtFUAl0tlucV4mVpFVmXVmu1uthDtcRc7gQupvSPQAsjTkGU6NIPsiIFjMpkEAB5MAABRVQpFYoAonh/W2onlRJHB5J3V7U4T0WetIFw4Sw3SotfY8IIUA===

[ocoanet]

(byte*)Unsafe.AsPointer(ref buffer[0]) might be even faster. Also, caching the pointer to _currentBuffer might be more beneficial.

Edit: I just noticed that the assembly for Unsafe.AsPointer and Marshal.UnsafeAddrOfPinnedArrayElement are almost identical (for the happy path).

[ltrzesniewski]

Thank you for the PR!

Note that BufferSegmentProvider is only used on initialization unless you use LogMessagePoolExhaustionStrategy.Allocate, so it doesn't really need to be micro-optimized, but I like optimized stuff. 🙂

Let's see how CreateStandaloneSegment looks like:

With `fixed`

; Assembly listing for method ZeroLog.BufferSegmentProvider:CreateStandaloneSegment(int):ZeroLog.BufferSegment (FullOpts)
; Emitting BLENDED_CODE for generic X64 + VEX on Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 1 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:
       push     rsi
       push     rbx
       sub      rsp, 40
       xor      eax, eax
       mov      qword ptr [rsp+0x20], rax
       mov      rbx, rcx
       mov      esi, edx
 
G_M000_IG02:
       mov      ecx, esi
       mov      edx, 1
       call     [System.GC:<AllocateUninitializedArray>g__AllocateNewArrayWorker|77_0[byte](int,bool):byte[]]
       mov      gword ptr [rsp+0x20], rax
       test     rax, rax
       je       SHORT G_M000_IG04
 
G_M000_IG03:
       mov      ecx, dword ptr [rax+0x08]
       test     ecx, ecx
       je       SHORT G_M000_IG04
       lea      rcx, bword ptr [rax+0x10]
       jmp      SHORT G_M000_IG05
 
G_M000_IG04:
       xor      ecx, ecx
 
G_M000_IG05:
       mov      gword ptr [rbx], rax
       mov      qword ptr [rbx+0x08], rcx
       mov      dword ptr [rbx+0x10], esi
       mov      rax, rbx
 
G_M000_IG06:
       add      rsp, 40
       pop      rbx
       pop      rsi
       ret      
 
; Total bytes of code 76

With `Marshal.UnsafeAddrOfPinnedArrayElement`

; Assembly listing for method ZeroLog.BufferSegmentProvider:CreateStandaloneSegment(int):ZeroLog.BufferSegment (FullOpts)
; Emitting BLENDED_CODE for generic X64 + VEX on Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 1 inlinees with PGO data; 2 single block inlinees; 1 inlinees without PGO data

G_M000_IG01:
       push     rsi
       push     rbx
       sub      rsp, 40
       mov      rbx, rcx
       mov      esi, edx
 
G_M000_IG02:
       mov      ecx, esi
       mov      edx, 1
       call     [System.GC:<AllocateUninitializedArray>g__AllocateNewArrayWorker|77_0[byte](int,bool):byte[]]
       test     rax, rax
       je       SHORT G_M000_IG04
       lea      rcx, bword ptr [rax+0x10]
       mov      gword ptr [rbx], rax
       mov      qword ptr [rbx+0x08], rcx
       mov      dword ptr [rbx+0x10], esi
       mov      rax, rbx
 
G_M000_IG03:
       add      rsp, 40
       pop      rbx
       pop      rsi
       ret      
 
G_M000_IG04:
       mov      ecx, 0x1A4E9
       mov      rdx, 0xD1FFAB1E
       call     [CORINFO_HELP_STRCNS]
       mov      rcx, rax
       call     [System.ArgumentNullException:Throw(System.String)]
       int3     
 
; Total bytes of code 84

With `Unsafe.AsPointer`

; Assembly listing for method ZeroLog.BufferSegmentProvider:CreateStandaloneSegment(int):ZeroLog.BufferSegment (FullOpts)
; Emitting BLENDED_CODE for generic X64 + VEX on Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 1 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:
       push     rsi
       push     rbx
       sub      rsp, 40
       mov      rbx, rcx
       mov      esi, edx
 
G_M000_IG02:
       mov      ecx, esi
       mov      edx, 1
       call     [System.GC:<AllocateUninitializedArray>g__AllocateNewArrayWorker|77_0[byte](int,bool):byte[]]
       cmp      dword ptr [rax+0x08], 0
       jbe      SHORT G_M000_IG04
       lea      rcx, bword ptr [rax+0x10]
       mov      gword ptr [rbx], rax
       mov      qword ptr [rbx+0x08], rcx
       mov      dword ptr [rbx+0x10], esi
       mov      rax, rbx
 
G_M000_IG03:
       add      rsp, 40
       pop      rbx
       pop      rsi
       ret      
 
G_M000_IG04:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
 
; Total bytes of code 60

With `Unsafe.AsPointer` + `MemoryMarshal.GetArrayDataReference`

; Assembly listing for method ZeroLog.BufferSegmentProvider:CreateStandaloneSegment(int):ZeroLog.BufferSegment (FullOpts)
; Emitting BLENDED_CODE for generic X64 + VEX on Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 1 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:
       push     rsi
       push     rbx
       sub      rsp, 40
       mov      rbx, rcx
       mov      esi, edx
 
G_M000_IG02:
       mov      ecx, esi
       mov      edx, 1
       call     [System.GC:<AllocateUninitializedArray>g__AllocateNewArrayWorker|77_0[byte](int,bool):byte[]]
       cmp      byte  ptr [rax], al
       lea      rcx, bword ptr [rax+0x10]
       mov      gword ptr [rbx], rax
       mov      qword ptr [rbx+0x08], rcx
       mov      dword ptr [rbx+0x10], esi
       mov      rax, rbx
 
G_M000_IG03:
       add      rsp, 40
       pop      rbx
       pop      rsi
       ret      
 
; Total bytes of code 50

Note that UnsafeAddrOfPinnedArrayElement is basically a wrapper over Unsafe:

public static unsafe IntPtr UnsafeAddrOfPinnedArrayElement<T>(T[] arr, int index)
{
    ArgumentNullException.ThrowIfNull(arr);

    // Unsafe.AsPointer is safe since array must be pinned
    void* pRawData = Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(arr));
    return (IntPtr)((byte*)pRawData + (uint)index * (nuint)sizeof(T));
}

So I'd say that Unsafe + GetArrayDataReference wins in terms of code size, and I also tend to use Unsafe more frequently as it's lower-level than Marshal, so I'd also prefer to switch to Unsafe if that's ok for you.

[ltrzesniewski]

Please use Unsafe.AsPointer + MemoryMarshal.GetArrayDataReference instead of Marshal.UnsafeAddrOfPinnedArrayElement, it'll be more in line with the rest of the code (see the suggestion in my comment below).

Thank you!

[ltrzesniewski]

Wait, (byte*)Unsafe.AsPointer(ref buffer[0]) actually makes the unit tests fail in release mode, let me check that.

[ltrzesniewski]

Ok, the tests call CreateStandaloneSegment with a bufferSize of 0 😞 so you'd need to write the following:

public static BufferSegment CreateStandaloneSegment(int bufferSize)
{
    if (bufferSize == 0)
        return default;

    var buffer = GC.AllocateUninitializedArray<byte>(bufferSize, pinned: true);
    var data = (byte*)Unsafe.AsPointer(ref buffer[0]);
    return new BufferSegment(data, bufferSize, buffer);
}

or even better:

public static BufferSegment CreateStandaloneSegment(int bufferSize)
{
    var buffer = GC.AllocateUninitializedArray<byte>(bufferSize, pinned: true);
    var data = (byte*)Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(buffer));
    return new BufferSegment(data, bufferSize, buffer);
}

This happens only in test code though, so no big deal.

And the GetSegment method could be written like this:

public BufferSegment GetSegment()
{
    lock (_lock)
    {
        var buffer = _currentBuffer;

        if (_currentSegment >= _segmentCount || buffer is null)
        {
            _currentBuffer = buffer = GC.AllocateUninitializedArray<byte>(BufferSize, pinned: true);
            _currentSegment = 0;
        }

        var offset = _segmentSize * _currentSegment++;
        var data = (byte*)Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(buffer)) + offset;
        return new BufferSegment(data, _segmentSize, buffer);
    }
}

[ltrzesniewski]

I'll merge this and inline UnsafeAddrOfPinnedArrayElement in a subsequent commit. Thank you for your contribution! 🙂

[unsafePtr]

Thank you! I know it's not a hot path, but tought there is no need to involve GC bookkeeping. Marshal.UnsafeAddrOfPinnedArrayElement is similar to the manual implementation, but it introduces a null check (ThrowIfNull) which the JIT doesn’t eliminate, resulting in slightly less optimal codegen compared to the intrinsic-based approach.