Conversation
Skip regex scanning in BaseParser#unnormalize and Text.unnormalize when the string has valid encoding and contains no '&' or '\r'. Because all reference patterns matched by REFERENCE and REFERENCE_RE begin with '&', strings without '&' can never match and can safely bypass the regex scan. Also add benchmark/text.yaml to measure Text.unnormalize performance.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The optimization preserves fallback semantics and includes focused regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Adds safe fast paths for entity-free text unnormalization, reducing unnecessary regex work and allocations.
Changes:
- Bypasses entity scanning for valid strings without ampersands.
- Preserves CRLF normalization and invalid-encoding behavior.
- Adds regression tests and direct performance benchmarks.
| File | Description |
|---|---|
lib/rexml/text.rb |
Adds fast paths to Text.unnormalize. |
lib/rexml/parsers/baseparser.rb |
Skips reference scanning when no ampersand exists. |
test/test_text.rb |
Tests plain text, CRLF, and invalid encoding. |
test/parser/test_base_parser.rb |
Tests parser unnormalization fast-path behavior. |
benchmark/text.yaml |
Benchmarks plain, CRLF, and entity-containing text. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a fast path to
BaseParser#unnormalizeandText.unnormalizewhen a string does not contain any entity references (&) or carriage returns (\r).In XML, all general entity references (
&name;) and numeric character references (&#...;) must begin with&. In REXML, bothText::REFERENCEandBaseParser::REFERENCE_REare based onXMLTokens::REFERENCE:Because every branch in this pattern strictly begins with
&, it is guaranteed that the regex will never match any string without&. Therefore, when an input contains no&and no\r, unnormalization is effectively a no-op that simply returns a duplicate of the string.By checking
rv.include?("&")andrv.valid_encoding?before running regex scans (rv.scan(REFERENCE_RE) and rv.gsub(REFERENCE)), regex execution is completely bypassed for plain text nodes.Benchmark & Performance
Existing Benchmarks (benchmark/*.yaml)
In the existing benchmark suite:
• benchmark/xpath_sort.yaml (//item[string(a) = 'v3'], which evaluates string values of plain text nodes):
• benchmark/parse_doctype.yaml (extracting text nodes containing entity references):
• Confirmed zero performance degradation across all test cases (single_entity, chained_entities, many_entities, repeated_entity). The fallback path introduces no measurable overhead.
New Benchmark (benchmark/text.yaml)
Added benchmark/text.yaml to measure
Text.unnormalizedirectly across various inputs:• Plain text extraction: +50% faster on CRuby (1.01M -> 1.52M i/s), and +140% (2.4x) faster with YJIT (1.69M -> 4.06M i/s).
• Longer plain text (1,000 chars): 5.3x faster on CRuby (278k -> 1.48M i/s), and 12.6x faster with YJIT (283k -> 3.56M i/s), as regex scanning over long strings is avoided entirely.
• Entity fallback: Zero overhead for strings that actually contain entities (matches master performance within noise margin).
Direct measurement of 25,000 accesses of plain text nodes via Text#value (Ruby 4.0.7):
• Eliminates 2 out of 3 object allocations on plain text node value extraction.
Backward Compatibility & Security
• Entity expansion limits: Any input containing
&(including entity bombs like Billion Laughs or quadratic expansion) does not take the fast path and goes through the existing Security.entity_expansion_text_limit checks.• Character reference validation: Numeric character references like
�also begin with&and continue to be strictly validated viaText.expand_character_reference(GH-372).• Invalid byte sequences: By guarding with
valid_encoding?, strings with invalid encodings continue to raiseArgumentErroridentically to the unoptimized path. Verified with 10,000 randomized fuzzing inputs matching master behavior 100%.• CRLF handling: Strings containing
\rcontinue to have newlines normalized to\nper the XML specification.Tests
• Added tests in test/test_text.rb and test/parser/test_base_parser.rb verifying entity-free strings and ensuring ArgumentError is raised on invalid byte sequences.
• Added benchmark/text.yaml for tracking unnormalization performance.
• All 843 tests and 2682 assertions pass (100%).