perf: FaceId-keyed FontManager caches -> FxHashMap (issue #459 item 1) - #460
Merged
Conversation
Switches fallback_metrics_cache, face_cache, and plan_cache from the default SipHash-based HashMap to FxHashMap, matching the pattern already used by atlas.rs's glyph cache. FaceId is a small Copy enum (bare discriminants plus one System(usize) variant); none of the three maps' keys are influenced by untrusted input, so this is a safe, mechanical hasher swap. Deliberately left unchanged: glyph_cache and system_fallback_cache, both keyed (in whole or part) by char, which is derived from PTY output. Note the existing atlas.rs procedural_entries map is already char-keyed FxHashMap, so the codebase's HashDoS posture on char keys isn't actually uniform today -- this change doesn't attempt to reconcile that, it only avoids introducing a *new* untrusted-input case beyond what already exists. plan_cache's PlanKey also carries Script/Direction derived from guess_segment_properties() over terminal-run text -- these are input-influenced, not free of it, but their realizable domain is a small, fixed, enumerable set of ISO-15924 script tags plus a 5-variant direction enum, so the conclusion (safe for FxHash) holds even though the input-derivation itself is real. Validation: - freminal-buffer/benches shaping_ligatures (before/after --save-baseline): no statistically significant change -- expected, its per-iteration call frequency to face_cache/plan_cache is much lower than the sustained multi-row churn that surfaced this cost. - Real-workload before/after flamegraph (btop full-screen redraw, same perf methodology as issue #459): sip::Hasher::write and hash_one::<&FaceId>, ~9.00% + ~2.92% self-time in the original capture (two of the hottest leaves), are absent from the fixed capture with no percent-limit filter; only a 0.04% residual attributable to the deliberately-untouched glyph_cache remains. See #459 for the full profiling investigation this follows up on.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFontManager’s internal fallback metrics, face, and shaping plan caches now use ChangesFont cache map optimization
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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
Switches three
FontManagercaches keyed purely or partly byFaceIdfrom Rust's default SipHash-basedHashMaptoFxHashMap, matching the pattern already established byatlas.rs's glyph cache. This is follow-up item 1 from #459's real-workload profiling investigation.fallback_metrics_cache: RefCell<HashMap<FaceId, Option<FallbackCellMetrics>>>->RefCell<FxHashMap<...>>face_cache: RefCell<HashMap<FaceId, Option<CachedFace>>>->RefCell<FxHashMap<...>>plan_cache: RefCell<HashMap<PlanKey, Arc<rustybuzz::ShapePlan>>>->RefCell<FxHashMap<...>>(PlanKey = (FaceId, bool, rustybuzz::Script, rustybuzz::Direction))Why this is safe
FaceIdis a smallCopyenum (bare discriminants plus oneSystem(usize)variant assigned locally during font discovery) — never derived from untrusted input.PlanKey's other components (bool,Script,Direction) are technically derived from terminal-run text viaguess_segment_properties(), so they are input-influenced, but their realizable domain is a small, fixed, enumerable set (ISO-15924 script tags + a 5-variant direction enum), so a HashDoS concern doesn't apply in practice.Deliberately left unchanged:
glyph_cache((char, GlyphStyle)key) andsystem_fallback_cache(charkey) — both keyed bychar, which comes directly from PTY output (untrusted input). Switching those raises a HashDoS-resistance trade-off that hasn't been explicitly decided and is out of scope here. (Note:atlas.rs'sprocedural_entriesmap is alreadychar-keyedFxHashMap, so the codebase's posture on this isn't actually uniform today — this PR doesn't attempt to reconcile that, only avoids introducing a new case beyond what already exists.)Validation
cargo test --all,cargo clippy --all-targets --all-features -- -D warnings,cargo machete— all green.shaping_ligaturesbenchmark (before/after via--save-baseline): no statistically significant change detected. Expected — that benchmark's call frequency toface_cache/plan_cacheper iteration is much lower than the sustained multi-row-per-frame churn that originally surfaced this cost in real-workload profiling.btopfull-screen redraw, sameperf --call-graph dwarf,65528methodology as Investigation: real-workload CPU profiling findings (idle + active TUI) — follow-up to #405, pivot from #457 #459):sip::Hasher::writeandhash_one::<&FaceId>— two of the hottest leaves in the original capture at ~9.00% + ~2.92% self-time — are absent from the fixed capture with no percent-limit filter; only a 0.04% residual attributable to the deliberately-untouchedglyph_cacheremains. Sample counts are low (~250-400/capture) and the two captures are different windows ofbtop's own dynamic content, so a precise percentage delta isn't defensible, but the categorical disappearance of a top-2 hot symbol is a credible confirmation.Adversarially reviewed for completeness (no missed construction sites or ad-hoc maps), correctness of the untouched exclusions, API/iteration-order safety, and style consistency against
atlas.rs— no blocking findings.Closes item 1 of #459.
Summary by CodeRabbit