(ReadOnly)Span<byte> overloads on GetBuffer to reduce allocations and make using existing buffers easier
#423
Comments
|
@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. 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: |
|
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;
}
} |
|
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. libplctag.NET/src/libplctag/Tag.cs Line 933 in e28ec36 libplctag.NET/src/libplctag/Tag.cs Line 938 in e28ec36 libplctag.NET/src/libplctag/Tag.cs Line 915 in e28ec36 libplctag.NET/src/libplctag/Tag.cs Line 926 in e28ec36 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 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 int value = 123456;
ReadOnlySpan<byte> bytes = MemoryMarshal.AsBytes(new ReadOnlySpan<int>(in value)); |
|
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 Something that I'm unsure about is how to expose these overloads in |
|
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<byte> 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<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
<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<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
<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<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
<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<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 and endianness matches)*.
int value = 123456;ReadOnlySpan<byte> bytes = MemoryMarshal.AsBytes(new ReadOnlySpan<int>(in value));
—
Reply to this email directly, view it on GitHub
<#423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AESB445VLP65HAOD5IQR7H3Z4OGBNAVCNFSM6AAAAABQGS2PC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRUHA2TQNRUGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yeah this case appears to be an exception to the guidelines
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. |
|
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 #if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATERThinking ahead, there could be requests for
Maybe it is worth splitting the library for modern and legacy .NET targeting? .NET BCL does something like this: 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. 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. |
|
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. |
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.libplctag.NET/src/libplctag.NativeImport/NativeMethods.cs
Lines 247 to 251 in e28ec36
https://github.com/libplctag/libplctag/blob/c55bc5876d938dda1c609750cde5ae4812d7b8a8/src/lib/libplctag.h#L505-L506
There are a few ways to do this, one approach can be found in Memory<T> and Span<T> usage guidelines under:
Alternatively and ideally, would use P/Invoke source generation but this requires .NET 7+. Something like:
This would also make it possible to create
ReadAsyncandWriteAsyncwithMemory<byte>overloads too. Similar to Stream.ReadAsync and Stream.WriteAsyncOriginally posted by @MitchRazga in #394 (comment)
The text was updated successfully, but these errors were encountered: