refactor(featureset): thread &'static FeatureSet instead of Arc - #652
refactor(featureset): thread &'static FeatureSet instead of Arc#652varex83agent wants to merge 3 commits into
Conversation
FeatureSet is resolved exactly once (after the cluster lock's fork version feeds the gnosis hotfix) and never mutated, so it lives for the whole process. Leak it once at the resolution point and thread a Copy &'static FeatureSet everywhere it previously flowed as Arc<FeatureSet>: - node/mod.rs leaks once via Box::leak; drops all Arc::clones threading it into the consensus controller, p2p behaviour, and core workflow. - Fields on ConsensusController::Config, qbft::Consensus (+ its Config), TrackerService, InclusionCore, InclusionChecker, WireInputs, and WireP2PParams lose the Arc. - get_round_timer_func and the three round-timer with_duty constructors take &'static; their new()/Default no longer allocate a throwaway FeatureSet::new() — the field is now Option<&'static FeatureSet> (None until bound to a duty, which is the only reader). - Collapses the second &FeatureSet convention in tracker/analysis.rs and infosync onto &'static, so there is a single convention. - tracker_feature_set (wire.rs) leaks its own small masked static instead of deep-cloning into a second Arc. - Rewrites the featureset crate module doc to mandate &'static. - Tests construct and leak their own set per test (Box::leak), equally cheap. Closes #616 Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
|
Could you please fix the conflicts and merge main? |
|
# Conflicts: # crates/core/src/tracker/inclusion.rs
rustdoc rejects intra-doc links to the private report_missed and report_att_inclusion functions from the public InclusionCore::new docs under -D warnings. Use plain code spans instead. Co-Authored-By: Bohdan Ohorodnii <varex83@users.noreply.github.com>
emlautarom1
left a comment
There was a problem hiding this comment.
LGTM, no semantic changes. Before merging check if we can remove explicit 'static annotations (run your agent against the diff and make it check).
| fn default_feature_set() -> &'static FeatureSet { | ||
| Box::leak(Box::new(FeatureSet::new())) |
There was a problem hiding this comment.
In general, the're are multiple places where we can drop the specific 'static lifetime annotation. This is one case, and by dropping it we can remove the need for this function altogether. Check for all signatures with 'static and see if we can drop it.
| feature_set: Arc<FeatureSet>, | ||
| /// `None` until bound to a duty; the round-one proposal-timeout override is | ||
| /// the only thing that reads it, and it never fires without a duty. | ||
| feature_set: Option<&'static FeatureSet>, |
There was a problem hiding this comment.
We could try to merge this field with the duty since they're both either None or Some at the same time. Not major, but removes the need for comments and an implicit dependency.
Closes #616
FeatureSetis resolved exactly once — after the cluster lock's fork version feeds the gnosis hotfix — and never mutated, so it lives for the whole process. This replaces theArc<FeatureSet>threading with aCopy&'static FeatureSetobtained by leaking once at the resolution point.Changes
node/mod.rs):let feature_set: &'static FeatureSet = Box::leak(Box::new(resolve_feature_set(...)?));. All threeArc::clones threading it into the consensus controller, p2p behaviour, and core workflow are dropped (&'staticisCopy).Arc:ConsensusController::Config,qbft::Consensus+ itsConfig,TrackerService,InclusionCore,InclusionChecker,WireInputs,WireP2PParams.get_round_timer_funcand the threewith_dutyconstructors take&'static FeatureSet. Thenew()/Defaultpaths no longer allocate a throwawayFeatureSet::new()— the field is nowOption<&'static FeatureSet>(Noneuntil bound to a duty, which is the only reader via the round-one proposal-timeout override).&FeatureSetconvention intracker/analysis.rs(9 signatures) andinfosyncis collapsed onto&'static FeatureSet.tracker_feature_set(wire.rs) leaks its own small masked static instead of deep-cloning the whole set into a secondArc.featuresetcrate module doc now mandates the&'staticconvention (leak once at startup).Box::leak(Box::new(...))), equally cheap per the issue.Acceptance
Arc<FeatureSet>in the workspace:FeatureSet::new()throwaway allocations for filling fields (timers useOption<&'static>=None).&'static FeatureSet).#[allow(clippy::too_many_arguments)]onTracker::startis retained: even withoutfeature_set,starthas 8 parameters, so clippy still requires it (clippy-D warningspasses with it in place).Verification (all pass)
cargo build --workspace --all-featurescargo +nightly fmt --allcargo clippy --workspace --all-targets --all-features -- -D warningscargo testfor touched crates (pluto-featureset,pluto-consensus,pluto-core,pluto-infosync,pluto-app) — all green, including thewiring.rsTier-1 tests.🤖 Generated with Claude Code