Skip to content

Use type index instead of type name for CompositeData#1019

Merged
azeey merged 6 commits into
gazebosim:mainfrom
luca-della-vedova:luca/type_id
Jul 15, 2026
Merged

Use type index instead of type name for CompositeData#1019
azeey merged 6 commits into
gazebosim:mainfrom
luca-della-vedova:luca/type_id

Conversation

@luca-della-vedova

Copy link
Copy Markdown
Contributor

🎉 New feature

Perf improvement.
CC @cheriehu

Summary

When contacts are enabled, using a std::string for type name causes the construction + destruction of MapOfData to be very expensive because we need to allocate + deallocate a std::string for each entry in the map and, I believe, one map is allocated for each contact data.
This PR changes it to type_index, which, according to cppreference, is meant exactly to be used as a key type for associative containers (which is what we have here).

I think this is not a big breakage since, according to the comment, the type is only public to allow friend classes to use it, not really because it's part of the public API.
It will technically break users that used the public type, as well as users that iterated on it and expected a stable alphabetical order, but I'm not sure how common these use case are.

Backport Policy

Read the above on the technical breakage.

  • This is safe to backport to the following versions:
    • Jetty
    • Ionic
    • Harmonic
    • Fortress
  • This should not be backported
  • I am not sure
  • Other (fill in yourself)

Test it

This starts to be more of a bottleneck for simulations with contacts after the in-flight optimizations landed, specifically:

$ gz sim -v3 3k_shapes.sdf --physics-engine libgz-physics-bullet-featherstone-plugin -s -z 1000000 -r

Checklist

  • Signed all commits for DCO
  • Added a screen capture or video to the PR description that demonstrates the feature
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • Updated Bazel files (if adding new files). Created an issue otherwise.
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers
  • Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Generated-by: Gemini 3

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
This reverts commit c362a62.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
This reverts commit 5610321.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@cheriehu

cheriehu commented Jul 2, 2026

Copy link
Copy Markdown

I tested it with

  1. Performance: don't create new msg::Contacts when adding contacts to ContactSensorData component gz-sim#3714
  2. disabling scopedName in the 3k_shapes.sdf
  3. merged gz-physics main into this PR locally

before this PR

BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_bullet/3000/iterations:1000       3.94 ms         3.93 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_dart/1/iterations:1000            28.7 ms         28.6 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_bullet/3000/iterations:1000       3.79 ms         3.78 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_dart/1/iterations:1000            27.8 ms         27.7 ms         1000

with this PR @ 38ac5aa

BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_bullet/3000/iterations:1000       3.15 ms         3.14 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_dart/1/iterations:1000            26.3 ms         26.3 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_bullet/3000/iterations:1000       3.12 ms         3.11 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_dart/1/iterations:1000            27.1 ms         27.0 ms         1000

There does seem to be an improvement!

side note: I also tested it with 09c06ea before the unordered_map commit was reverted and the results were worse

BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_bullet/3000/iterations:1000       6.22 ms         6.21 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_dart/1/iterations:1000            30.1 ms         30.0 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_bullet/3000/iterations:1000       6.43 ms         6.41 ms         1000
BM_RuntimeWorldContacts/lengthy_sdf_3k_shapes_dart/1/iterations:1000            31.1 ms         31.0 ms         1000

@cheriehu

cheriehu commented Jul 3, 2026

Copy link
Copy Markdown

before this PR
3k_shapes_gzsim3714

after this PR
3k_shapes_gzphysics1019

I think the most obvious change is that the large gz::physics::GetContactsFromLastStepFeature::World<gz::physics::FeaturePolicy<double, 3ul>, gz::sim::v11::systems::PhysicsPrivate::ContactFeatureList>::GetContactsFromLastStep portion in the middle drops from 30.96% to 27.68% and if you search "string" nothing appears anymore

@luca-della-vedova
luca-della-vedova marked this pull request as ready for review July 3, 2026 02:22

@azeey azeey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely an API break since the CompositeData type is meant to be inherited. Any downstream users who inherited from it to create their own types similar to ExpectData will be affected. So, strictly speaking, this should not be backported. However, I highly doubt anyone is doing that. If it was a large performance improvement, I wouldn't hesitate, but it is modest. I'd appreciate a second opinion from @scpeters

@github-project-automation github-project-automation Bot moved this from Inbox to In review in Core development Jul 13, 2026
@scpeters

Copy link
Copy Markdown
Member

This is definitely an API break since the CompositeData type is meant to be inherited. Any downstream users who inherited from it to create their own types similar to ExpectData will be affected. So, strictly speaking, this should not be backported. However, I highly doubt anyone is doing that. If it was a large performance improvement, I wouldn't hesitate, but it is modest. I'd appreciate a second opinion from @scpeters

If this only modified detail headers (like include/gz/physics/detail/CompositeData.hh), then I would be more willing to consider backporting it, but since the MapOfData type is defined in include/gz/physics/CompositeData.hh, I think we should only do it if the performance improvement is significant. Based on @azeey's assessment that this is a "modest" improvement, I vote against backporting

@azeey
azeey enabled auto-merge (squash) July 15, 2026 18:31
@azeey

azeey commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Okay, let's not backport it then.

@azeey
azeey merged commit 62f5c45 into gazebosim:main Jul 15, 2026
13 of 14 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Core development Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants