Eliminate heap allocations in Random/RandomNumberGenerator polyfills#85
Eliminate heap allocations in Random/RandomNumberGenerator polyfills#85
Conversation
… singleton Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
…tting 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 pull request eliminates heap allocations in Random and RandomNumberGenerator polyfill implementations by utilizing ArrayPool<T> for temporary buffers instead of allocating new arrays on every call. The changes also introduce a cached singleton RandomNumberGenerator instance to avoid repeatedly creating and disposing native OS crypto handles, and replace StringBuilder with pooled char arrays for fixed-length string generation.
Changes:
- Replace heap-allocated byte arrays with
ArrayPool<byte>rentals in Random and RandomNumberGenerator polyfills - Introduce cached singleton RandomNumberGenerator instance to avoid create/dispose overhead
- Replace StringBuilder with
ArrayPool<char>for fixed-length string generation - Optimize substring operations to avoid redundant string copies when output is already correct length
- Properly clear cryptographic data from pooled buffers using
clearArray: true
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| PolyShim/NetCore21/Random.cs | Uses ArrayPool for NextBytes(Span) instead of buffer.ToArray() |
| PolyShim/NetCore21/RandomNumberGenerator.cs | Uses ArrayPool for GetBytes(Span) and introduces cached singleton RNG for Fill() |
| PolyShim/NetCore30/RandomNumberGenerator.cs | Uses ArrayPool in GetInt32 rejection loop and cached singleton RNG |
| PolyShim/Net60/Random.cs | Uses ArrayPool for NextInt64 and NextSingle instead of allocating byte arrays in loops |
| PolyShim/Net80/RandomNumberGenerator.cs | Replaces StringBuilder with ArrayPool in GetString and optimizes substring in GetHexString |
| PolyShim/Net100/Random.cs | Uses ArrayPool in GetHexString, replaces StringBuilder with ArrayPool in GetString, and optimizes substring |
The changes are well-structured and follow good practices for most parts. However, there is one performance issue in NetCore30/RandomNumberGenerator.cs that should be addressed before merging.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PolyShim/NetCore21/Random.cs
Outdated
| var bufferArray = ArrayPool<byte>.Shared.Rent(buffer.Length); | ||
| try | ||
| { | ||
| random.NextBytes(bufferArray); |
There was a problem hiding this comment.
This can populate more than necessary
There was a problem hiding this comment.
Fixed in b84a204 — reverted to new byte[buffer.Length] instead of ArrayPool.Rent. Since Random.NextBytes(byte[]) fills the entire array and has no count-limited overload, using a rented buffer (which may be larger) would over-generate. The exact-size allocation avoids this.
…ditionals, formatting 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
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #85 +/- ##
============================
============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
NetCore21/Random.cs: Reverted ArrayPool approach — usenew byte[buffer.Length]sinceRandom.NextBytes(byte[])has no count overload and would over-generate with a larger rented buffer; removed unusedusing System.BuffersNetCore21/RandomNumberGenerator.cs: UseArrayPool<byte>forGetBytes(Span<byte>); cacheRandomNumberGeneratorExsingleton —Instanceas propertyNetCore30/RandomNumberGenerator.cs: Cache singleton RNG; useGetBytes(buffer, 0, 4)to avoid over-generating bytes —Instanceas propertyNet60/Random.cs: Usebuffer.AsSpan(0, 8)/buffer.AsSpan(0, 4)when callingNextByteson rented buffers to avoid over-generating bytesNet80/RandomNumberGenerator.cs: UseArrayPool<char>forGetString;clearArray: true; remove redundanthex.Length == stringLengthconditional fromSubstringNet100/Random.cs: UseArrayPool<char>forGetString; useArrayPool<byte>forGetHexString; remove redundanthex.Length == stringLengthconditional; add blank line beforetryOriginal prompt
Overview
Several polyfill implementations in this repository perform unnecessary heap allocations that can be eliminated using
ArrayPool<T>(already available as a polyfill itself), pooledchar[]instead ofStringBuilderfor fixed-length string building, and a cachedRandomNumberGeneratorsingleton. The following files all need changes.1.
PolyShim/NetCore21/Random.cs—NextBytes(Span<byte>)Problem:
buffer.ToArray()allocates a newbyte[]on every call.Fix: Use
ArrayPool<byte>:Also add
using System.Buffers;to the file.2.
PolyShim/NetCore21/RandomNumberGenerator.cs—GetBytes(Span<byte>)andFill(Span<byte>)Problem 1:
GetBytes(Span<byte>)allocatesnew byte[data.Length]on every call.Fix: Use
ArrayPool<byte>(withclearArray: truesince this is crypto data):Problem 2:
Fill(Span<byte>)callsRandomNumberGenerator.Create()andDispose()on every single call — creating and destroying a native OS crypto handle each time.Fix: Cache a shared static singleton (thread-safe, as
RandomNumberGeneratorimplementations are thread-safe):Add a file-scoped helper class before the polyfill class:
Then update
Fill:Also add
using System.Buffers;to the file.3.
PolyShim/NetCore30/RandomNumberGenerator.cs—GetInt32—new byte[4]inside rejection-sampling loop +Create()per callProblem: Both
new byte[4]inside the loop ANDRandomNumberGenerator.Create()per call:Fix: Cache a singleton RNG, and use
ArrayPool<byte>rented outside the loop:Add a file-scoped helper class:
Then update
GetInt32:Also add
using System.Buffers;to the file.4.
PolyShim/Net60/Random.cs—NextInt64andNextSingleProblem 1:
NextInt64allocatesnew byte[8]inside the rejection-s...This pull request was created from Copilot chat.
💡 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.