Use unordered maps for component look-ups in views

[adlarkin]

Signed-off-by: Ashton Larkin ashton@openrobotics.org

Summary

Related to #711

Currently, when calling EntityComponentManager::Each, the following methods get called:

1. EntityComponentManager::Each
2. View::Component
3. View::ComponentImplementation
4. EntityComponentManager::ComponentImplementation
5. ComponentStorageBase::Component

There are a few std::map lookups in this process:

• https://github.com/ignitionrobotics/ign-gazebo/blob/ignition-gazebo3_3.8.0/src/View.cc#L68
• https://github.com/ignitionrobotics/ign-gazebo/blob/ignition-gazebo3_3.8.0/include/ignition/gazebo/detail/ComponentStorageBase.hh#L191

Since lookup time in a std::map is O(log(n)), it's better to use std::unordered_map which has a lookup time of O(1) (assuming no collisions). This PR makes these changes, with the hope of improving the performance of ecm.Each.

Testing

I tested these changes applied to ign-gazebo at commit a41cf80. I used the testing methodology that was followed in #678 (take a look at that PR for more details about how to run the tests). The results are as follows (the model ecm.Each call I am referring to in the test results is a part of the UpdateSim in the physics system that can be found here):

3000 non-static simple shapes

Using std::map: 15.75% RTF, .803ms for the model ecm.Each call

Using std::unordered_map: 16.5% RTF, .306ms for the model ecm.Each call

3000 static simple shapes

Using std::map: 50% RTF, .754ms for the model ecm.Each call

Using std::unordered_map: 70% RTF, .289ms for the model ecm.Each call

Takeaways

After running some tests, it appears that the performance gains from this change are minimal, and in some cases, not noticeable at all. This change seems to help the use case where ecm.Each is being used on a large(r) set of components that match a lot of entities.

We could also change EntityComponentManagerPrivate::views to an unordered map, but this would require some more work/thought because the key of this map is actually a std::set. I don't think we should be using a std::set as the key of a map, but this seems to be necessary due to some limitations in our current implementation of views (this leads me to my next point that we need to fix the current design of views - see the next paragraph).

The real reason why this change does not help the performance of ecm.Each much is because of how views are currently being used. See #711 (comment) for more information - I plan to address what is mentioned in this comment next.

Also, on a related note: I believe that changing https://github.com/ignitionrobotics/ign-gazebo/blob/ignition-gazebo3_3.8.0/include/ignition/gazebo/detail/View.hh#L37-L38 to a std::unordered_set may help a bit with performance, but I don't believe this can be done in an existing release because there are methods in the ECM (like FindView) that use this std::set.

Edit:
I'd like to give one other perspective as to why the changes here don't result in much of an improvement. If we look at the callback loop in ecm.Each again, the time complexity can be calculated as:

O(E * (C * L))

The variables are defined as follows:

• E: the number of entities (or number of for loops)
• C: the number of components
• L: the lookup time for a component in a view
• C * L: the time complexity for view.Component<ComponentTypeTs>(entity, this)...

Before this PR, L was O(log(n)) since std::maps were used. After this PR, L is now O(1). It's worth analyzing the time complexity for two cases:

• We have a lot of entities and few components in an ecm.Each call. The complexity is now:

O(E * (C * L)) = O(E * (C * 1)) = O(C * E)

Before this PR, the time complexity for this case would be O(C * E * log(E)).

• The number of components is large (i.e., approaching the number of entities), the time complexity is now:

O(E * (E * 1)) = O(E^2)

Before this PR, the time complexity for this case would be O(E^2 * log(E)).

