-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add ASCII renderer optimizations #652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughReplaces dynamic list with a pre-sized Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant ASCIIQRCode as ASCIIQRCode.Generate
participant SpanPath as HAS_SPAN Fast Path
participant Builder as StringBuilder Fallback
Caller->>ASCIIQRCode: Generate ASCII QR lines
ASCIIQRCode->>ASCIIQRCode: compute sideLength & lineCapacity
alt HAS_SPAN enabled && token lengths equal && small capacity
ASCIIQRCode->>SpanPath: stackalloc Span<char>, fill each line directly
SpanPath-->>ASCIIQRCode: populated string[] lines
ASCIIQRCode-->>Caller: return string[] lines
else Fallback
ASCIIQRCode->>Builder: reuse/reset builder per line, append blocks
Builder-->>ASCIIQRCode: write resulting string into pre-sized array
ASCIIQRCode-->>Caller: return string[] lines
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoder/ASCIIQRCode.cs (1)
71-74: Cache spans outside the hot loop
AsSpan()gets re-evaluated for every module even though the underlying strings never change. Cachingvar darkSpan = darkColorString.AsSpan();/var whiteSpan = whiteSpaceString.AsSpan();once before the loops lets you reuse them in the inner loop and avoids repeated method calls in this perf-sensitive path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/ASCIIQRCode.cs(2 hunks)
⏰ 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). (5)
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET Core 3.1
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: additional-tests
Co-authored-by: Günther Foidl <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoder/ASCIIQRCode.cs (1)
59-81: Verify thelineCapacity < 510threshold.The 510 limit leaves a 2-char safety buffer in the 512-char stackalloc, but this might unnecessarily exclude cases where
lineCapacityis 510-512. Unless there's a specific reason for this buffer, consider using<= 512to maximize the fast path coverage.If the 2-char buffer is intentional, adding a brief comment explaining why would improve maintainability:
+ // Limit to 510 to leave buffer space for safety/alignment if (darkColorString.Length == whiteSpaceString.Length && lineCapacity < 510)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
QRCoder/ASCIIQRCode.cs(2 hunks)
⏰ 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). (6)
- GitHub Check: Test .NET Framework 4.6.2
- GitHub Check: Test .NET 6.0 Windows
- GitHub Check: Test .NET Core 2.1
- GitHub Check: Test .NET 5.0 Windows
- GitHub Check: Test .NET Core 3.1
- GitHub Check: additional-tests
🔇 Additional comments (3)
QRCoder/ASCIIQRCode.cs (3)
57-58: LGTM! Pre-allocation eliminates List overhead.Pre-computing
lineCapacityand switching to a fixed-size array removes dynamic growth costs and enables the span optimization below.
82-82: LGTM! StringBuilder capacity optimization.Initializing the
StringBuilderwithlineCapacityand reusing it across iterations (with reset on line 94) eliminates repeated buffer allocations in the fallback path.
93-96: LGTM! Efficient array population and return.Direct array indexing (line 93) and
StringBuilderreset viaLength = 0(line 94) are efficient choices. The direct return of the pre-allocated array (line 96) completes the optimization, withGetGraphic(line 35) handling the join operation as needed.
Summary by CodeRabbit