Use ArrayPool for poolable byte buffers #392
Conversation
| } | ||
|
|
||
| ArrayPool<byte>.Shared.Return(buffer); |
There was a problem hiding this comment.
In any case where there's the possibility of throwing (e.g. calling an underlying stream we don't control), we should consider wrapping that logic try + finally.
try
{
// logic that may throw
}
finally
{
ArrayPool<byte>.Shared.Return(buffer);
}To ensure we don't leak a buffer and degrade the performance of the ArrayPool for other callers.
This should be unnecessary when we're confident we won't throw.
There was a problem hiding this comment.
I added try/finally in a few places but not in others. Leaking a buffer isn't the end of the world. There can be a perf hit for having try/catch, even when nothing actually throws at run-time.
| var buffer = new byte[257]; | ||
| var buffer = ArrayPool<byte>.Shared.Rent(257); | ||
|
|
||
| while (true) | ||
| { | ||
| var b = reader.GetByte(); | ||
| if (b == 0) | ||
| return bytes.ToArray(); | ||
| buffer[0] = b; | ||
| reader.GetBytes(buffer, 1, b); | ||
| bytes.Write(buffer, 0, b + 1); | ||
| var len = reader.GetByte(); | ||
| if (len == 0) | ||
| break; | ||
| buffer[0] = len; | ||
| reader.GetBytes(buffer, offset: 1, count: len); | ||
| bytes.Write(buffer, 0, len + 1); | ||
| } | ||
|
|
||
| ArrayPool<byte>.Shared.Return(buffer); |
There was a problem hiding this comment.
Could we also kill thevar bytes = new MemoryStream(); allocation, and write directly into the buffer (keeping track of the written bytes)?
There was a problem hiding this comment.
The issue is we don't know how large the stream will be in the end. Each iteration of the loop will add another chunk of bytes, and the result needs to be trimmed to size (with the current API). It's a pretty bizarre encoding format (XMP in GIF — see https://dl.photoprism.app/pdf/20120101-Adobe_XMP_Specification_Part_3.pdf section 1.1.2 on page 11).
There was a problem hiding this comment.
I pushed some more improvements in GifReader.
|
@iamcarbon I'm going to merge but if you have any further thoughts, I'll file a follow up PR. |
No description provided.