zeromq / netmq

A 100% native C# implementation of ZeroMQ for .NET
Other
2.95k stars 744 forks source link

Msg using InitPool still allocates for AtomicCounter #858

Closed ronaldvdv closed 4 years ago

ronaldvdv commented 4 years ago

Environment

NetMQ Version:  4.0.0.207
Operating System: Windows
.NET Version:  .NET Core 3.1

Expected behaviour

I've implemented a small IBufferPool implementation that uses an ArrayPool<byte> for sharing byte buffers between Msg instances. My goal is to be able to send frames without allocation where possible.

Actual behaviour

When profiling I realized the Msg.InitPool() call is still allocating, because it instantiates an AtomicCounter.

Steps to reproduce the behaviour

Add the following class:

    public class ArrayPoolBufferPool : IBufferPool
    {
        private readonly ArrayPool<byte> _pool = ArrayPool<byte>.Shared;

        public void Dispose()
        {

        }

        public void Return(byte[] buffer)
        {
            _pool.Return(buffer);
        }

        public byte[] Take(int size)
        {
            return _pool.Rent(size);
        }
    }

Then initialize it as follows.

var bufferPool = new ArrayPoolBufferPool(); BufferPool.SetCustomBufferPool(bufferPool);

Finally, create a socket, use socket.SendFrame(...data...) and run a profiler to measure allocations. It will show that Msg.InitPool() still allocates 20 bytes for the AtomicCounter.

ronaldvdv commented 4 years ago

My simple thought would be

somdoron commented 4 years ago

Sounds good to me, we can add that to the interface, or add a new interface which also allows creating the AtomicCounter. The default implementation can be to just use the GC.

ronaldvdv commented 4 years ago

Cool! Not sure which option is best...

Do you have a strong preference? And should I give it a try and create a pull request?

somdoron commented 4 years ago

Let's try to change the existing interface together with c# 8.0 default implementation for the new methods, which will just allocate from the GC.

ronaldvdv commented 4 years ago

I've forked and created a branch, but I noticed since NetMQ also targets .NET Standard 2.0, we cannot use default interface implementations (see https://github.com/dotnet/docs/issues/13656). So for now I'll go with the option of adding another interface (so we don't break existing code either), but I'm happy to change that if you feel otherwise.

dxdjgl commented 4 years ago

Due to the new release can this issue be closed?