Data marshaling wrapper for a TCP server

Question

After a lot of research, I created this wrapper which I think fits very well to my needs. My project is a TCP server and I will be using this wrapper every time the server receives a packet. Basically, the wrapper will work marshaling raw byte[] into higher-level representation of the data as marshalable structs.

What do you think about this wrapper? Any hint on how I can improve it? I'm very concerned about performance.

public static class MyMarshal
{
    /// <summary>
    /// Marshals a raw buffer to a given marshalable struct.
    /// </summary>
    public static unsafe T GetStructure<T>(byte[] buffer) where T : struct
    {
        fixed (byte* bufferPin = buffer)
        {
            return (T)Marshal.PtrToStructure(new IntPtr(bufferPin), typeof(T));
        }
    }

    /// <summary>
    /// Marshals a given T instance into a raw buffer.
    /// </summary>
    public static unsafe byte[] GetBytes<T>(T obj) where T : struct
    {
        byte[] rawBuffer = new byte[Marshal.SizeOf(obj)];

        fixed (byte* rawBufferPin = rawBuffer)
        {
            Marshal.StructureToPtr<T>(obj, new IntPtr(rawBufferPin), false);
        }

        return rawBuffer;
    }

    /// <summary>
    /// Crates a zero-initialized marshaled instance of T
    /// </summary>
    public static unsafe T CrateEmpty<T>() where T : struct
    {
        int typeSize = Marshal.SizeOf(typeof(T));

        byte* rawBuffer = stackalloc byte[typeSize];

        for (int i = 0; i < typeSize; i++)
            rawBuffer[i] = 0;

        T zeroInited = (T)Marshal.PtrToStructure(new IntPtr(rawBuffer), typeof(T));

        return zeroInited;
    }
}

Summing up, the wrapper have 3 methods:

• GetStructure (used right after I receive any valid packet to get the respective structure)
• GetBytes (used when I want to send a packet to a client, converts a given struct value to a raw buffer to be send to the client)
• CreateEmpty (used to create a zero-initialized instance of a given struct. This method is used when I have complex structures with non-blittable arrays, etc. The goal of this method is to have a generic zero-initializer method even to non-blittable structs - those with non-blittable arrays and/or strings).

I want to raise another questions too:

1. What do you think about this mixed use of unsafe code and the Marshal class? Is there any problem on using that way?
2. In the method MyMarshal.CreateEmpty, what do you think about the stackalloc usage? Some structures that will be initialized with this method will be a little big (usually something between 100 and 1024 bytes). Is there any downside or danger of allocating this length in the stack several times per seconds? I'm expecting to process something around 5k packet per second.

Also, he TCP server will work asynchronously using the async/await pattern, so I don't expect to deal with shared stated and other common problems to a multi-threaded application.

Answer 1

The wrapper seems pretty good to me. The only change you might consider is that CreateWrapper really only needs to run once for each type; since these are structs it can return copies of the same empty object each time. Something like this should work:

static class EmptyHolder<T> where T : struct
{
    public static T EmptyInstance;
    // assign EmptyInstance in static constructor
}

and then:

public static unsafe T CreateEmpty<T>() where T : struct
{
    return EmptyHolder<T>.EmptyInstance;
}

Of course there's a tradeoff of an uncollectable memory for each type you call this with. And with anything performance-related, you'll need to measure to see if it delivers any benefit. The original code should be pretty fast so maybe it's not significant.

There shouldn't be any danger in no matter how often you call stackalloc, by definition it's not possible for any two such calls to affect the same stack at the same time. And there's no danger of any sort of recursion that would have more than 1 of these allocations on the same stack since the block is freed at the end of this function.

Answer 2

There simply aren't enough comments in your code for the average C# developer to grok what's happening quickly. I don't meant to slight a large group of people, but let's face facts here, we live our lives in the wonderful world where memory is managed for us, and pointers are never thought of, excepts in the abstract "classes are reference types" kind of way. Be nice to the Jr. dev that has to read this code 5 years from now. Leave some brief comments about what this code is doing and why you've chosen to use the fixed keyword and raw pointers.

Other than that, it appears to be a sane implementation to me. For what it's worth, it looks very much like an implementation of my own. Personally, I chose to use Marshal for everything so that I didn't I have to compile with the unsafe flag, or declare uses of the methods as unsafe. I can't say if it would perform any better or worse than your implementation in terms of memory or speed. If a safe implementation was just as good, I would prefer it so that the code doesn't leak the the unmanaged memory details up through the abstraction.

---

I posted my own implementation for review. It does not require the use of the unsafe keyword and context.