libplctag / libplctag.NET

A .NET wrapper for libplctag.
https://libplctag.github.io/
Mozilla Public License 2.0
220 stars 55 forks source link

`(ReadOnly)Span<byte>` overloads on `GetBuffer` to reduce allocations and make using existing buffers easier #423

Open timyhac opened 2 weeks ago

timyhac commented 2 weeks ago

Would be beneficial if there were (ReadOnly)Span<byte> overloads to reduce allocations and make using existing buffers easier. I'm happy to do a PR to implement if you are interested.

The native method declarations in C# would need to change since they use byte[] but won't be too much trouble since the native API uses a pointer anyway. https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag.NativeImport/NativeMethods.cs#L247-L251

https://github.com/libplctag/libplctag/blob/c55bc5876d938dda1c609750cde5ae4812d7b8a8/src/lib/libplctag.h#L505-L506

LIB_EXPORT int plc_tag_set_raw_bytes(int32_t id, int offset, uint8_t *buffer, int buffer_length);
LIB_EXPORT int plc_tag_get_raw_bytes(int32_t id, int offset, uint8_t *buffer, int buffer_length);

There are a few ways to do this, one approach can be found in Memory\ and Span\ usage guidelines under:

Rule #9: If you're wrapping a synchronous p/invoke method, your API should accept Span as a parameter.

Alternatively and ideally, would use P/Invoke source generation but this requires .NET 7+. Something like:

[LibraryImport(DLL_NAME, EntryPoint = nameof(plc_tag_get_raw_bytes))]
[UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])]
public static partial int plc_tag_get_raw_bytes(Int32 tag_id, int start_offset, [MarshalUsing(CountElementName = nameof(buffer_length))] out Span<byte> buffer, int buffer_length);

[LibraryImport(DLL_NAME, EntryPoint = nameof(plc_tag_set_raw_bytes))]
[UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])]
public static partial int plc_tag_set_raw_bytes(Int32 tag_id, int start_offset, ReadOnlySpan<byte> buffer, int buffer_length);

This would also make it possible to create ReadAsync and WriteAsync with Memory<byte> overloads too. Similar to Stream.ReadAsync-system-threading-cancellationtoken)) and Stream.WriteAsync-system-threading-cancellationtoken))

public ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default);
public ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default);

Originally posted by @MitchRazga in https://github.com/libplctag/libplctag.NET/issues/394#issuecomment-2410778370

timyhac commented 2 weeks ago

@mitchrazga - this idea is worth exploring and I would be keen to see what improvements this would make possible.

