Skip to content

perf(security): avoid Unicode folding for ASCII characters - #907

Merged
rng1995 merged 1 commit into
NVIDIA:mainfrom
longfie:perf/security-character-folding-866
Oct 28, 2026
Merged

rng1995 merged 1 commit into
NVIDIA:mainfrom
longfie:perf/security-character-folding-866

Conversation

@longfie

@longfie longfie commented Oct 25, 2026

Copy link
Copy Markdown

Summary

Refs #866.

Add ASCII fast paths to _security_skeleton_piece and _fold_security_character. NFKC and the pinned confusable map leave ASCII unchanged, so ordinary characters need only lowercasing (and the existing one-letter alphabetic check).

This is a deliberately narrow change: five production lines, no new cache, no change to non-ASCII normalization, source offsets, or runtime checks. It addresses two of the per-character hotspots in #866, not every bottleneck in that issue. It does not duplicate the token-gap seeking, confusable containment, or view-cache work in #583.

Regression coverage

  • All 128 ASCII characters, including controls and punctuation, match the original Unicode pipeline.
  • ASCII folding makes no Unicode normalization calls.
  • Dotted I, confusables, fullwidth characters, multi-character expansions, combining marks, surrogates, and the empty sentinel retain their behavior.
  • Instruction matches and runtime callback counts agree with the original implementation on plain, ASCII-obfuscated, Unicode-obfuscated, and multi-checkpoint inputs.
  • The new test file fails against the base revision: 1 failed, 26 passed. With this change: 27 passed.

Validation

macOS, Python 3.14.8, base d7338f58200a9ae2a24810cee08911ab68b107a7:

  • make format, make lint, make format-check: pass.
  • Focused security suites: 606 passed.
  • make test-unit: 15427 passed, 14 skipped, 250 deselected, 4 xfailed.
  • make test-integration: 241 passed, 5 skipped, 15449 deselected.
  • Separate exhaustive comparison of both helpers across 1,114,112 Unicode code points: zero differences.
  • Full CLI --no-llm --format json scans of three local fixtures agree on every report field except timestamps and random finding IDs: plain text (score 0, 0 findings), ASCII-obfuscated instruction (42, 2), Unicode-obfuscated instruction (50, 3).

The suites report existing unregistered timeout marker warnings and a duplicate ZIP member fixture warning. Live provider tests were not run; provider credentials were removed from the test environment.

Performance scope

For all 128 ASCII characters repeated 100 times through both helpers, normalization calls fall from 25,600 to 0, with identical output. Restoring the original source restores 25,600 calls and the original file hash.

An eight-round alternating microbenchmark over 270,336 ASCII characters measured median combined helper time of 69.1 ms before / 28.5 ms after (~2.42x). This is a helper microbenchmark, not an end-to-end scanner speedup claim.

Checklist

  • Focused change linked to an existing issue
  • Regression coverage and local unit/integration checks
  • DCO-signed commit

Skip NFKC normalization and confusable translation for ASCII characters in
the security skeleton helpers. Preserve the Unicode fallback and the
one-letter ASCII sentinel, without adding caches or changing runtime checks.

Add regression coverage for ASCII equivalence, Unicode edge cases,
non-character inputs, and unchanged instruction matches and runtime checks.

Refs NVIDIA#866

Signed-off-by: longankang <1790716272@qq.com>

@longfie longfie left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Self-review of d4fb9f9:

  • The published diff contains only the two ASCII fast paths and their regression tests.
  • The non-ASCII path, one-letter sentinel, source spans, and runtime-check cadence remain unchanged. There is no new cache or retained input.
  • The ASCII shortcut is guarded by a regression asserting the pinned confusable map contains no ASCII keys.
  • New regression: 1 failure on the base revision, all 27 tests pass on this commit. Focused suites: 606 passed. Full local unit suite: 15427 passed; integration suite: 241 passed.
  • Exhaustive helper comparison across all Unicode code points found no output differences. Full scanner fixture reports agree apart from timestamps/random IDs.
  • Performance numbers are helper-level measurements only; this does not claim to resolve every hotspot in #866.

No blocking findings in this self-review. This is not independent maintainer approval; upstream CI and maintainer review are still required.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Value and readiness

The five production lines remove Unicode normalization work for ASCII while retaining the original non-ASCII pipeline and runtime checkpoints. I found no required code change at d4fb9f9e26eb103e15b05ef7f4756af308542de2.

Material findings

No required change found. ASCII NFKC/confusable/casefold output equals lowercasing; the one-letter alphabetic sentinel is preserved for empty, multicharacter, control and punctuation inputs. The new tests guard the confusable-map invariant, all ASCII code points, representative Unicode cases, detection output and callback counts. The author self-review raises no outstanding finding.

PIC tradeoffs

The measured speedup is limited to these helpers; the PR correctly avoids claiming an end-to-end scanner improvement.

Verification and gaps

Read both complete changed files, description, author self-review, and empty comments/threads. The authored regression includes a base failure and reports exhaustive Unicode comparison; these are author-reported results. All six hosted checks succeeded. I performed static review without running contributor code.

@rng1995
rng1995 merged commit 4ca1d7c into NVIDIA:main Oct 28, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants