perf(text): resolve each font slot once per run - #235
Conversation
|
All contributors have signed the CLA — thank you! ✍️ ✅ Posted by the CLA bot. |
Greptile SummaryThis PR introduces a per-measurement font-chain memo keyed by normalized family and style, then routes paragraph, marker, tab, field, and text-run lookup through it.
Confidence Score: 4/5The PR appears safe to merge, with only non-blocking cleanup needed for overly verbose comments and docstrings. The memo uses the same key normalization and lookup semantics as the existing implementation, remains scoped to one immutable store/input pair, and preserves validation and error behavior across the changed measurement paths. Files Needing Attention: crates/ooxml-text/src/measure/chain_memo.rs, crates/ooxml-text/src/measure/mod.rs
|
| Filename | Overview |
|---|---|
| crates/ooxml-text/src/measure/chain_memo.rs | Adds bounded, per-call chain and validation memoization with focused behavior tests; explanatory documentation exceeds the repository’s concise-comment convention. |
| crates/ooxml-text/src/measure/mod.rs | Creates one memo per paragraph measurement and passes it consistently through empty-paragraph, marker, and run preparation paths. |
| crates/ooxml-text/src/measure/prepare.rs | Replaces direct font-chain resolution and validation with behavior-equivalent memo calls across text, tab, field, and fallback-metrics paths. |
| crates/ooxml-text/src/measure/list_marker.rs | Reuses the per-call memo for list-marker font-chain validation without changing marker measurement semantics. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
M[measure_paragraph] --> C[Create per-call ChainMemo]
C --> K[Normalize family and style key]
K --> H{Memo hit?}
H -->|Yes| R[Reuse resolved chain]
H -->|No| I[Resolve from MeasureInput fontChains]
I --> B{Below 128 entries?}
B -->|Yes| S[Store chain result]
B -->|No| R
S --> R
R --> V{Validation verdict cached?}
V -->|Yes| U[Reuse verdict]
V -->|No| F[Validate IDs against FontStore]
F --> U
U --> P[Prepare marker and runs]
Reviews (1): Last reviewed commit: "perf(text): memoize font chain resolutio..." | Re-trigger Greptile
| //! Per-call memoization of font-chain resolution. | ||
| //! | ||
| //! A [`ChainMemo`] is created once per [`super::measure_paragraph`] call and | ||
| //! never escapes it: one store, one immutable `fontChains` map, one lifetime. | ||
| //! A cached chain or validation verdict therefore can never outlive the state | ||
| //! it was resolved against. | ||
|
|
There was a problem hiding this comment.
Trim redundant memo documentation
The module documentation and related inline comments repeatedly explain lifetime and bounded-cache behavior already expressed by the implementation, increasing maintenance and documentation-drift costs. Keep the module summary concise and remove similarly redundant explanations at the memo declaration and integration point.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Measurement resolved and revalidated a run's font fallback chain for every character, even though `w:rFonts` fixes the family and style per slot: the family is a function of the slot, and bold/italic only vary by whether the slot is complex-script. Resolve the four slots at most once each instead. Measured against the parent (release, minimum of six interleaved rounds): a single-family body paragraph 110.3µs -> 65.2µs, a 40-run rich paragraph 440.0µs -> 139.7µs, mixed Latin/CJK/Arabic 176.9µs -> 107.3µs, and a 4096-character run 9.69ms -> 4.59ms. Adds slot-routing coverage, which the suite had none of: every character class must measure through its own slot's family, and `w:hint="eastAsia"` must move only the ambiguous ones. Co-Authored-By: codex <codex@openai.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
34753b3 to
41b2499
Compare
TL;DR:
Summary:
w:rFontsfixes the family per slot and bold/italic only vary by whether the slot is complex-script, so(family, bold, italic)is a function of the slot within a runw:hint="eastAsia"must move only the ambiguous ones, and both hold when the chain is rebuilt per character rather than keptTest plan:
cargo test --workspacegreen (85 suites, 0 failures)cargo clippy --workspace --all-targets --all-features -- -D warningsandcargo fmt --all -- --checkcleanw:hint="eastAsia", complex-script style overrides, combining marks, list markers, small caps, bidi and every error path