I had a go at benchmarking this but was not able to see any allocation (other than the source buffer, which won't appear in the measurement) when using the current native method declaration nor when using a pointer-based one. This makes sense to me; that P/Invoke merely pins the buffer in place as opposed to making a copy.

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Security;

public class Program
{
    public static void Main(string[] args)
    {
        var summary = BenchmarkRunner.Run<Tests>();
    }
}

[SimpleJob(RuntimeMoniker.Net472, baseline: true)]
[SimpleJob(RuntimeMoniker.Net80)]
[MemoryDiagnoser]
public class Tests
{
    private byte[] data;
    private int tag_id;

    [Params(10, 30, 100, 300, 1000)]
    public int N;

    [GlobalSetup]
    public void Setup()
    {
        tag_id = Native.plc_tag_create("protocol=ab-eip&gateway=%s&path=1,0&plc=controllogix&name=@raw", 1000);
        var result = Native.plc_tag_set_size(tag_id, N);
        data = new byte[N];
    }

    [Benchmark]
    public int Original()
    {
        return plctag.Original(tag_id, 0, data, data.Length);
    }

    [Benchmark]
    public int BytePtr()
    {
        return plctag.BytePtr(tag_id, 0, data);
    }
}

public static class plctag
{
    public static int Original(Int32 tag_id, int start_offset, byte[] buffer, int buffer_length)
    {
        return Native.Original(tag_id, start_offset, buffer, buffer_length);
    }

    public static int BytePtr(Int32 tag_id, int start_offset, Span<byte> buffer)
    {
        unsafe
        {
            fixed (byte* ptr = buffer)
            {
                return Native.BytePtr(tag_id, start_offset, ptr, buffer.Length);
            }
        }
    }

}

[SuppressUnmanagedCodeSecurity]
static class Native
{
    const string DLL_NAME = "plctag";
    const string ENTRYPOINT = "plc_tag_get_raw_bytes";

    [DllImport(DLL_NAME, EntryPoint = nameof(plc_tag_create), CallingConvention = CallingConvention.Cdecl, ExactSpelling = true, CharSet = CharSet.Ansi)]
    public static extern Int32 plc_tag_create([MarshalAs(UnmanagedType.LPStr)] string lpString, int timeout);

    [DllImport(DLL_NAME, EntryPoint = nameof(plc_tag_set_size), CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
    public static extern int plc_tag_set_size(Int32 tag, int new_size);

    [DllImport(DLL_NAME, EntryPoint = ENTRYPOINT, CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
    public static extern int Original(Int32 tag_id, int start_offset, [Out] byte[] buffer, int buffer_length);

    [DllImport(DLL_NAME, EntryPoint = ENTRYPOINT, CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
    public static extern unsafe int BytePtr(Int32 tag_id, int start_offset, byte* buffer, int buffer_length);

}

The results of this are:

// * Summary *

BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.4249/23H2/2023Update/SunValley3)
11th Gen Intel Core i7-1195G7 2.90GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK 9.0.100-rc.1.24452.12
  [Host]               : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  .NET 8.0             : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  .NET Framework 4.7.2 : .NET Framework 4.8.1 (4.8.9277.0), X64 RyuJIT VectorSize=256

| Method   | Job                  | Runtime              | N    | Mean       | Error    | StdDev   | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------- |--------------------- |--------------------- |----- |-----------:|---------:|---------:|------:|--------:|----------:|------------:|
| Original | .NET 8.0             | .NET 8.0             | 10   |   612.1 ns |  9.88 ns |  9.24 ns |  1.01 |    0.02 |         - |          NA |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 10   |   608.7 ns |  9.68 ns |  8.08 ns |  1.00 |    0.02 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 10   |   605.0 ns | 11.80 ns | 11.59 ns |  0.94 |    0.02 |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 10   |   643.1 ns | 11.88 ns | 10.53 ns |  1.00 |    0.02 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| Original | .NET 8.0             | .NET 8.0             | 30   |   649.8 ns | 11.00 ns | 10.28 ns |  0.91 |    0.02 |         - |          NA |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 30   |   713.3 ns | 13.79 ns | 14.76 ns |  1.00 |    0.03 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 30   |   682.8 ns | 10.50 ns |  9.31 ns |  0.93 |    0.02 |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 30   |   737.1 ns | 14.55 ns | 13.61 ns |  1.00 |    0.03 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| Original | .NET 8.0             | .NET 8.0             | 100  |   759.0 ns | 13.78 ns | 12.22 ns |  1.09 |    0.03 |         - |          NA |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 100  |   694.8 ns | 11.25 ns | 13.40 ns |  1.00 |    0.03 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 100  |   731.4 ns | 14.61 ns | 15.01 ns |  1.08 |    0.03 |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 100  |   679.6 ns | 13.56 ns | 15.61 ns |  1.00 |    0.03 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| Original | .NET 8.0             | .NET 8.0             | 300  |   743.1 ns | 14.55 ns | 14.29 ns |  0.90 |    0.02 |         - |          NA |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 300  |   830.3 ns | 14.75 ns | 12.31 ns |  1.00 |    0.02 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 300  |   764.9 ns | 14.71 ns | 12.28 ns |  0.90 |    0.04 |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 300  |   850.1 ns | 17.03 ns | 33.61 ns |  1.00 |    0.05 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| Original | .NET 8.0             | .NET 8.0             | 1000 | 1,045.4 ns | 19.85 ns | 18.56 ns |  1.04 |    0.03 |         - |          NA |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 1000 | 1,006.0 ns | 18.89 ns | 17.67 ns |  1.00 |    0.02 |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 1000 | 1,007.1 ns | 19.01 ns | 18.67 ns |  1.01 |    0.03 |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 1000 |   998.2 ns | 18.87 ns | 22.46 ns |  1.00 |    0.03 |         - |          NA |
timyhac commented 2 weeks ago

Or am I missing the point by removing the source buffer from the equation?

[SimpleJob(RuntimeMoniker.Net472, baseline: true)]
[SimpleJob(RuntimeMoniker.Net80)]
[MemoryDiagnoser]
public class Tests
{
    private int tag_id;

    [Params(10, 30, 100, 300, 1000)]
    public int N;

    [GlobalSetup]
    public void Setup()
    {
        tag_id = Native.plc_tag_create("protocol=ab-eip&gateway=%s&path=1,0&plc=controllogix&name=@raw", 1000);
        var result = Native.plc_tag_set_size(tag_id, N);
    }

    [Benchmark]
    public int Original()
    {
        var data = new byte[N];
        return plctag.Original(tag_id, 0, data, data.Length);
    }

    [Benchmark]
    public int BytePtr()
    {
        Span<byte> data = stackalloc byte[N];
        var result = plctag.BytePtr(tag_id, 0, data);
        // parse the buffer and take action...
        return result;
    }
}
// * Summary *

BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.4249/23H2/2023Update/SunValley3)
11th Gen Intel Core i7-1195G7 2.90GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK 9.0.100-rc.1.24452.12
  [Host]               : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  .NET 8.0             : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  .NET Framework 4.7.2 : .NET Framework 4.8.1 (4.8.9277.0), X64 RyuJIT VectorSize=256

| Method   | Job                  | Runtime              | N    | Mean       | Error    | StdDev   | Ratio | RatioSD | Gen0   | Allocated | Alloc Ratio |
|--------- |--------------------- |--------------------- |----- |-----------:|---------:|---------:|------:|--------:|-------:|----------:|------------:|
| Original | .NET 8.0             | .NET 8.0             | 10   |   613.6 ns |  5.55 ns |  4.92 ns |  0.96 |    0.03 | 0.0057 |      40 B |        1.00 |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 10   |   642.1 ns | 12.53 ns | 17.96 ns |  1.00 |    0.04 | 0.0057 |      40 B |        1.00 |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 10   |   597.3 ns |  5.42 ns |  4.80 ns |  0.92 |    0.03 |      - |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 10   |   650.0 ns | 12.72 ns | 20.18 ns |  1.00 |    0.04 |      - |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| Original | .NET 8.0             | .NET 8.0             | 30   |   685.5 ns | 12.84 ns | 10.72 ns |  0.91 |    0.02 | 0.0086 |      56 B |        1.00 |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 30   |   756.1 ns | 14.79 ns | 11.54 ns |  1.00 |    0.02 | 0.0086 |      56 B |        1.00 |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 30   |   688.8 ns | 10.55 ns |  9.36 ns |  0.94 |    0.02 |      - |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 30   |   736.5 ns | 10.11 ns |  9.45 ns |  1.00 |    0.02 |      - |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| Original | .NET 8.0             | .NET 8.0             | 100  |   791.1 ns | 11.21 ns | 10.49 ns |  1.01 |    0.02 | 0.0200 |     128 B |        1.00 |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 100  |   781.8 ns | 10.52 ns |  9.84 ns |  1.00 |    0.02 | 0.0200 |     128 B |        1.00 |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 100  |   749.2 ns |  9.96 ns |  9.31 ns |  1.02 |    0.02 |      - |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 100  |   732.1 ns | 14.30 ns | 13.37 ns |  1.00 |    0.02 |      - |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| Original | .NET 8.0             | .NET 8.0             | 300  |   796.0 ns | 10.60 ns |  8.85 ns |  0.99 |    0.02 | 0.0515 |     328 B |        1.00 |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 300  |   800.5 ns | 12.77 ns | 11.32 ns |  1.00 |    0.02 | 0.0515 |     329 B |        1.00 |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 300  |   721.4 ns |  8.80 ns |  7.35 ns |  0.84 |    0.02 |      - |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 300  |   860.8 ns | 16.65 ns | 15.57 ns |  1.00 |    0.02 |      - |         - |          NA |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| Original | .NET 8.0             | .NET 8.0             | 1000 | 1,209.0 ns | 23.00 ns | 21.51 ns |  1.02 |    0.02 | 0.1621 |    1024 B |        1.00 |
| Original | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 1000 | 1,187.1 ns | 18.75 ns | 17.53 ns |  1.00 |    0.02 | 0.1621 |    1027 B |        1.00 |
|          |                      |                      |      |            |          |          |       |         |        |           |             |
| BytePtr  | .NET 8.0             | .NET 8.0             | 1000 | 1,290.0 ns | 24.58 ns | 25.24 ns |  1.01 |    0.03 |      - |         - |          NA |
| BytePtr  | .NET Framework 4.7.2 | .NET Framework 4.7.2 | 1000 | 1,275.7 ns | 25.44 ns | 24.99 ns |  1.00 |    0.03 |      - |         - |          NA |
MitchRazga commented 2 weeks ago

Thanks @timyhac! I was initially just sharing some rough thoughts in that PR but great to see that you are open to this!

Your benchmark matches my expectations, since the allocations are not within the P/Invoke. They are from the application side where the caller needs to conform to the method signatures in the Tag API. https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag/Tag.cs#L933 https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag/Tag.cs#L938 https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag/Tag.cs#L915 https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag/Tag.cs#L926 For example, with a stack-allocated buffer Span<byte> or a pooled buffer Memory<byte> the caller would need to .ToArray() to get byte[] which creates a copy.

Since the libplctag core library doesn't appear to keep a reference to the source buffer and copies into the internal buffer it can be short-lived.

The primary motivation would be to provide the flexibility of using either heap-allocated, stack-allocated, pooled, or sliced buffers but it could also simplify the API if you wanted since byte[] has an implicit conversion to (ReadOnly)Span<byte> so this:

public void SetBuffer(byte[] buffer)
public void SetBuffer(int start_offset, byte[] buffer, int length)

could be replaced with:

public void SetBuffer(ReadOnlySpan<byte> buffer)

It would then be the callers responsibility to slice if required.

For example:

byte[] data1 = new byte[10];
SetBuffer(data1);

Span<byte> data2 = stackalloc byte[10];
SetBuffer(data2);

Memory<byte> data3 = new byte[500];
Memory<byte> slicedData3 = data3[..10];
SetBuffer(slicedData3.Span);

As a side note, on the application side, unsafe methods such as MemoryMarshal.AsBytes)) allow for a "buffer-free" way to cast a value to (ReadOnly)Span<byte>. So if Get/SetBuffer supports ReadOnlySpan<byte> these could be could potentially be used as a partial replacement for the depreciated mapper system (if precautions are taken, endianness matches etc.).

int value = 123456;
ReadOnlySpan<byte> bytes = MemoryMarshal.AsBytes(new ReadOnlySpan<int>(in value));
timyhac commented 2 weeks ago

This sounds good and I agree with the reasoning.

I think we still need to have at least these byte[]-based overloads so we don't ask consumers to take a dependency on System.Memory, although I imagine that the majority of people using Span would be working with recent versions of .NET that don't require that package.

public void GetBuffer(int start_offset, byte[] buffer, int length);
public void SetBuffer(int start_offset, byte[] buffer, int length);

Something that I'm unsure about is how to expose these overloads in libplctag.NativeImport. My understanding is that it is a C# taboo to have unsafe methods that are public. Having said that, the NativeImport package is for interop so it could be the exception to the rule..

timyhac commented 2 weeks ago

Although we could just expose a Span based overload in NativeImport layer also as you suggest.

On Sun, 20 Oct 2024, 10:14 pm Mitch Razga, @.***> wrote:

Thanks @timyhac https://github.com/timyhac! I was initially just sharing some rough thoughts in that PR but great to see that you are open to this!

Your benchmark matches my expectations, since the allocations are not within the P/Invoke. They are from the application side where the caller needs to conform to the method signatures in the Tag API.

https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag/Tag.cs#L933

https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag/Tag.cs#L938

https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag/Tag.cs#L915

https://github.com/libplctag/libplctag.NET/blob/e28ec36addeee8eae304607b49be52dccab1fcea/src/libplctag/Tag.cs#L926 For example, with a stack-allocated buffer Span or a pooled buffer https://learn.microsoft.com/en-us/dotnet/api/system.buffers.memorypool-1.rent?view=net-8.0#system-buffers-memorypool-1-rent(system-int32) Memory the caller would need to .ToArray() to get byte[] which creates a copy.

Since the libplctag core library doesn't appear to keep a reference to the source buffer and copies into the internal buffer https://github.com/libplctag/libplctag/blob/c55bc5876d938dda1c609750cde5ae4812d7b8a8/src/lib/lib.c#L3956-L3959 it can be short-lived.

The primary motivation would be to provide the flexibility of using either heap-allocated, stack-allocated, pooled, or sliced buffers but it could also simplify the API if you wanted since byte[] has an implicit conversion to (ReadOnly)Span so this:

public void SetBuffer(byte[] buffer)public void SetBuffer(int start_offset, byte[] buffer, int length)

could be replaced with:

public void SetBuffer(ReadOnlySpan buffer)

It would then be the callers responsibility to slice https://learn.microsoft.com/en-us/dotnet/api/system.span-1.slice?view=net-8.0#system-span-1-slice(system-int32-system-int32) if required.

For example:

byte[] data1 = new byte[10]; SetBuffer(data1); Span data2 = stackalloc byte[10]; SetBuffer(data2); Memory data3 = new byte[500];Memory slicedData3 = data3[..10]; SetBuffer(slicedData3.Span);

As a side note, on the application side, unsafe methods such as MemoryMarshal.AsBytes https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.memorymarshal.asbytes?view=net-8.0#system-runtime-interopservices-memorymarshal-asbytes-1(system-readonlyspan((-0))) allow for a "buffer-free" way to cast a value to (ReadOnly)Span. So if Get/SetBuffer supports ReadOnlySpan these could be could potentially be used as a partial replacement for the depreciated mapper system (if precautions are taken and endianness matches).

int value = 123456;ReadOnlySpan bytes = MemoryMarshal.AsBytes(new ReadOnlySpan(in value));

— Reply to this email directly, view it on GitHub https://github.com/libplctag/libplctag.NET/issues/423#issuecomment-2424858641, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESB445VLP65HAOD5IQR7H3Z4OGBNAVCNFSM6AAAAABQGS2PC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRUHA2TQNRUGE . You are receiving this because you were mentioned.Message ID: @.***>

MitchRazga commented 2 weeks ago

Something that I'm unsure about is how to expose these overloads in libplctag.NativeImport. My understanding is that it is a C# taboo to have unsafe methods that are public. Having said that, the NativeImport package is for interop so it could be the exception to the rule..

Yeah this case appears to be an exception to the guidelines

❌ DO NOT have publicly exposed methods that take pointers, arrays of pointers, or multidimensional arrays as parameters.

Pointer Parameters

In general, pointers should not appear in the public surface area of a well-designed managed code framework. Most of the time, pointers should be encapsulated. However, in some cases pointers are required for interoperability reasons, and using pointers in such cases is appropriate.

✔️ DO provide an alternative for any member that takes a pointer argument, because pointers are not CLS-compliant.

❌ AVOID doing expensive argument checking of pointer arguments.

✔️ DO follow common pointer-related conventions when designing members with pointers.

For example, there is no need to pass the start index, because simple pointer arithmetic can be used to accomplish the same result.

However, an alternative design could be to do something like:

public void GetBuffer(ReadOnlySpan<byte> buffer)
{
    ThrowIfAlreadyDisposed();
    ref byte bufferReference = ref MemoryMarshal.GetReference(buffer);
    var result = (Status)_native.plc_tag_get_raw_bytes(nativeTagHandle, 0, ref bufferReference, buffer.Length);
    ThrowIfStatusNotOk(result);
}
int plc_tag_get_raw_bytes(int tag, int start_offset, ref byte buffer, int buffer_length);

Not sure of the optimal approach. I have a busy week so I'll work on it when I get a chance.

MitchRazga commented 1 week ago

Haven't forgotten about this, just been extremely busy sorry.

You raised a really good point about how consumers on older frameworks would have to take a dependency on System.Memory. A preprocessor directive like below should work since anything above .NET Standard 2.1 and .NET Core 2.1 (and .NET 5+ too) would have System.Memory within their metapackage.

#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER

Thinking ahead, there could be requests for Span<char> or Span<byte> overloads for the string methods at some point, so this approach might get messy over time. Also, to fully support Native AOT and trimming the NativeImport layer will need P/Invoke source generation.

...this IL stub is generated at run time, it isn't available for ahead-of-time (AOT) compiler or IL trimming scenarios.

Maybe it is worth splitting the library for modern and legacy .NET targeting? .NET BCL does something like this: .netcoreapp.cs csproj

On a side note, I noticed that between 1.5.2 and 1.6.0-alpha.0 that the nuget package now targets specific frameworks instead of .NET Standard 2.0 which means that .NET Framework 4.6.1, 4.6.2, .NET Core 2.0-2.2 etc. usage is removed. Not sure if this was intentional but if thought I'd mention that .NET 6 end of life is November 12, 2024 so after that point anything below .NET 8 will be unsupported.

I think this is going to be a bit of a balancing act between retaining legacy compatibility while also supporting the modern features.

Let me know your thoughts. I'm happy to work on this, but I don’t want to step on your toes or push the library in a direction you don't want.

timyhac commented 1 week ago

Yes it would have been ideal to only need to target NetStandard2.0 but I wasn't able to get the native library copying working the correct way for both NETFramework and NETCore/net5.0+ unless multitargeting is used. I did mention "some systems" would have support dropped in the release notes although I don't recall those targets would be dropped - thanks for finding this.

Your suggestion to have two separate libraries does seem to be what Microsoft recommends. I did initially attempt to do it this way but realized that I would need to ship at least 2 additional packages - and the codebase would still be just as complex (I think?).

I think its still worth even hacking up a branch that only supports NET8 - this should be enough to benchmark. If the gains are there then I'm happy to find a way to integrate it and support it over the long term.