As we can see in this analysis, the real issue is that we still have to spend time finding each component in the view (as mentioned in #711 (comment)) - the time complexity for the ecm.Each callback loop should really be O(E) (all components should be cached at the time of view creation, which would mean that C * L is no longer part of the equation for time complexity). If anyone believes that my analysis here may be incorrect, feel free to mention what I am missing, and I will update this accordingly.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

[codecov]

Codecov Report

Merging #752 (292fb9c) into main (2b3879f) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 292fb9c differs from pull request most recent head 22adcce. Consider uploading reports for the commit 22adcce to get more accurate results

@@            Coverage Diff             @@
##             main     #752      +/-   ##
==========================================
- Coverage   65.34%   65.32%   -0.03%     
==========================================
  Files         240      240              
  Lines       17602    17609       +7     
==========================================
+ Hits        11502    11503       +1     
- Misses       6100     6106       +6     

Impacted Files	Coverage Δ
...ude/ignition/gazebo/detail/ComponentStorageBase.hh	80.00% <ø> (ø)
include/ignition/gazebo/detail/View.hh	100.00% <100.00%> (ø)
src/SimulationRunner.cc	92.73% <0.00%> (-1.04%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b3879f...22adcce. Read the comment docs.

[adlarkin]

This hash function was motivated by https://stackoverflow.com/a/27952689, but I'm wondering if there's a better hash function that we can use. The main concern I have with the current function is that we are implicitly converting uint64_t (_pair.first) to std::size_t. Any thoughts?

[adlarkin]

When running tests locally, I noticed that the Unit_Gui_TEST segfaults on shutdown. It appears to be related to the unordered map that I introduced in View (if I revert this back to a std::map, the segfault disappears). I'm not sure why there's a crash on shutdown though, because I did some debugging and verified that all checks in the GUI unit test pass. Any thoughts? Perhaps it's something on my machine if this test failure doesn't occur in GitHub actions/CI.

[adlarkin]

ABI checker is failing - I'm not sure if this is a false positive or not, since the files I am modifying are in the include/.../detail directory.

[iche033]

hmm I know we don't promise ABI/API compatibility for the headers in detail directory and users shouldn't be using the functions in these classes but I wonder if this would cause any undefined behavior for users linking to our libraries? cc @j-rivero

[iche033]

After running some tests, it appears that the performance gains from this change are minimal, and in some cases, not noticeable at all. This change seems to help the use case where ecm.Each is being used on a large(r) set of components that match a lot of entities.

Do you have some rough numbers from profiling? I'm mainly gauging to see if it's worth risking breaking ABI to get these performance improvements.

[adlarkin]

Do you have some rough numbers from profiling? I'm mainly gauging to see if it's worth risking breaking ABI to get these performance improvements.

I tested 3000 non-static simple shapes, calling ecm.Each on the models with 5 components as a part of this call. The profiling time for the ecm.Each call before this PR was 4.2ms, and after this PR was 2.9ms. So again, not that much - just a little more than 1ms gain for a world with A LOT of shapes. For worlds with fewer shapes, the improvements will be less noticeable I believe.

I should also mention that those numbers are running headless (physics system plugin only in my server config) with TPE.

hmm I know we don't promise ABI/API compatibility for the headers in detail directory and users shouldn't be using the functions in these classes but I wonder if this would cause any undefined behavior for users linking to our libraries?

I'm not sure about this either. I originally thought that these changes would have to target fortress, but @chapulina believes that they could land in citadel for the reasons you stated (not promising ABI/API for headers in the detail directory). We can wait to see what @j-rivero says about the linking.

[j-rivero]

We can wait to see what @j-rivero says about the linking.

Umm, downloaded the latest version of ignition-gazebo5 and run:

include/ignition/gazebo on  main via 喝 v3.10.2 ❯ nm -DC /usr/lib/x86_64-linux-gnu/libignition-gazebo5.so.5 | grep View::AddComponent
00000000001e42f0 T ignition::gazebo::v5::detail::View::AddComponent(unsigned long, unsigned long, int)

Somehow the binary symbol for the detail class is there. Might be wrong but looks like an valid ABI breakage to me.

[iche033]

Somehow the binary symbol for the detail class is there. Might be wrong but looks like an valid ABI breakage to me.

hmm sounds like we won't be able to land this in a released version and should probably also ticket an issue about hiding symbols of classes in detail.

the perf improvement from 4.2s to 2.9s is actually good in my opinion. We could target this to F or wait till you find a better solution with caching data in Views.

[adlarkin]

ticket an issue about hiding symbols of classes in detail

Done: #759

We could target this to F or wait till you find a better solution with caching data in Views

I went ahead and re-targeted this PR to Fortress, and also added some actual performance numbers/comparisons in the PR description. Based on the numbers I'm seeing in my tests/comparisons, I think that this change could be worthwhile for worlds with a lot of entities. If you wouldn't mind reading the updated PR description and letting me know what you think, that would be great!

[adlarkin]

This PR is probably no longer needed since #856 is now open. Once #856 is merged, we can close this.

[adlarkin]

I'm closing this since #856 has been merged.