flatbuffers: ByteBufferAllocator shouldn't have a pointer property, but instead use Memory [C#, all OS, master]

Looking at the C# support for directly reading and writting to memory other than byte[]. For example, ByteBuffer can be initialized with a custom allocator which uses shared memory / memory mapped files, it has an issue with using pointers.

https://github.com/google/flatbuffers/blob/e1defaae5ea4e18eedde4c51ea82c05b802dc153/net/FlatBuffers/ByteBuffer.cs#L51-L58

https://github.com/google/flatbuffers/blob/e1defaae5ea4e18eedde4c51ea82c05b802dc153/net/FlatBuffers/ByteBuffer.cs#L137-L149

Looking at the documentation for AddrOfPinnedObject, it says:

This method is used to get a stable pointer to the object. Pinning an object prevents the garbage collector from moving it around in memory, thereby reducing the efficiency of the garbage collector.

The way this is working, it will pin the byte[] of the ByteArrayAllocator for the entire lifetime of the object. Which means the garbage collector can’t move this memory around, as it sees fit.


Proposal

Instead of exposing a byte* pointer directly from ByteBufferAllocator, it would be better to expose a Memory<byte> property instead. Callers can get a byte* out of a Memory<byte> with code like the following:

Memory<byte> buffer = ...;
fixed (byte* ptr = buffer.Span)
{
    // use `ptr` as usual
}

The fixed keyword will pin any managed objects (like a byte[]) only for the amount of time the fixed statement is in scope. Once it goes out of scope, the managed object will be unpinned, and can be moved by the GC freely.

Another advantage of using a Memory<byte> instead, is that ByteBuffer can have a ToMemory method, like it has ToSpan and ToArraySegment methods. In certain situations it is necessary to get a Memory<T> instead of a Span<T>. For example, if you are using async code, you cannot use a Span<T>. A concrete example of this scenario is in the Apache Arrow C# library, where it is attempting to write to a Stream object. In order to modify this code so it can handle reading and writing to memory other than byte[], it will need a ToMemory method on ByteBuffer.

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Comments: 18 (15 by maintainers)

Commits related to this issue

Most upvoted comments

I was able to incorporate your UseMemoryPlayground version into our actual application and give it a try. My tests involved using our custom allocators with the new Span / Memory based API and profiling the “inner loop” creation of a flat buffer. The tests were against .NET Framework, which is likely the slower than .NET Core.

The tests showed that this new approach is just slightly slower than our current builds. Times went from about 35ms to about 40ms for the build operation.

I think this is an acceptable difference in performance for the gains in API improvements. I say we should move forward with this new approach.

I was not the one who added the original UNSAFE_BYTE_BUFFER configuration so there may be a use case out there which needs the extra perf?

@eerhardt that all sounds fine to me.