perf(fclrave): visit only changed bodies in Synchronize and EnsureBodies - #1571
Open
ziyan wants to merge 5 commits into
Open
perf(fclrave): visit only changed bodies in Synchronize and EnsureBodies#1571ziyan wants to merge 5 commits into
ziyan wants to merge 5 commits into
Conversation
FCLCollisionChecker::_GetEnvManager calls FCLCollisionManagerInstance:: Synchronize on every environment-level CheckCollision, and that walked every cached body doing two weak_ptr locks each, whether or not anything had moved. The cost was proportional to the size of the environment rather than to the amount of change, so a scene where nothing moved cost about as much as one where everything did. FCLSpace now keeps a revision log: _MarkBodyChanged records a body against a monotonic counter, and CollectBodyIndicesChangedAfter returns the bodies recorded since a caller's last sync. Each manager keeps a cursor and revisits only those bodies. The mark is set inside FCLSpace::_Synchronize, where an info is actually refreshed, rather than at the point of mutation. Several core paths bump the body update stamp without going through _PostprocessChangedParameters, so no change callback fires for them; marking at the point of refresh covers those, and FCLSpace::Synchronize stays a full scan for the same reason. In a benchmark of 190 environment-level checks per cycle over 200 bodies, the per-check cost drops from about 6.5us to about 2us, and now scales with the number of bodies that moved instead of being flat.
Profiling a fleet controller showed CollectBodyIndicesChangedAfter at 5.2% of the scheduling thread, almost all of it std::_Rb_tree_increment. The log holds one entry per body, so a manager whose cursor lags walks most of the tree, and chasing red-black tree nodes to do it costs more than the walk it replaced. Store the entries in a vector sorted by revision instead. Revisions only increase, so marking a body appends, and "changed after" binary searches once and then walks contiguous memory. A body's earlier entries are left in place and skipped on read rather than erased from the middle, with a compaction pass once the superseded entries outnumber the live ones.
ziyan
marked this pull request as ready for review
August 11, 2026 18:16
…dex reuse - mark the body before mutating its info everywhere, so a throw between the mark and the mutation cannot leave a changed body unrecorded - advance the manager's revision cursor only after the whole Synchronize succeeded, including the attached-bodies block and the bulk register; advancing earlier dropped every body after a throwing one from all future calls - clear FCLKinBodyInfo::nEnvBodyIndex in Reset and restore it before ReloadKinBodyLinks can throw; the active-DOF and attached-bodies callbacks survive Reset and kept marking the stale index - tear down RemoveUserData and DestroyEnvironment state at the index the body was initialized under, falling back to a scan of _vecInitializedBodies when the body already lost its index, so the mark and the reset cannot target different slots - retry attached bodies that were skipped because the space had not initialized them yet; nAttachedBodiesUpdateStamp was already consumed, so nothing else would revisit them
…fresh A non-null vcolobjs entry means the object is registered in the broadphase manager. The active-DOF refresh stored the link BV even when the link went inactive, i.e. right after unregistering it, so the next refresh fed an object the manager does not hold to unregisterObject (or, with FCLRAVE_USE_REPLACEOBJECT, to replaceObject, which updates a missing tree entry). Store the BV only on the path that registered it and reset the entry otherwise.
EnsureBodies walked every environment body on each environment-level CheckCollision, and in steady state nearly every cache entry is already valid. Collect the indices recorded in the FCLSpace revision log instead, with a cursor separate from Synchronize's so both consumers see the same marks. Recovery for a body the space stopped tracking (the null-info branch in Synchronize) is preserved: re-initializing the body in the space records a new mark in InitKinBody, which this cursor picks up, so EnsureBodies still re-adds the body. Every other path that fills a body's info marks it as well, so consuming a mark on the not-initialized skip is safe. A valid cache entry holding a different body than the one now at that index used to be skipped until the entry expired; that skip now consumes the mark, so drop the stale entry and add the new body immediately. Benchmark (fclsyncbench, 200 bodies, best of 3): 1.4 -> 0.85 us/check with few moved bodies, 1.5 -> 1.0 us/check with all bodies moved.
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
FCLCollisionChecker::_GetEnvManagercallsFCLCollisionManagerInstance::EnsureBodiesandSynchronizeon every environment-levelCheckCollision, and both walked every cached body regardless of whether anything moved. The cost tracked environment size rather than amount of change: in a 200-body benchmark, moving nothing cost 6.9us per check and moving all 200 cost 6.5us.Change
FCLSpacekeeps a change log._MarkBodyChangedrecords a body against a monotonic revision counter,CollectBodyIndicesChangedAfterreturns the bodies recorded since a caller's last sync, and each manager keeps a cursor so it revisits only those.The mark is set inside
FCLSpace::_Synchronize, where an info is actually refreshed, rather than at the point of mutation.KinBody::Link::SetTransform,KinBody::IncrementUpdateStampandGetNonAdjacentLinksbump the body update stamp without going through_PostprocessChangedParameters, so no change callback fires for them. Marking at the point of refresh covers those, andFCLSpace::Synchronizedeliberately stays a full scan for the same reason -- its per-body cost is one integer compare.Commit 2 stores the log in a vector sorted by revision rather than a
std::map. Revisions only increase, so marking appends and a query binary searches once then walks contiguous memory; superseded entries are skipped on read and dropped by a compaction pass.Commit 3 hardens the bookkeeping: marks are recorded before the mutation they describe, the manager cursor advances only after a whole
Synchronizesucceeded, teardown inRemoveUserData/DestroyEnvironmentis indexed off the slot the body was initialized under, and attached bodies skipped because the space had not initialized them yet are retried.Commit 4 fixes the active-DOF refresh caching a link BV it had just unregistered; the next refresh then unregistered (or
replaceObjectd) an object the broadphase manager does not hold.Commit 5 applies the change log to
EnsureBodieswith its own cursor, separate fromSynchronize's so both consumers see the same marks. Recovery for a body the space stopped tracking is preserved: re-initializing it records a new mark inInitKinBody, which the cursor picks up. A valid cache entry holding a different body than the one now at that index is dropped and re-added instead of skipped.Results
Benchmark, 190 checks/cycle over 200 bodies: ~6.5us -> ~1.1us per check (~2us after commits 1-2, ~1.1us after commit 5), and the cost now scales with how many bodies moved instead of being flat.
Commit 5 measured on its own, building the tree at commit 4 and at commit 5 and running the same benchmark:
Those are the minimum of several runs per configuration. The benchmark host is shared and noisy: individual runs of the same build vary by up to 2x, and the first run after a build is consistently slow from cold caches, so single numbers are not meaningful. The minimum is the honest estimator here because contention only ever adds time. What carries the conclusion is the floor rather than any single row: after commit 5 every row reached 1.06-1.20us in its best run, while before it never went below 1.58us on any row across three runs.
Profiled on a production fleet controller, share of the scheduling thread:
FCLCollisionManagerInstance::SynchronizeCollectBodyIndicesChangedAfterEnsureBodieswas a further 8.2% of the same thread before commit 5.Testing
Re-run on the final tree unless noted:
SetDOFValues,Link::SetTransform, grab/release, active-DOF tracking, geometry group switches, link and body enabling, and adding and removing bodies mid-run. Each iteration answers the same query with the long-lived cached checker and with a checker created that instant; any disagreement is a stale cache. 17,598 comparisons, zero mismatches, and zero FCLunregisterObjectwarnings (commit 4 removes the 12 an earlier revision produced). An earlier revision that marked via aProp_LinkTransformscallback fails it with 22 mismatches, so the check is sensitive to exactly this class of bug.test/test_collision.pyfcl suite: unchanged before and after.