Compile time: share settings registration code across settings - #15454
Merged
Conversation
Contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
zachbai
approved these changes
Aug 22, 2026
Contributor
There was a problem hiding this comment.
Overview
This PR refactors settings registration so the macro emits per-setting metadata/callbacks and shared generic registration code handles the common SettingsManager wiring.
Concerns
- Several added comments/doc comments describe the refactor history (for example, what the macro "used to" do or the "formerly expanded" implementation) rather than documenting the stable current-state rationale. Warp's comment guidance requires comments to avoid transformation phrasing.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…c registration The register_settings_events! macro expanded a full registration function for every setting. Move that body into settings::registration, where it is generic over only the settings group and value types, so settings that share those types share one compiled instantiation. The macro now only builds a SettingCallbacks struct of function pointers at its expansion site, which keeps the method resolution of the old expanded code. Emit change events through the new SettingChangeEvent trait so define_setting! and implement_setting_for_enum! generate one concat_idents! block per setting instead of four. Co-Authored-By: Warp <agent@warp.dev>
vorporeal
force-pushed
the
david/compile-time-settings-macros
branch
from
August 23, 2026 00:33
9ab2588 to
8a94633
Compare
Co-authored-by: warp-for-oss[bot] <277970191+warp-for-oss[bot]@users.noreply.github.com>
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.
Description
This is PR 2 of 3 in a stack that reduces the compile time of the
warpcrate. It is stacked on #15453.The settings macros are the largest single source of macro-expanded code in the
warpcrate. Before this change, they produced 10.7 MB of the 36.7 MB total macro expansion, and the settings registration path produced about 660K lines of LLVM IR.This PR makes two changes:
register_settings_events!macro expanded a full registration function (parse, update, clear, load, equality, and syncable callbacks) for every one of the ~300 registered settings. The body now lives insettings::registration::register_setting_events_impl, which is generic over only the settings group type and the value type. Settings that share those two types (for example, the manyboolsettings in one group) now share one compiled instantiation. A thin per-setting wrapper gathers theSettingtrait metadata (storage key, defaults, platforms, and so on) into a plain struct before it hands off to the shared body.define_setting!andimplement_setting_for_enum!expanded four separateconcat_idents!blocks per setting to construct the group's*ChangedEventvariant at each emit site. Each setting now gets one smallSettingChangeEventimpl, and the emit sites call<Self as SettingChangeEvent>::change_event(reason). This cutsconcat_idents!from 606 uses (2.49 MB) to 351 uses (0.13 MB).How method resolution is preserved
The old macro expansion called setting methods (for example
set_valueandcurrent_value_is_syncable) at the expansion site. This matters:Theme,SystemThemes, and one test type define inherentcurrent_value_is_syncablemethods that shadow theSettingtrait default. To keep this behavior, the macro builds aSettingCallbacksstruct of function pointers at its expansion site. Each callback is a small non-capturing closure with the concrete setting type, so inherent methods still shadow trait defaults exactly as before. The shared generic body only invokes the function pointers.Alternatives considered and discarded
S. It removed the macro bytes but only cut 22K IR lines (−0.13%), because the body still monomorphized once per setting. The (group, value) split in this PR cuts the registration path from 660K to 238K IR lines.Setting. Moving the body into a default method has the same per-setting monomorphization problem, and it changes method resolution for the shadowed inherent methods.Box<dyn Any>. This would compile the body exactly once, but it needs runtime downcasts on every settings update and loses type safety. The (group, value) fn-pointer design gets most of the win with no runtime cost.Measured results (Apple M5 Pro, rustc 1.92.0, dev profile, relative to PR 1)
cargo llvm-lines -p warp --lib: 17,279,212 → 17,022,929 lines (−1.5%); copies 580,747 → 573,201. The settings registration path drops from ~660K to ~238K lines (−64%).-Zmacro-stats): 36.7 MB → 32.2 MB (−12.4% versus master). The 1.99 MBgenerate_settings_event_fn!output is gone.warplib: unchanged (15.4/14.8 s versus 15.4/14.6 s).warplib (deps cached): unchanged within run-to-run noise (71–74 s versus 68 s; master measured ~71 s on the same machine).The wall-clock effect of this PR alone is within measurement noise. The change still removes a large fixed cost from macro expansion and codegen, and it makes each new setting much cheaper to add.
Linked Issue
N/A — compile-time work from a profiling session.
ready-to-specorready-to-implement.Testing
cargo nextest run -p settings: 71/71 passed.cargo nextest run -p warp -E 'test(cloud_preferences) or test(theme) or test(/settings::/)': 168/168 passed.generate_settings_schemaoutput (221 settings) is byte-identical to the output on the base branch../script/formatand all presubmit clippy passes are clean.This is a pure refactor with no user-visible behavior change, so no new tests were added. The schema byte-identity check plus the existing settings test suite cover the behavior that could regress.
./script/runAgent Mode
Warp conversation
Co-Authored-By: Warp agent@warp.dev
CHANGELOG-NONE