Cache localized bundle resolution to cut main-thread disk lookups (#1347) - #1355
Conversation
…eipete#1347) localizedBundle()/codexBarLocalizationResourceBundle()/lprojBundle() did Bundle(url:)/Bundle(path:) filesystem lookups on every call. Menu row bodies re-evaluate them on every closed-menu rebuild tick on the main thread, so idle CPU climbs. Cache the (constant) resource bundle and the resolved localized bundle keyed on the current language; a language switch transparently re-resolves. Single lock with compute-outside-lock keeps disk work off the critical section and avoids re-entrant deadlock. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Codex review: needs maintainer review before merge. Reviewed June 7, 2026, 11:53 PM ET / 03:53 UTC. Summary Reproducibility: yes. at source level: current main still resolves the resource bundle and .lproj bundle during each L() path, and the linked issue discussion includes a sample pointing at localizedBundle() under closed-menu rebuilds. I did not run a live multi-day app reproduction in this read-only review. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused localization-cache fix after CI and maintainer review, then use #1347 for any remaining closed-menu rebuild or formatter hot paths outside this PR. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main still resolves the resource bundle and .lproj bundle during each L() path, and the linked issue discussion includes a sample pointing at localizedBundle() under closed-menu rebuilds. I did not run a live multi-day app reproduction in this read-only review. Is this the best way to solve the issue? Yes, the proposed cache is a narrow maintainable fix for repeated localization filesystem lookups and keeps explicit language switching covered by tests. Packaged-app CPU proof would improve end-to-end confidence but does not point to a different implementation. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against db184430bc4b. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
CI failed because the language-switch assertion used "ja", which has no ja.lproj in the repo and correctly falls back to en.lproj. Use "es" instead, which matches an actual catalog. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Fixes the localization hot path behind the long-uptime idle main-thread spin in #1347 (the part not covered by the
isRunningTestsProcess()cache already on main, as confirmed by @YossiGS'ssamplelanding onrebuildClosedMenuIfNeeded → populateMenu → menu body getters → localizedBundle()).localizedBundle(),codexBarLocalizationResourceBundle()andlprojBundle()didBundle(url:)/Bundle(path:)filesystem lookups on every call. A singleL("…")resolves the resource bundle twice, readsUserDefaults, and does an.lprojdisk lookup — all uncached. Menu row bodies (MetricRow,ProviderCostContent,UsageMenuCardView.Model) callL()/codexBarLocalizationSignature(), and they re-evaluate on every closed-menu rebuild tick on the main thread, so the per-call cost shows up as sustained idle CPU.This PR caches:
.mainresolution is cached, custom-arg calls used by tests stay direct), andlocalizedBundle()keyed on the current language, so a settings/locale language switch transparently re-resolves.A single
NSLockwith the compute happening outside the lock keeps the disk work off the critical section and avoids re-entrant deadlock (the localized-bundle compute calls back into the resource-bundle accessor).Proof
Micro-benchmark of
localizedBundle()resolution, 20k iterations (temporary test, removed before commit):This is a lower bound: in the test harness
Bundle.mainis not a.app, so the resource-bundleBundle.main.url(forResource:"CodexBar_CodexBar")+Bundle(url:)path (run twice perL()pre-fix) isn't even exercised. In the shipped app that disk work is also eliminated, so the real-app savings are larger. Each menu rebuild callsL()many times across providers/rows, multiplying the saved time per closed-menu tick.Test plan
swift buildLocalizationBundleCacheTests(4 cases): correct.lprojper language, re-resolve on switch + switch-back, unknown-language fallback toen.lproj, survives explicit cache reset. Assertions are concurrency-safe (they verify the resolved bundle is always correct for the requested language rather than relying on global cache identity).make check(SwiftFormat + SwiftLint, 0 violations)swift test— only pre-existing, environment-dependent failures remain (PreferencesPaneSmokeTests/MiniMaxMenuCardModelTests), which also fail on cleanorigin/mainon a machine whoseUserDefaults.standard appLanguageis set (verified by reverting this patch); they pass whereappLanguageis unset (CI).Refs #1347