Optimize large buffer allocations with ArrayPool#78
Conversation
Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
PolyShim/Net100/Random.cs
Outdated
| { | ||
| var bytes = new byte[(stringLength + 1) / 2]; | ||
| random.NextBytes(bytes); | ||
| var byteCount = (stringLength + 1) / 2; |
PolyShim/Net50/Convert.cs
Outdated
| public static string ToHexString(byte[] value, int startIndex, int length) | ||
| { | ||
| var c = new char[length * 2]; | ||
| var c = ArrayPool<char>.Shared.Rent(length * 2); |
| var buffer = new char[4096]; | ||
|
|
||
| while (true) | ||
| var buffer = ArrayPool<char>.Shared.Rent(4096); |
There was a problem hiding this comment.
Maybe can use MemoryPool here, no? And in other places where async methods that support Memory are used.
There was a problem hiding this comment.
Done in a132bff - switched to MemoryPool for cleaner disposal pattern. Note: StringBuilder doesn't support Span/Memory append in older frameworks, so we convert the Memory slice to string (which allocates, but is necessary for compatibility).
| { | ||
| var bytes = new byte[(stringLength + 1) / 2]; | ||
| RandomNumberGenerator.Fill(bytes); | ||
| var byteCount = (stringLength + 1) / 2; |
There was a problem hiding this comment.
Done in a132bff - inlined byteCount calculation
PolyShim/NetCore20/File.cs
Outdated
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| int charsRead; | ||
| while ((charsRead = await reader.ReadAsync(buffer, 0, 4096).ConfigureAwait(false)) > 0) |
There was a problem hiding this comment.
Can read into entire buffer, it's okay if there's more bytes
There was a problem hiding this comment.
Done in a132bff - changed to read into full buffer (buffer.Length) instead of limiting to bufferSize
PolyShim/NetCore20/File.cs
Outdated
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| int charsRead; | ||
| while ((charsRead = await reader.ReadAsync(buffer, 0, 4096).ConfigureAwait(false)) > 0) |
There was a problem hiding this comment.
Same here, it's okay if we read more bytes. Apply this logic in other relevant places
There was a problem hiding this comment.
Done in a132bff - applied the same change to both ReadAllTextAsync overloads
Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
…ool, read full buffer Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
PolyShim/Net70/TextReader.cs
Outdated
| { | ||
| var result = new StringBuilder(); | ||
| var buffer = new char[4096]; | ||
| using var memoryOwner = MemoryPool<char>.Shared.Rent(4096); |
There was a problem hiding this comment.
Rename to var buffer (here and in other places)
There was a problem hiding this comment.
Done in d57680f - renamed memoryOwner to buffer
PolyShim/Net70/TextReader.cs
Outdated
| // Convert Memory slice to string for StringBuilder (allocates but necessary for older frameworks) | ||
| result.Append(memoryOwner.Memory.Slice(0, charsRead).ToString()); |
There was a problem hiding this comment.
Just make a loop and append one char at a time
There was a problem hiding this comment.
Done in d57680f - changed to append char-by-char in a loop to avoid string allocation
| var bytes = ArrayPool<byte>.Shared.Rent((stringLength + 1) / 2); | ||
| try | ||
| { | ||
| RandomNumberGenerator.Fill(bytes.AsSpan(0, (stringLength + 1) / 2)); |
There was a problem hiding this comment.
Why slice here if we're slicing later anyway?
There was a problem hiding this comment.
Done in d57680f - removed the slice in Fill, now fills the whole buffer since we substring the final result anyway
| var hex = lowercase ? Convert.ToHexStringLower(bytes) : Convert.ToHexString(bytes); | ||
| return hex.Substring(0, stringLength); | ||
| var hex = lowercase | ||
| ? Convert.ToHexStringLower(bytes, 0, (stringLength + 1) / 2) |
There was a problem hiding this comment.
Why slice here if we're slicing the final string anyway?
There was a problem hiding this comment.
Done in d57680f - removed slicing in Convert.ToHexString since we substring the final result
PolyShim/NetCore10/Stream.cs
Outdated
| break; | ||
| while (true) | ||
| { | ||
| var bytesRead = source.Read(buffer, 0, bufferSize); |
There was a problem hiding this comment.
Done in d57680f - changed to read up to buffer.Length
|
|
||
| var content = new StringBuilder(); | ||
| var buffer = new char[4096]; | ||
| var buffer = ArrayPool<char>.Shared.Rent(bufferSize); |
There was a problem hiding this comment.
Done in d57680f - switched to MemoryPool for consistency with other async Memory-based methods, using char-by-char append
|
|
||
| var content = new StringBuilder(); | ||
| var buffer = new char[4096]; | ||
| var buffer = ArrayPool<char>.Shared.Rent(bufferSize); |
There was a problem hiding this comment.
Done in d57680f - switched to MemoryPool for consistency with other async Memory-based methods, using char-by-char append
…cing, use MemoryPool in File Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
PolyShim/NetCore20/File.cs
Outdated
| content.Append(buffer, 0, charsRead); | ||
|
|
||
| // Append char by char to avoid string allocation | ||
| var span = buffer.Memory.Span; |
There was a problem hiding this comment.
Inline this (and apply to other places where this pattern is used)
There was a problem hiding this comment.
Done in 65c8a88 - inlined buffer.Memory.Span access directly in the for loop, applied to all 3 locations (File.cs ReadAllTextAsync both overloads, and TextReader.cs ReadToEndAsync)
Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce temporary buffer allocations in PolyShim polyfills by switching several hot paths to pooled buffers (ArrayPool<T> / MemoryPool<T>), lowering GC pressure in stream, text, and hex-related helpers.
Changes:
- Use
ArrayPool<byte>in the .NET 3.5Stream.CopyTopolyfill. - Use
MemoryPool<char>inFile.ReadAllTextAsync(NetCore20) andTextReader.ReadToEndAsync(Net70) loops. - Use
ArrayPool<char>/ArrayPool<byte>in hex conversion and RNG hex string helpers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| PolyShim/NetCore20/File.cs | Switch ReadAllTextAsync buffering to pooled Memory<char> and per-char StringBuilder appends. |
| PolyShim/NetCore10/Stream.cs | Switch CopyTo buffer to ArrayPool<byte>. |
| PolyShim/Net80/RandomNumberGenerator.cs | Switch GetHexString byte buffer to ArrayPool<byte>. |
| PolyShim/Net70/TextReader.cs | Switch ReadToEndAsync(CancellationToken) buffer to MemoryPool<char> and per-char appends. |
| PolyShim/Net50/Convert.cs | Switch ToHexString char buffer to ArrayPool<char> and construct string from the used slice. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PolyShim/NetCore20/File.cs
Outdated
| using var buffer = MemoryPool<char>.Shared.Rent(bufferSize); | ||
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| int charsRead; | ||
| while ((charsRead = await reader.ReadAsync(buffer, 0, buffer.Length).ConfigureAwait(false)) > 0) | ||
| while ((charsRead = await reader.ReadAsync(buffer.Memory, cancellationToken).ConfigureAwait(false)) > 0) | ||
| { |
There was a problem hiding this comment.
In this NetCore20 polyfill, calling StreamReader.ReadAsync(Memory, CancellationToken) will resolve to PolyShim’s TextReader.ReadAsync(Memory, ...) polyfill on NETSTANDARD<2.1/NETCOREAPP<2.1, which allocates a new char[] via buffer.ToArray() on every loop iteration (see PolyShim/NetCore21/TextReader.cs). That defeats the goal of reducing allocations; consider using ArrayPool (or a rented char[] via MemoryMarshal.TryGetArray) and the array-based ReadAsync(char[], int, int) overload instead.
PolyShim/NetCore20/File.cs
Outdated
| // Append char by char to avoid string allocation | ||
| for (var i = 0; i < charsRead; i++) | ||
| { | ||
| content.Append(buffer.Memory.Span[i]); | ||
| } |
There was a problem hiding this comment.
The per-character append loop is a significant slowdown and the comment is misleading: StringBuilder.Append(char[], int, int) does not allocate intermediate strings. If you switch back to an array-backed buffer, you can append the chunk in one call (or, on newer TFMs, use StringBuilder.Append(ReadOnlySpan) with a slice) to keep performance comparable to the BCL implementation.
PolyShim/NetCore20/File.cs
Outdated
| using var buffer = MemoryPool<char>.Shared.Rent(bufferSize); | ||
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| int charsRead; | ||
| while ((charsRead = await reader.ReadAsync(buffer, 0, buffer.Length).ConfigureAwait(false)) > 0) | ||
| while ((charsRead = await reader.ReadAsync(buffer.Memory, cancellationToken).ConfigureAwait(false)) > 0) | ||
| { |
There was a problem hiding this comment.
Same issue in the encoding overload: StreamReader.ReadAsync(Memory, CancellationToken) on older TFMs routes through the PolyShim TextReader.ReadAsync(Memory, ...) polyfill, which allocates via buffer.ToArray() each call. To actually reduce allocations, prefer a pooled char[] + ReadAsync(char[], int, int) here.
PolyShim/NetCore20/File.cs
Outdated
| // Append char by char to avoid string allocation | ||
| for (var i = 0; i < charsRead; i++) | ||
| { | ||
| content.Append(buffer.Memory.Span[i]); | ||
| } |
There was a problem hiding this comment.
The per-character append loop is likely to be much slower than appending the chunk directly (and Append(char[], int, int) doesn’t allocate a string). Consider using chunk append rather than appending one char at a time.
PolyShim/Net70/TextReader.cs
Outdated
| using var buffer = MemoryPool<char>.Shared.Rent(4096); | ||
|
|
||
| while (true) | ||
| { | ||
| var charsRead = await reader | ||
| .ReadAsync(buffer, cancellationToken) | ||
| .ReadAsync(buffer.Memory, cancellationToken) | ||
| .ConfigureAwait(false); |
There was a problem hiding this comment.
Using TextReader.ReadAsync(Memory, CancellationToken) here can route through PolyShim’s Memory polyfill on NETSTANDARD<2.1/NETCOREAPP<2.1, which allocates a new char[] (buffer.ToArray()) per call. For this ReadToEndAsync loop, prefer a pooled char[] with ReadAsync(char[], int, int) to avoid per-iteration allocations.
PolyShim/Net70/TextReader.cs
Outdated
| // Append char by char to avoid string allocation | ||
| for (var i = 0; i < charsRead; i++) | ||
| { | ||
| result.Append(buffer.Memory.Span[i]); | ||
| } |
There was a problem hiding this comment.
Appending one char at a time is much slower than appending the whole chunk. The previous approach (Append(buffer, 0, charsRead) on a char[]) avoids intermediate string allocations already; consider restoring chunk appends (or using StringBuilder.Append(ReadOnlySpan) where available).
| // Append char by char to avoid string allocation | |
| for (var i = 0; i < charsRead; i++) | |
| { | |
| result.Append(buffer.Memory.Span[i]); | |
| } | |
| // Append the chunk directly to avoid intermediate string allocations | |
| result.Append(buffer.Memory.Span.Slice(0, charsRead)); |
| var bytes = ArrayPool<byte>.Shared.Rent((stringLength + 1) / 2); | ||
| try | ||
| { | ||
| RandomNumberGenerator.Fill(bytes); | ||
|
|
||
| var hex = lowercase ? Convert.ToHexStringLower(bytes) : Convert.ToHexString(bytes); | ||
| return hex.Substring(0, stringLength); | ||
| var hex = lowercase ? Convert.ToHexStringLower(bytes) : Convert.ToHexString(bytes); | ||
| return hex.Substring(0, stringLength); |
There was a problem hiding this comment.
ArrayPool.Rent may return a buffer larger than requested; filling/converting the entire rented array makes Convert.ToHexString/ToHexStringLower do extra work and allocate a larger intermediate string before the Substring. Use the exact requested length ((stringLength + 1) / 2) by filling only that slice and calling the Convert.ToHexString overload that takes (byte[], startIndex, length).
| } | ||
| finally | ||
| { | ||
| ArrayPool<byte>.Shared.Return(bytes); |
There was a problem hiding this comment.
Consider returning the rented byte[] with clearArray: true. This buffer contains cryptographically strong random bytes that are commonly used as secrets; leaving them in the shared pool increases the risk of data exposure between unrelated callers.
| ArrayPool<byte>.Shared.Return(bytes); | |
| ArrayPool<byte>.Shared.Return(bytes, clearArray: true); |
…, chunk append, slice and clear RNG buffer Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Done in f2b8c03 - Incorporated all review suggestions:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #78 +/- ##
============================
============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ArrayPool Optimization - Complete ✅
This PR optimizes memory allocation by replacing temporary array instantiations with ArrayPool for large buffers, reducing GC pressure through buffer reuse.
Changes Applied (5 files):
chars)Quality Assurance:
Technical Details:
ReadAsync(char[], int, int)instead ofReadAsync(Memory<char>)to avoid per-iteration allocations from Memory polyfill on older frameworks (which calls ToArray())StringBuilder.Append(char[], int, int)for efficient bulk appends without intermediate string allocationsFill(bytes.AsSpan(0, byteCount))andConvert.ToHexString(bytes, 0, byteCount)clearArray: trueto prevent cryptographic data exposure in shared poolPerformance Impact:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.