Fix Rust font fallback performance - #177
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds a synchronized global cache for font support and selection. Font registration clears the cache. Tests verify repeated probes, selection caching, and cache clearing. ChangesFont support caching
Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant select_font
participant FontSupportCache
participant probe_font_support
select_font->>FontSupportCache: request font for character and style
FontSupportCache->>probe_font_support: probe uncached font support
probe_font_support-->>FontSupportCache: return support result
FontSupportCache-->>select_font: return selected font
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Address global mutex contention, unbounded cache growth, and missing regression coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves Rust font fallback performance by caching font support and selections across conversions.
Changes:
- Adds process-wide support and selection caches.
- Invalidates caches when fonts are registered.
- Adds focused cache behavior tests.
File summaries
| File | Description |
|---|---|
minipdf-rs/crates/minipdf/src/pdf.rs |
Implements font caching and related tests. |
minipdf-rs/crates/minipdf/src/lib.rs |
Triggers cache invalidation during font registration. |
Review details
Suppressed comments (1)
minipdf-rs/crates/minipdf/src/pdf.rs:1384
- This test does not actually prove selection reuse: repeating the same call and checking
selections.len() == 1also passes if the cache lookup is removed and each call recomputes then overwrites the same key. Add an observable probe counter or test seam so the optimization is protected against that regression.
for _ in 0..100 {
assert_eq!(
cache.select_font(&fonts, 'A', false, false, Some("Arial")),
None
);
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+10
to
+12
| struct FontSupportCache { | ||
| support_by_character: HashMap<char, Vec<Option<bool>>>, | ||
| selections: HashMap<FontSelectionKey, Option<usize>>, |
Comment on lines
+805
to
+809
| let cache = FONT_SUPPORT_CACHE.get_or_init(|| Mutex::new(FontSupportCache::default())); | ||
| cache | ||
| .lock() | ||
| .expect("font support cache lock poisoned") | ||
| .select_font(fonts, ch, bold, italic, preferred_font) |
| name: name.into(), | ||
| data: font_data.into(), | ||
| }); | ||
| pdf::clear_font_support_cache(); |
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.
Summary
Performance
Using the existing
nthu_article.docxfixture (2,073 words) with copied Arial fonts:All generated PDFs remained byte-identical to the baseline.
Validation
cargo fmt --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace(112 library tests and 1 CLI test passed)git diff --checkFixes #164
Summary by CodeRabbit
Performance
Bug Fixes