FirelyTeam / firely-net-sdk

The official Firely .NET SDK for HL7 FHIR
Other
816 stars 340 forks source link

Improve memory allocation during HttpClient usage #2769

Closed kfrancis closed 3 months ago

kfrancis commented 3 months ago

I was recently doing some improvement on my own HttpClient usage in libraries of our design after reading through https://xaml.dev/post/removing-memory-allocations-in-http-requests-using-arraypools, and someone on our team asked if we could improve allocation when utilizing this library. We are using this library to communicate with various FHIR endpoints, but we're finding there's a lot of allocation that seems that it could be optimized.

It does look possible when looking at HttpClientRequester

So, I've created a netstandard2.0 compatible version of LimitArrayPoolWriteStream that we use for this purpose (see article, we adapted this for use in netstandard2 which might be applicable here).

internal sealed class LimitArrayPoolWriteStream : Stream
{
    private const int InitialLength = 256;

    private readonly int _maxBufferSize;
    private byte[] _buffer;
    private int _length;

    public LimitArrayPoolWriteStream(int maxBufferSize) : this(maxBufferSize, InitialLength) { }

    public LimitArrayPoolWriteStream(int maxBufferSize, long capacity)
    {
        if (capacity < InitialLength)
        {
            capacity = InitialLength;
        }
        else if (capacity > maxBufferSize)
        {
            throw CreateOverCapacityException(maxBufferSize);
        }

        _maxBufferSize = maxBufferSize;
        _buffer = ArrayPool<byte>.Shared.Rent((int)capacity);
    }

    protected override void Dispose(bool disposing)
    {
        Debug.Assert(_buffer != null);

        ArrayPool<byte>.Shared.Return(_buffer);
        _buffer = null!;

        base.Dispose(disposing);
    }

    public ArraySegment<byte> GetBuffer() => new(_buffer, 0, _length);

    public byte[] ToArray()
    {
        var arr = new byte[_length];
        Buffer.BlockCopy(_buffer, 0, arr, 0, _length);
        return arr;
    }

    private void EnsureCapacity(int value)
    {
        if ((uint)value > (uint)_maxBufferSize) // value cast handles overflow to negative as well
        {
            throw CreateOverCapacityException(_maxBufferSize);
        }
        else if (value > _buffer.Length)
        {
            Grow(value);
        }
    }

    private void Grow(int value)
    {
        Debug.Assert(value > _buffer.Length);

        // Extract the current buffer to be replaced.
        var currentBuffer = _buffer;
        _buffer = null!;

        // Determine the capacity to request for the new buffer.  It should be
        // at least twice as long as the current one, if not more if the requested
        // value is more than that.  If the new value would put it longer than the max
        // allowed byte array, than shrink to that (and if the required length is actually
        // longer than that, we'll let the runtime throw).
        var twiceLength = 2 * (uint)currentBuffer.Length;
        var newCapacity = twiceLength > int.MaxValue ?
            Math.Max(value, int.MaxValue) :
            Math.Max(value, (int)twiceLength);

        // Get a new buffer, copy the current one to it, return the current one, and
        // set the new buffer as current.
        var newBuffer = ArrayPool<byte>.Shared.Rent(newCapacity);
        Buffer.BlockCopy(currentBuffer, 0, newBuffer, 0, _length);
        ArrayPool<byte>.Shared.Return(currentBuffer);
        _buffer = newBuffer;
    }

    public override void Write(byte[] buffer, int offset, int count)
    {
        Debug.Assert(buffer != null);
        Debug.Assert(offset >= 0);
        Debug.Assert(count >= 0);

        EnsureCapacity(_length + count);
        Buffer.BlockCopy(buffer, offset, _buffer, _length, count);
        _length += count;
    }

    public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
    {
        Write(buffer, offset, count);
        return Task.CompletedTask;
    }

    public override void WriteByte(byte value)
    {
        var newLength = _length + 1;
        EnsureCapacity(newLength);
        _buffer[_length] = value;
        _length = newLength;
    }

    public override void Flush() { }
    public override Task FlushAsync(CancellationToken cancellationToken) => Task.CompletedTask;

    public override long Length => _length;
    public override bool CanWrite => true;
    public override bool CanRead => false;
    public override bool CanSeek => false;

    public override long Position { get { throw new NotSupportedException(); } set { throw new NotSupportedException(); } }
    public override int Read(byte[] buffer, int offset, int count) { throw new NotSupportedException(); }
    public override long Seek(long offset, SeekOrigin origin) { throw new NotSupportedException(); }
    public override void SetLength(long value) { throw new NotSupportedException(); }

    private static InvalidOperationException CreateOverCapacityException(int maxBufferSize)
    {
        return new InvalidOperationException($"Buffer size exceeded maximum of {maxBufferSize} bytes.");
    }
}

And we use it in the following way (deserializing soap content):

using var requestMessage = new HttpRequestMessage(HttpMethod.Post, uri)
{
    Content = httpContent
};
using var message = await client.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead, ct).ConfigureAwait(false);
using var data = new LimitArrayPoolWriteStream(int.MaxValue, message.Content.Headers.ContentLength ?? 256);
await message.Content.CopyToAsync(data).ConfigureAwait(false);
var bufferSegment = data.GetBuffer();

// Convert the byte buffer to a string which we can then deserialize into XML (or whatever)
var resultContent = Encoding.UTF8.GetString(bufferSegment.Array, bufferSegment.Offset, bufferSegment.Count);

In our case, the servers don't support sending back the Content-Length header - but that might be different in this case (and more efficient) when supported.

Kasdejong commented 3 months ago

We tried this, and we will keep it in mind for later, but for now there is hardly any benefit in this. Our deserializers only take string input, and consume an order of magnitude more memory than the overhead of the HTTP client. In other words: the potential gain from this is very limited. On top of this, we are forced to cast everything to strings anyways as to not break our API.

Once again, this may become beneficial in the future, but would require a major overhaul to be noticeable.