Repository navigation
perf(build): SettingsRuntime reference + Settings pages as a workspace pane (#12737) - #13862
Conversation
… the settings catalog Every @LiveSetting holds @Environment(\.settingsRuntime). SettingsRuntime was a struct embedding the whole SettingCatalog (~24 KB), so each view with a live setting stored the catalog inline. Release codegen for ContentView then spent ~490s in LLVM register allocation on 103k-instruction value witnesses and 58 closure-context destructors (~413 KB each), 32 MB of machine code per arch. The runtime's members are all immutable, so a final class keeps semantics and shrinks each environment slot to one pointer.
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthrough
ChangesSettingsRuntime
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The runtime representation changes to reduce copying overhead, with no actionable merge risk identified in the reviewed change. 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Independent review of 0c2608a: immutable stored fields and existing actor/service references preserve the runtime dependency behavior; initialization remains MainActor-isolated, with no mutable value or Equatable contract removed. A second independent agent reached the same conclusion. Exact-head native Swift package and compile admission checks pass, all required checks are green, and CodeRabbit reported no actionable findings. Verified the cited universal profiling run completed successfully. Landing the measured compile reduction; the full production nightly improvement still needs observation after merge. BasaltUnwind g1 🗝️ |
|
this is green we review a bit then let's get stuff merged asap |
Nightly Release compile of the
cmuxmodule drops from 935 s to 487 s per architecture. The nightly build step should go from about 20 min to about 12.5 min.Cause.
SettingsRuntimewas a struct that embedded the wholeSettingCatalog(about 24 KB of key declarations). Every@LiveSettingholds@Environment(\.settingsRuntime), so each view with a live setting stored the catalog inline.ContentViewhas five of them. Its fields are resilient SwiftUI wrappers, so IRGen expands a field-by-field destroy wherever aContentViewis copied or destroyed. Each expansion moved five 24 KB catalogs through registers: a 103k-instruction function with a 50 KB stack frame. There were 58 closure-context destructors of 413 KB each, plus 1.2 MB value witnesses, which made 32 MB of machine code per architecture inContentView.o. LLVM's greedy register allocator took about 490 s on that file. With WMO, LLVM codegen runs on one thread for each file, so this single file set the length of the whole module's LLVM stage.Fix.
SettingsRuntimeis now afinal class. All of its members are immutablelets (actors, a@MainActorlog, an existential and the catalog), so reference semantics do not change behavior. Each environment slot is now one pointer.cmuxApp,UpdateTitlebarAccessory,CanvasHostedPanelContentViewandSettingsWindowScene, which store the runtime by value, shrink the same way.Measured on
warp-macos-26-arm64-12x, the nightly build runner class, with cold caches. The runs used temporary experiment workflows on branches that were not merged.ContentView.swiftcompile, per-file arm64ContentView.ocmuxWMO frontend, each arch (universal)Runs: before 35800014258 (profile) and 35806087265 (object symbol sizes). After: 35807874614.
The remaining WMO cost is SIL optimization at 280 s. It runs on one thread per architecture, and only splitting the module or dropping whole-module mode reduces it.
swift testinPackages/macOS/CmuxSettingsUIpasses (177 tests).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Turns
SettingsRuntimeinto an immutable reference type so views no longer store the full settings catalog inline. This halves the nightlycmuxcompile time per architecture (935 s to 487 s) and reducesContentView's object file from 45 MB to 12.9 MB.All existing members were already immutable, so semantics are unchanged and
swift testpasses.Written for commit 0c2608a. Summary will update on new commits.
Summary by CodeRabbit
Also includes #12737 (author lawrencecchen), merged here on request. It gives each Settings category its own page, adds Feature Flags search, and opens Settings as a workspace (Bonsplit) pane, with the window as a fallback. Conflicts with current main were resolved as follows:
.computerssection: kept in the new per-page switch.AutomationSection: main's managed socket-policy state is kept. The socket password draft moves topageDrafts, as in 12737.Localizable.xcstrings: merged per key withscripts/merge-xcstrings.py. No key had changes on both sides.includeEmptyLocalWorkspacesoption is added to them. Row selection keeps main'sisSelectablerule. The empty state usesincludeLocalMachine: trueto match the outline.