Remove hash map allocation on every update step (backport #1005)#1013
Merged
Conversation
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai> (cherry picked from commit 84c9ba1) # Conflicts: # bullet-featherstone/src/Base.hh # dartsim/src/Base.hh # dartsim/src/SimulationFeatures.hh
Contributor
Author
|
Cherry-pick of 84c9ba1 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
19 tasks
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
luca-della-vedova
left a comment
Contributor
There was a problem hiding this comment.
Fixed conflicts on this (plus all others), would appreciate another pair of eyes!
azeey
approved these changes
Jul 1, 2026
azeey
enabled auto-merge (squash)
July 1, 2026 21:12
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.
🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸
🎉 New feature
Part of gazebosim/gz-sim#3608
Improve performance by removing allocation + deallocation of a hash map for all link poses in each update step.
Summary
Guided by the profiling report by @caguero, Priority 2.
Building and destroying a hash map, where each node needs to call malloc / free, is an expensive operation.
I tried the quick recommendation to reserve the hash map but found out it didn't help much and I thought the "best" approach of not sending updates for static links might be a bit tricky (and still, it is not mutually exclusive, we can still do it in a followup).
Thus the approach is to remove the extra map and make sure we use the preexisting
prevLinkPoseshash map instead.Before there was an
By caching the pose through iterations, we can get rid of the else statement (since it will be just set to the value in the previous iteration).
Finding out which links have been removed is a bit trickier. I thought since removing links is something that happens fairly rarely, we could do a fast check where, if at any point in time the size of our cache is larger than the size of our links map, we trigger a full traversal and delete stale links.
I believe this should be robust even to cases where we add and remove a link in the same iteration. Adding a link with a new ID will make the cache allocate a new element and thus it will become larger than the links array, triggering the reallocation.
At the same time, this approach will do virtually no work at all iterations, instead of having to do expensive lookups / traversal at every step for, again, something I believe happens very rarely (link deletion).
Backport Policy
Test it
Run gz-sim with and without this PR:
On my machine I actually don't observe a noticeable improvement under
dartsimsince it's already very slow (~1% RTF, I'm open to revert the commit that makes this change there 1a78c62), but on bullet-featherstone the RTF goes from ~10-20% to 20-30%, so about 75% increase.Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.Backports: If this is a backport, please use Rebase and Merge instead.
This is an automatic backport of pull request #1005 done by Mergify.