perf(md/parse): restore parser state via checkpoint - #11257
Conversation
|
Merging this PR will improve performance by 53.34%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
ce78d01 to
1b29623
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughThe Markdown parser now stores complete parser checkpoints and shared emphasis contexts. Paragraph and ATX heading parsing use deferred-inline start and finish methods. Deferred fragments are reparsed in place, with validation for event ranges, source boundaries, subtree completeness, token alignment, and definition counts. Invalid fragments return failure. Diagnostics and trivia merge into the existing output. No-progress recovery now emits Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/biome_markdown_parser/src/parser.rs (1)
532-552: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for the deferred-inline start/finish pair.
start_deferred_inlineandfinish_deferred_inlinenow form a paired API, andDeferredInlineStartcarries the invariant that the caller must callfinish_deferred_inlineat the exact end of the inline region. That invariant is not recoverable from the names and types. Document it on both methods, including the fact that the recordedevent_rangemust contain the complete subtree expected by the flavour.As per coding guidelines: "For developer-facing comments, explain behavior, invariants, panics, module rationale, or non-obvious rationale" and "Use rustdoc documentation for documenting new features".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/biome_markdown_parser/src/parser.rs` around lines 532 - 552, Add rustdoc to both start_deferred_inline and finish_deferred_inline documenting that they must be used as a pair, with finish_deferred_inline called at the exact end of the inline region. Describe that the resulting event_range must contain the complete subtree required by the selected DeferredInlineFlavor.Source: Coding guidelines
crates/biome_markdown_parser/src/to_html.rs (1)
2123-2126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a separate test for the heading case.
This assertion covers a reference link inside an ATX heading with trailing hard-break spaces. That is the new deferred
AtxParagraphpath, not emphasis scoping, so the enclosing test name no longer describes all of its content. A dedicated test makes the failure attribution clearer if the heading path regresses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/biome_markdown_parser/src/to_html.rs` around lines 2123 - 2126, Extract the ATX heading reference-link assertion from the existing test into a dedicated test focused on deferred AtxParagraph heading rendering, leaving the existing test scoped to emphasis behavior. Preserve the assertion input and expected HTML exactly so regressions identify the heading path separately.crates/biome_markdown_parser/src/inline_phase.rs (1)
336-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo assertions in these tests cannot fail.
validate_deferred_inlinestakes&MarkdownParserOutput, sooutput.events.len()andoutput.deferred_inlines.len()cannot change. The names promise that the output is left unchanged, but the checks are tautological, and a reader may believe the mutating path is covered. Either drop the two length assertions, or callparse_deferred_inlineswith a mutable output so the "leaves output unchanged" claim is actually tested.The name
invalid_deferred_range_leaves_output_unchangedis also slightly off:0..3is a valid range for three events, and the rejection comes from the non-char-boundary source slice on"é".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/biome_markdown_parser/src/inline_phase.rs` around lines 336 - 374, Remove the tautological events and deferred_inlines length assertions from invalid_deferred_range_leaves_output_unchanged, since validate_deferred_inlines receives an immutable output. Rename the test to describe rejection caused by the invalid UTF-8 character boundary in the source slice, and keep the existing validation assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/biome_markdown_parser/src/inline_phase.rs`:
- Around line 72-79: Add release-safe validation to the replacement loop around
`event_range` before subtracting or consuming events, rejecting overlapping,
out-of-order, or invalid half-open ranges. On invalid input, fail gracefully
using the same behavior as the existing fragment path, while
preserving/restoring `output.events` if it has already been taken; do not rely
on the debug-only `validate_deferred_inlines` assertion.
---
Nitpick comments:
In `@crates/biome_markdown_parser/src/inline_phase.rs`:
- Around line 336-374: Remove the tautological events and deferred_inlines
length assertions from invalid_deferred_range_leaves_output_unchanged, since
validate_deferred_inlines receives an immutable output. Rename the test to
describe rejection caused by the invalid UTF-8 character boundary in the source
slice, and keep the existing validation assertion.
In `@crates/biome_markdown_parser/src/parser.rs`:
- Around line 532-552: Add rustdoc to both start_deferred_inline and
finish_deferred_inline documenting that they must be used as a pair, with
finish_deferred_inline called at the exact end of the inline region. Describe
that the resulting event_range must contain the complete subtree required by the
selected DeferredInlineFlavor.
In `@crates/biome_markdown_parser/src/to_html.rs`:
- Around line 2123-2126: Extract the ATX heading reference-link assertion from
the existing test into a dedicated test focused on deferred AtxParagraph heading
rendering, leaving the existing test scoped to emphasis behavior. Preserve the
assertion input and expected HTML exactly so regressions identify the heading
path separately.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2eb530b2-0ae5-4a12-b2c8-c8fb0b2eb252
📒 Files selected for processing (7)
crates/biome_markdown_parser/src/inline_phase.rscrates/biome_markdown_parser/src/lib.rscrates/biome_markdown_parser/src/parser.rscrates/biome_markdown_parser/src/syntax/header.rscrates/biome_markdown_parser/src/syntax/inline/emphasis.rscrates/biome_markdown_parser/src/syntax/mod.rscrates/biome_markdown_parser/src/to_html.rs
1b29623 to
e174f9b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_markdown_parser/src/parser.rs (1)
483-489: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant doc comment.
Line 483 repeats
set_emphasis_contextand its return type. Remove it, or document an invariant that callers must preserve.As per coding guidelines, “Avoid comments whose content is already recoverable from names and types.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/biome_markdown_parser/src/parser.rs` around lines 483 - 489, Remove the redundant doc comment immediately above set_emphasis_context; do not replace it unless documenting a non-obvious invariant callers must preserve.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/biome_markdown_parser/src/parser.rs`:
- Around line 483-489: Remove the redundant doc comment immediately above
set_emphasis_context; do not replace it unless documenting a non-obvious
invariant callers must preserve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c1ed4f8-2f1a-422b-856d-32d0fce07876
📒 Files selected for processing (7)
crates/biome_markdown_parser/src/inline_phase.rscrates/biome_markdown_parser/src/lib.rscrates/biome_markdown_parser/src/parser.rscrates/biome_markdown_parser/src/syntax/header.rscrates/biome_markdown_parser/src/syntax/inline/emphasis.rscrates/biome_markdown_parser/src/syntax/mod.rscrates/biome_markdown_parser/src/to_html.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/biome_markdown_parser/src/lib.rs
- crates/biome_markdown_parser/src/to_html.rs
- crates/biome_markdown_parser/src/inline_phase.rs
- crates/biome_markdown_parser/src/syntax/inline/emphasis.rs
- crates/biome_markdown_parser/src/syntax/mod.rs
- crates/biome_markdown_parser/src/syntax/header.rs
✅ Organic activityNo automation signals detected in the analyzed events. This is an automated analysis by AgentScan |
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
5d57c99 to
8814837
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Summary
DeferredInlineStartwhich makes sure that the deferred strategy isn't miusedTest Plan
Green CI
Docs
N/A