Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the performance of EntityComponentManager::Each #711

Closed
adlarkin opened this issue Mar 26, 2021 · 2 comments
Closed

Improve the performance of EntityComponentManager::Each #711

adlarkin opened this issue Mar 26, 2021 · 2 comments
Assignees
Labels
performance Runtime performance

Comments

@adlarkin
Copy link
Contributor

adlarkin commented Mar 26, 2021

Desired behavior

As discussed in #678 (comment), it seems like adding more components to an ecm.Each call reduces performance. Considering how often we call this method in ign-gazebo, it would be nice to optimize ecm.Each so that performance is the same regardless of the number of components used.

Alternatives considered

TBD

Implementation suggestion

See #711 (comment)

Additional context

Could be related to #429

@adlarkin
Copy link
Contributor Author

adlarkin commented Apr 9, 2021

After some further investigation, I believe that the views are responsible for most of the performance in ecm.Each. The issue is that the views aren't really working as a true "cache". Once we have the view, we have to find the components in the view that correspond to the current entity, every single time we want to use an entity (view.Component<ComponentTypeTs>(entity, this)...): https://github.com/ignitionrobotics/ign-gazebo/blob/ignition-gazebo5_5.0.0/include/ignition/gazebo/detail/EntityComponentManager.hh#L386-L394

In reality, we shouldn't have to do this, because the view should already have these components cached for every entity that has them. I think it could be good to change how components are saved in a view. Currently, we store components in a view using a std::map, with a std::pair being the key of this map: https://github.com/ignitionrobotics/ign-gazebo/blob/ignition-gazebo3_3.8.0/include/ignition/gazebo/detail/View.hh#L129-L131

I tried to put a band-aid on the drawbacks of using a std::map with a key of std::pair in #752, but in reality, I believe that a better solution would be to store something like a table of component data in a view. This table would have the columns be the components, and the rows be entities - then, when it's time to use this view with an entity, the entity would simply index the proper row of data in the view's table, instead of having to find each entity's component individually.

If we were to take this approach, I believe that it would be crucial to create the following mappings (these mappings need to be fast):

A part of this proposed change/enhancement may involve changing how we store component IDs. Right now, we scope component IDs relative to their type (I'm not sure if this is an issue or not; I just wanted to point this out as something that we may need to consider when making changes): https://github.com/ignitionrobotics/ign-gazebo/blob/ignition-gazebo5_5.0.0/include/ignition/gazebo/Types.hh#L78-L86

@chapulina
Copy link
Contributor

Done in #856 🎉

Core development automation moved this from In progress to Done Aug 26, 2021
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Runtime performance
Projects
None yet
Development

No branches or pull requests

2 participants