perf(system): eliminate two Planetesimal clones in planetesimals_inte…#21
Merged
Merged
Conversation
…rsect
The Roche-limit computation in planetesimals_intersect cloned both `p`
and `prev_p` purely to satisfy the borrow checker — only three scalar
fields (mass, mass, radius) are read from the clones. Planetesimal
carries Vec<Planetesimal> moons + Vec<Ring> rings, so each clone is a
deep tree copy. In hot post-accretion loops this is per-iteration waste.
Replace the two clones with direct scalar extraction:
let (larger_mass, smaller_mass, smaller_radius) = if p.mass >= prev_p.mass {
(p.mass, prev_p.mass, prev_p.radius)
} else {
(prev_p.mass, p.mass, p.radius)
};
Note the third argument: the original code passed `&smaller.radius`
(the smaller body's radius, matching roche_limit_au's documented
contract that the third arg is the moon/smaller body's radius). The
refactor preserves this — using smaller_radius rather than the larger
body's radius keeps the physics identical.
For the capture_moon call site, use std::mem::swap with a `>=` tiebreak
to orient the bodies in place: after the swap, prev_p holds the larger
body. This matches the original `match p.mass >= prev_p.mass { ... }`
ordering exactly (p wins on equal mass).
All 16 fixture tests remain bit-identical after the refactor — zero
fixture drift under cargo test, as the clone-elimination itself does
not change the arithmetic order.
# Conflicts: # src/structs/system.rs
The merge of master (post PR 1 + PR 2) changed events_log from &mut AccreteEvents to Option<&mut AccreteEvents>. The capture_moon call site at planetesimals_intersect was missed during conflict resolution and moved the Option instead of reborrowing it, leaving the subsequent coalesce_planetesimals and moons_to_rings calls trying to use a moved value. Add .as_deref_mut() to match the surrounding calls.
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.
Problem
planetesimals_intersectclones bothpandprev_ppurely to satisfy the borrow checker. Only three scalar fields (mass, mass, radius) are ever read from the clones, to compute the Roche limit.PlanetesimalcarriesVec<Planetesimal>moons plusVec<Ring>rings, so each clone is a deep tree copy of every captured satellite. In the post-accretion bombardment loop these clones happen per-iteration and they consumed once and dropped.This was caught while profiling a ~2.56M-system Rayon batch on a 12-core machine. The simulation completed correctly, but the moon-tree clones were a measurable share of the per-system cost.
Change
Replace the two clones in
planetesimals_intersectwith direct scalar extraction:Note the third argument: the original code passed
&smaller.radius, matchingroche_limit_au's documented contract that the third arg is the moon (smaller) body's radius. The refactor preserves this. Usingsmaller_radiuskeeps the physics bit-identical.For the
capture_mooncall site (which previously passed&larger, &smaller), usestd::mem::swapwith a>=tiebreak to orient the bodies in place: after the swap,prev_pholds the larger body. This matches the originalmatch p.mass >= prev_p.mass { ... }ordering exactly (pwins on equal mass).A new reproducibility test (
same_seed_same_system_after_fix_3a) is added to lock in that two successive runs at a fixed seed produce identicalDebug-formatted output.Impact