Skip to content

Conversation

@Shane32
Copy link
Owner

@Shane32 Shane32 commented Oct 13, 2025

@gfoidl Although it only takes a few nanoseconds (billionths of a second) to run, it is 30% slower than the original code assuming all characters can be encoded. On the other hand, it is a thousand times faster with zero allocations for a string that requires UTF-8.

Method Mean Error StdDev Gen0 Allocated
OriginalAsciiValid 6.222 ns 0.4550 ns 1.3415 ns - -
OriginalInvalidLastChar 9,625.746 ns 190.4809 ns 247.6790 ns 0.1373 888 B
OptimizedAsciiValid 8.275 ns 0.1907 ns 0.2119 ns - -
OptimizedInvalidLastChar 11.375 ns 0.2461 ns 0.2302 ns - -

I could not find a more optimized approach.

@Shane32 Shane32 added this to the 1.7.1 milestone Oct 13, 2025
@Shane32 Shane32 self-assigned this Oct 13, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 13, 2025

📝 Walkthrough

Walkthrough

Replaced exception-based ISO-8859-1 validation with a direct per-character range check in QRCoder/QRCodeGenerator.cs, removing a static Encoding fallback and associated GetByteCount call.

Changes

Cohort / File(s) Change summary
ISO-8859-1 validation refactor
QRCoder/QRCodeGenerator.cs
Removed encoder-fallback based validation; implemented IsValidISO to iterate characters and return false if any > 0xFF; eliminated static readonly fallback Encoding and exception-driven flow.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

performance

Suggested reviewers

  • gfoidl

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly summarizes the removal of exception-throwing behavior for characters outside the ISO-8859-1 range by highlighting the core change to direct range checks. It aligns directly with the PR’s objective of replacing exception-driven validation with a more efficient implementation. The phrasing is clear and specific, avoiding unnecessary technical detail while conveying the primary functionality. It is concise and understandable to team members scanning the change history.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch iso_no_exception

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Shane32 Shane32 added the performance Performance related enhancements or benchmarks label Oct 13, 2025
@Shane32 Shane32 requested a review from gfoidl October 13, 2025 02:10
Copy link
Collaborator

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoiding the potential allocation is better than trading the few nano seconds here.

Speeding this up could be done by vectorized code, but it gets ugly and long -- especially it can't be implemented for all TFMs.

.NET (in recent versions) provides Ascii.IsValid (and Utf8.IsValid), which use a vectorized approach, so we could check for valid ascii (the majority of inputs will be ascii) first, and when not fallback to the manual loop. In the worst case that's $O(2n)$.
But for what? Just a few ns in trade for complicated code...

I'm fine with the solution you have here.

@Shane32 Shane32 merged commit 4cb1f83 into master Oct 13, 2025
8 checks passed
@Shane32 Shane32 deleted the iso_no_exception branch October 13, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Performance related enhancements or benchmarks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants