PngByteQRCode: use ArrayPool.Rent to prevent byte[] allocation
#615
Conversation
📝 WalkthroughWalkthroughReplaces per-call byte[] allocations with ArrayPool-backed buffers in PngByteQRCode for supported targets; changes DrawScanlines to return ArraySegment, updates WriteScanlines/Deflate/Adler32 to accept ArraySegment, and returns rented buffers after use. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as GetGraphic(...)
participant Drawer as DrawScanlines
participant Writer as WriteScanlines
participant Deflater as Deflate
participant Adler as Adler32
rect #EEF6FF
Caller->>Drawer: Request scanlines (pixelsPerModule, drawQuietZones)
note right of Drawer: Rent buffer from ArrayPool when supported\nReturn ArraySegment<byte>
end
rect #F6FFF0
Caller->>Writer: Pass ArraySegment<byte> scanlines
Writer->>Deflater: Deflate(scanlines.Array, scanlines.Count)
Writer->>Adler: Adler32(scanlines.Array, 0, scanlines.Count)
end
rect #FFF6EE
Writer->>Caller: Complete writing
Caller->>Drawer: Return rented buffer to ArrayPool (if rented)
note right of Drawer: Fallback: heap allocation — no pool return needed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TimothyMakkison
force-pushed
the
arraypool_pngbyte
branch
from
d0574cf to
d6ffc67
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
QRCoder/PngByteQRCode.cs (1)
93-136: ArrayPool usage is correct; fix formatting issues.The implementation correctly handles the case where
ArrayPool.Rentreturns a larger array than requested:
- Line 102 clears only the required
scanLinesLengthbytes- Line 135 returns an
ArraySegment<byte>that exposes only the valid portion- All array access within the method stays within bounds
This addresses the concern raised in the past review comment about array length.
However, the pipeline is failing due to whitespace/indentation issues on lines 100, 103, and 105.
Apply this diff to fix the formatting:
var bytesPerScanline = (matrixSize * pixelsPerModule + 7) / 8 + 1; // A monochrome scanline is one byte for filter type then one bit per pixel. var scanLinesLength = bytesPerScanline * matrixSize * pixelsPerModule; - #if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 +#if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 var scanlines = ArrayPool<byte>.Shared.Rent(scanLinesLength); Array.Clear(scanlines, 0, scanLinesLength); - #else +#else var scanlines = new byte[scanLinesLength]; - #endif +#endif for (var y = 0; y < matrixSize; y++)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/PngByteQRCode.cs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/PngByteQRCode.cs (1)
QRCoder/QRCodeGenerator.cs (1)
ArraySegment(652-715)
🪛 GitHub Actions: Format (Pull Request)
QRCoder/PngByteQRCode.cs
[error] 80-80: WHITESPACE: Fix whitespace formatting. Delete 8 characters.
[error] 82-82: WHITESPACE: Fix whitespace formatting. Delete 8 characters.
[error] 83-83: WHITESPACE: Fix whitespace formatting. Replace 8 characters with '\s\s\s\s\s\s\s\s'.
[error] 100-100: WHITESPACE: Fix whitespace formatting. Delete 8 characters.
[error] 103-103: WHITESPACE: Fix whitespace formatting. Delete 8 characters.
[error] 104-104: WHITESPACE: Fix whitespace formatting. Replace 8 characters with '\s\s\s\s\s\s\s\s'.
[error] 105-105: WHITESPACE: Fix whitespace formatting. Delete 8 characters.
[error] 106-106: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n\s\s\s\s\s\s\s\s'.
[error] 80-80: IDE0055: Fix formatting
🪛 GitHub Check: format
QRCoder/PngByteQRCode.cs
[failure] 82-82:
Fix formatting
[failure] 80-80:
Fix formatting
[failure] 82-82:
Fix formatting
[failure] 80-80:
Fix formatting
[failure] 105-105:
Fix formatting
[failure] 103-103:
Fix formatting
[failure] 100-100:
Fix formatting
[failure] 105-105:
Fix formatting
[failure] 103-103:
Fix formatting
[failure] 100-100:
Fix formatting
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
QRCoder/PngByteQRCode.cs (5)
1-3: LGTM!The conditional compilation directive correctly targets platforms where
ArrayPool<T>is available.
42-46: LGTM!The ArrayPool rent-use-return pattern is correctly implemented. The rented buffer is properly returned after use.
269-269: LGTM!The signature change from
byte[]toArraySegment<byte>is necessary to support pooled buffers. SincePngBuilderis a private sealed class, this is not a breaking change.
324-328: LGTM with same offset assumption.The
Deflatemethod correctly accepts and unpacksArraySegment<byte>. The same assumption about offset being 0 applies here as well (see comment on lines 288-289).
288-289: Adler32 offset assumption verified
BothDrawScanlines(PngByteQRCode.cs:135) and related code (QRCodeGenerator.cs:702) instantiateArraySegment<byte>with offset0, so passing0toAdler32is correct.
TimothyMakkison
force-pushed
the
arraypool_pngbyte
branch
from
d6ffc67 to
ea16006
Compare
|
FYI, we use 'squash and merge', so no need to force push if you don't want to. The commit history will show a single commit for the entire PR regardless. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
QRCoder/PngByteQRCode.cs (2)
78-82: Fix formatting issues.The ArrayPool pattern is correct, but the pipeline is failing due to whitespace/indentation on the preprocessor directives.
Apply this diff to fix the formatting:
- var scanLines = DrawScanlines(pixelsPerModule, drawQuietZones); - png.WriteScanlines(scanLines); - #if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 - ArrayPool<byte>.Shared.Return(scanLines.Array!); - #endif + var scanLines = DrawScanlines(pixelsPerModule, drawQuietZones); + png.WriteScanlines(scanLines); +#if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 + ArrayPool<byte>.Shared.Return(scanLines.Array!); +#endif
42-46: Fix formatting issues.The ArrayPool pattern is correct, but the pipeline is failing due to whitespace/indentation on the preprocessor directives.
Apply this diff to fix the formatting:
- var scanLines = DrawScanlines(pixelsPerModule, drawQuietZones); - png.WriteScanlines(scanLines); - #if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 - ArrayPool<byte>.Shared.Return(scanLines.Array!); - #endif + var scanLines = DrawScanlines(pixelsPerModule, drawQuietZones); + png.WriteScanlines(scanLines); +#if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 + ArrayPool<byte>.Shared.Return(scanLines.Array!); +#endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/PngByteQRCode.cs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/PngByteQRCode.cs (1)
QRCoder/QRCodeGenerator.cs (1)
ArraySegment(652-715)
🪛 GitHub Actions: Format (Pull Request)
QRCoder/PngByteQRCode.cs
[error] 100-100: WHITESPACE: Fix whitespace formatting. Delete 8 characters.
[error] 103-103: WHITESPACE: Fix whitespace formatting. Delete 8 characters.
[error] 104-104: WHITESPACE: Fix whitespace formatting. Replace 8 characters with '\s\s\s\s\s\s\s\s'.
[error] 105-105: WHITESPACE: Fix whitespace formatting. Delete 8 characters.
[error] 106-106: WHITESPACE: Fix whitespace formatting. Replace 9 characters with '\n\s\s\s\s\s\s\s\s'.
[error] 100-100: IDE0055: Fix formatting
🪛 GitHub Check: format
QRCoder/PngByteQRCode.cs
[failure] 100-100:
Fix formatting
[failure] 105-105:
Fix formatting
[failure] 103-103:
Fix formatting
[failure] 100-100:
Fix formatting
[failure] 105-105:
Fix formatting
[failure] 103-103:
Fix formatting
[failure] 100-100:
Fix formatting
[failure] 105-105:
Fix formatting
[failure] 103-103:
Fix formatting
[failure] 100-100:
Fix formatting
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
QRCoder/PngByteQRCode.cs (4)
1-3: LGTM!The conditional using directive correctly gates ArrayPool support for compatible target frameworks.
107-136: LGTM!The loop logic correctly bounds all array accesses, and the returned
ArraySegment<byte>properly encapsulates the exact data length regardless of the underlying array size.
269-292: LGTM!The signature change to
ArraySegment<byte>correctly propagates through the call chain. SincePngBuilderis a private sealed class, this is not a breaking change. The null-forgiving operator onscanlines.Array!is safe because both allocation paths guarantee a non-null array.
324-328: LGTM!The
Deflatemethod correctly accepts and usesArraySegment<byte>, properly accessing the underlying array and count. The null-forgiving operator is safe given the guaranteed non-null array from the allocation paths.
Sorry force of habit, good to know |
|
Okay, did my TODO list, I've used Have you considered using a formatter like csharpier for this project? Iirc Mapperly has it to automatically format code as its pushed, would save code rabbit from rescanning the commit |
We have it set up to check that I don't want to use another formatter, like prettier or csharpier, which isn't compatible with the way that VS formats things by default. I have not checked out how to integrate an automatic formatting step into the push process for .NET code bases, although I've done that with my react projects, and it's certainly nice to have. |
|
I don't care too much about Coderabbit's checks; I noticed it was reading the build check outputs here and providing a suggestion based on that. Coderabbit is nice sometimes but can also be a bit verbose (and sometimes wrong). In PR #611 just now it correctly caught a bug unrelated to the PR 👍 . And it's always nice that it writes a PR summary. |
|
Reopening, I realised that my benchmarks weren't measuring PNG QR code performance, so I wanted to verify that the changes were signficant. Here are the actual results using GFoidl's profiling code #595. Before
After
|
|
I copied Before
After
|
Shane32
changed the title
ArrayPool.Rent to prevent byte[] allocationArrayPool.Rent to prevent byte[] allocation
3 participants
WIP
TODO
scanLinesLengthSummary by CodeRabbit