perf(parser): Make all parsers work by byte instead of char

[kdy1]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 18bf7dd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Code Review: PR #11318 - Performance optimization: Make all parsers work by byte instead of char

Overview

This PR refactors the input parsing infrastructure to operate on bytes instead of chars, which is a significant performance optimization. The changes are substantial but well-structured.

Positive Aspects ✅

1. Performance-focused design: The byte-level operations should significantly improve parsing performance, especially for ASCII-heavy code (which is the common case).

2. Careful UTF-8 handling: The bump() implementation in crates/swc_common/src/input.rs:133-156 correctly calculates UTF-8 character lengths:

   • 1 byte for ASCII (< 0x80)
   • 2 bytes for chars < 0xe0
   • 3 bytes for chars < 0xf0
   • 4 bytes for chars >= 0xf0

3. Fallback mechanism: The code properly maintains cur_as_char() for cases where full Unicode character processing is needed (identifiers, strings, etc.), showing good design thinking.

4. Test coverage: The existing unit tests have been updated to reflect the byte-based API changes.

Issues & Concerns 🔴

1. Potential UTF-8 boundary safety issue (High Priority)

Location: crates/swc_common/src/input.rs:144-152

The UTF-8 length calculation in bump() uses a simplified heuristic that may not be robust for all invalid UTF-8 sequences:

let len = if first_byte < 0x80 {
    1 // ASCII
} else if first_byte < 0xe0 {
    2 // 2-byte UTF-8
} else if first_byte < 0xf0 {
    3 // 3-byte UTF-8
} else {
    4 // 4-byte UTF-8
};

Issue: This doesn't validate that the subsequent bytes are valid UTF-8 continuation bytes (should be in range 0x80-0xBF). While the input is presumably valid UTF-8 (since it comes from a &str), slicing at incorrect boundaries could cause panics in debug builds or undefined behavior.

Recommendation: Since the input is &str, we know it's valid UTF-8. However, add a comment explaining this assumption and consider using str::chars().next() to get the actual character length for the fast path, or add debug assertions to validate continuation bytes.

2. Unsafe code without complete documentation (Medium Priority)

Locations: Multiple instances throughout the codebase

Many unsafe blocks lack detailed safety comments explaining why the operation is safe. Examples:

• crates/swc_common/src/input.rs:71, 79, 154
• crates/swc_ecma_lexer/src/common/lexer/mod.rs: numerous unsafe slicing operations

Recommendation: Per the project's guidelines (#1 and #7 in CLAUDE.md), add comprehensive safety documentation. For example:

// Safety: first_byte comes from self.remaining.as_bytes() which is guaranteed to be
// valid UTF-8. The length calculation follows UTF-8 encoding rules (RFC 3629).
// get_unchecked is safe because we've checked that remaining is not empty.

3. Incorrect ASCII validation (Medium Priority)

Location: crates/swc_common/src/input.rs:160-165

fn cur_as_ascii(&self) -> Option<u8> {
    let first_byte = *self.remaining.as_bytes().first()?;
    if first_byte <= 0x7f {
        Some(first_byte)
    } else {
        None
    }
}

Issue: The check first_byte <= 0x7f should be first_byte < 0x80 for consistency with ASCII definition (0-127). While functionally equivalent, using < 0x80 is more idiomatic and matches the pattern used elsewhere (line 144).

Recommendation: Change to if first_byte < 0x80 for consistency.

4. Missing boundary checks (Medium Priority)

Location: crates/swc_common/src/input.rs:84-92

The bump_one() method has a debug_assert! but will silently corrupt state in release builds if called at EOF:

pub fn bump_one(&mut self) {
    if !self.remaining.is_empty() {
        self.remaining = unsafe { self.remaining.get_unchecked(1..) };
        self.last_pos.0 += 1;
    } else {
        unsafe {
            debug_unreachable!("bump should not be called when cur() == None");
        }
    }
}

Issue: The else branch will still execute in release mode but does nothing, potentially causing position tracking issues.

Recommendation: Consider removing the else branch entirely or adding a panic in debug mode that becomes an unreachable hint in release mode.

5. Complex escape sequence handling (Low Priority)

Location: crates/swc_css_parser/src/lexer/mod.rs:709-787

The string reading logic has become more complex with the byte-based approach, particularly around escape sequences where we need to convert bytes back to chars:

let cur_byte = l.input.cur();
let cur_char = if let Some(b) = cur_byte {
    if is_non_ascii(b) {
        l.input.cur_as_char()
    } else {
        Some(b as char)
    }
} else {
    None
};

Recommendation: This is correct but verbose. Consider extracting this pattern into a helper method like cur_as_char_for_string() to reduce duplication and improve readability.

Performance Considerations ⚡

1. Excellent byte-search optimization: The use of byte_search! macros with lookup tables (lines 51-66 in lexer/mod.rs) for scanning is an excellent optimization pattern.

2. Smart fast-path design: The code correctly identifies that ASCII is the common case and optimizes for it while falling back to slower Unicode handling when needed.

3. Reduced allocations: Operating on byte slices rather than iterating over chars should reduce allocations and improve cache locality.

Testing Recommendations 🧪

Per CLAUDE.md guideline #5, please ensure:

1. Add tests for edge cases:

   • Multi-byte UTF-8 sequences (2, 3, and 4-byte sequences)
   • Strings containing emoji and other non-BMP characters
   • Mixed ASCII and Unicode content
   • Strings with escape sequences followed by Unicode

2. Add benchmarks comparing before/after performance, especially for:

   • Large ASCII files
   • Files with mixed ASCII/Unicode
   • Files with many escape sequences

Example test to add:

#[test]
fn test_multibyte_utf8() {
    with_test_sess("hello 世界 🌍", |mut i| {
        // Test bump() correctly handles multi-byte sequences
        assert_eq!(i.cur(), Some(b'h'));
        // ... test full sequence
    });
}

Documentation 📚

Per CLAUDE.md guideline #7, please add:

1. Module-level documentation explaining the byte-based approach and when to use cur_as_char()
2. Safety invariants for the Input trait
3. Examples showing proper usage patterns

Minor Issues

1. Formatting: Remember to run cargo fmt --all before committing (CLAUDE.md Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)

2. Code comment clarity: Some comments could be more precise:

   • Line 13 in input.rs: "Remaining input as str" could be "Remaining unparsed input slice"

Summary

This is a well-thought-out performance optimization with good architectural decisions. The main concerns are:

• Must fix: UTF-8 boundary handling needs more robust validation or better documentation of safety assumptions
• Should fix: Unsafe code needs comprehensive safety documentation
• Should fix: Add comprehensive tests for multi-byte UTF-8 sequences
• Nice to have: Minor consistency improvements and helper methods for common patterns

The performance benefits should be significant, especially for typical JavaScript/TypeScript code which is mostly ASCII. Great work on maintaining the fast-path optimization pattern!

Overall Assessment: Approve with requested changes ✅ (pending fixes for UTF-8 safety and documentation)

🤖 Generated with Claude Code

[github-actions]

Binary Sizes

File	Size
swc.linux-x64-gnu.node	31M (31920840 bytes)

Commit: f324cd3

[codspeed-hq]

CodSpeed Performance Report

Merging #11318 will degrade performances by 5.03%

_{Comparing kdy1/parser-by-byte (18bf7dd) with main (88c6ac7)¹}

Summary

⚡ 6 improvements
❌ 5 regressions
✅ 127 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	css/lexer/bootstrap_5_1_3	11 ms	11.5 ms	-5.03%
❌	css/lexer/foundation_6_7_4	9 ms	9.4 ms	-4.8%
❌	css/lexer/tailwind_3_1_1	1.8 ms	1.9 ms	-4.48%
❌	es/lexer/cal-com	13.1 ms	13.5 ms	-3.16%
⚡	es/lexer/colors	26.4 µs	25.5 µs	+3.4%
❌	es/lexer/typescript	107.6 ms	110.6 ms	-2.72%
⚡	html/lexer/css_2021_spec	40 ms	38.6 ms	+3.48%
⚡	html/lexer/github_com_17_05_2022	14.4 ms	14 ms	+3.03%
⚡	html/lexer/stackoverflow_com_17_05_2022	14.5 ms	14 ms	+3.31%
⚡	html/parser/parser_document/stackoverflow_com_17_05_2022	22.7 ms	22.2 ms	+2.05%
⚡	html/parser/parser_document_fragment/stackoverflow_com_17_05_2022	22.6 ms	22.1 ms	+2.06%

Footnotes

1. No successful run was found on main (b0d3154) during the generation of this report, so 88c6ac7 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[claude]

Code Review for PR #11318: perf(parser): Make all parsers work by byte instead of char

I've reviewed this PR which converts the parsers from character-based to byte-based processing for improved performance. Here's my detailed feedback:

✅ Strengths

1. Well-structured refactoring: The conversion from char to u8 is systematic and consistent across all parsers (CSS, ECMAScript, HTML, XML).

2. Smart hybrid approach: The code correctly distinguishes between:

   • Fast path: ASCII bytes (< 0x80) processed directly as bytes
   • Slow path: Non-ASCII bytes where cur_as_char() is called to get full UTF-8 characters

3. Correct UTF-8 byte length calculation: The bump() implementation in crates/swc_common/src/input.rs:143-152 correctly calculates UTF-8 character lengths:

   let len = if first_byte < 0x80 {
       1 // ASCII
   } else if first_byte < 0xe0 {
       2 // 2-byte UTF-8
   } else if first_byte < 0xf0 {
       3 // 3-byte UTF-8
   } else {
       4 // 4-byte UTF-8
   };

   This matches the UTF-8 specification correctly.

4. Performance optimization: The new bump_bytes(n) method allows efficient advancement when the byte count is already known, avoiding redundant character boundary calculations.

5. Proper bump_one() usage: The method is correctly restricted to single-byte ASCII operations (see crates/swc_ecma_parser/src/lexer/table.rs:346).

⚠️ Potential Issues & Concerns

1. Invalid UTF-8 handling (Medium severity)

The UTF-8 byte length calculation assumes valid UTF-8 input. For invalid bytes (0xC0-0xC1, 0xF5-0xFF), it will calculate incorrect lengths:

• 0xC0, 0xC1: Invalid overlong encodings, but treated as 2-byte
• 0xF5-0xFF: Invalid (beyond Unicode range), but treated as 4-byte

Recommendation: Since parsers typically receive valid UTF-8 from Rust strings, this is acceptable. However, consider adding a debug assertion or documentation comment noting this assumption.

Location: crates/swc_common/src/input.rs:143-152

2. Missing UTF-8 boundary validation in bump_bytes() (Low severity)

The bump_bytes() method has a debug assertion for length but doesn't verify UTF-8 character boundaries:

pub fn bump_bytes(&mut self, n: usize) {
    debug_assert!(n <= self.remaining.len());
    self.remaining = unsafe { self.remaining.get_unchecked(n..) };
    self.last_pos.0 += n as u32;
}

Recommendation: Add a debug assertion or documentation comment that callers must ensure n aligns with UTF-8 character boundaries.

Location: crates/swc_common/src/input.rs:76-80

3. Character conversion pattern in CSS/HTML parsers (Low severity)

Throughout the CSS and HTML lexers, there's a repetitive pattern:

let ch = if is_non_ascii(c) {
    l.input.cur_as_char()
} else {
    Some(c as char)
};

This is done before consuming the byte. While correct, it adds cognitive overhead.

Recommendation: Consider creating a helper method like cur_as_full_char() that encapsulates this pattern. Example:

impl Input {
    fn cur_as_full_char(&self) -> Option<char> {
        match self.cur()? {
            c if c < 0x80 => Some(c as char),
            _ => self.cur_as_char()
        }
    }
}

Locations: Multiple occurrences in crates/swc_css_parser/src/lexer/mod.rs and crates/swc_html_parser/src/lexer/mod.rs

4. Test coverage (Medium severity)

I notice the PR has a test failure ("Test with @swc/cli" failed). Additionally:

• No new unit tests specifically for the byte-based Input trait changes
• Existing tests in crates/swc_common/src/input.rs:392-425 were updated but not expanded

Recommendation:

• Fix the failing test
• Add unit tests covering:
  • Multi-byte UTF-8 characters (2, 3, and 4 bytes)
  • Mixed ASCII and non-ASCII text
  • bump_bytes() with various byte counts
  • Edge cases with special Unicode characters (emoji, etc.)

5. Performance verification (Low severity)

While the change should improve performance, I don't see benchmark results in the PR description.

Recommendation: Run the existing benchmarks and include results showing the performance improvement this change provides.

📋 Code Quality

1. Comments: The code has good inline comments explaining the byte-based approach
2. Follows CLAUDE.md: ✅ The code avoids unstable features, has comments in English, and appears performant
3. Documentation: ⚠️ The trait methods could use more doc comments explaining the byte vs. char distinction

🔒 Security

No security concerns identified. The unsafe code blocks are properly justified and bounded by debug assertions.

📊 Summary

This is a solid performance optimization with generally good implementation. The main concerns are:

1. Fix the failing test
2. Add more comprehensive test coverage for UTF-8 handling
3. Consider the recommendations for invalid UTF-8 and helper methods

The core algorithm is correct, and the refactoring is well-executed. Once the test failure is resolved and coverage is improved, this should be ready to merge.

---

Review generated by Claude Code 🤖