-
Notifications
You must be signed in to change notification settings - Fork 268
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
Use statically-typed views for better performance #856
Conversation
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a few questions/notes for my reviewers.
Do you notice that when launching I am testing with 2 workspaces, one on this branch and the other on new ECM (this branch):
original ECM (
So looks like it made a difference on my machine. Could you try comparing against fortress built from source and see if you also see these improvements? |
I never ran this with the GUI the first time around, but now that you have made the point, I went ahead and tested it myself. Yes, the scene loads a lot faster for me now with these changes. I'm also seeing loading times decrease from a few minutes to a few seconds. Thanks for pointing this out!
I went ahead an did a comparison with source builds and have updated the PR description with my results. I do notice some improvements now, especially when echoing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few minor optimization comments in code.
I'm also randomly testing examples worlds. So far I ran into one crash which I'm not able to reproduce on the main
branch. To reproduce:
ign gazebo -v 4 -r --levels levels.sdf
Right click and remove the red vehicle -> segfault
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
I've addressed all review feedback, except for updating the migration guide with things that had to be deleted or deprecated (I'll do this once we know no other changes have to be made).
I tested this locally as well, and also noticed a segfault. I will debug this and update the PR once I've found a fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a first pass. There's a lot of changes, so please allow me a few passes until I get the hang of it.
I have some suggestions to allow us to tick-tock most of the API and ease migration in #873.
Meanwhile, can you add some unit tests for the new classes / files? i.e. BaseView
, ComponentStorage
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
…t test Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
74c5d80
to
ee3d3d0
Compare
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
I ran performance tests again and did not see any noticeable changes in the numbers. You can see the results in the PR description.
I think at this point, there are 2 lingering CI issues:
|
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
I'm blindly trying @j-rivero's approach from gazebosim/gz-gui@5f7f05c in 66bb2a2 It's worth noting that it fails when compiling the
Nope, the same happens on GitHub actions, so it's definitely an issue with the PR. Very strange that it fails to create the |
That's strange - I mentioned in a previous comment that tests started failing in https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/6727/, which was a build for #943. But, I don't see how changes in #943 would cause a break. Another thing to note is that these tests are not failing on MacOS CI 🤔 |
…tuff Signed-off-by: Louise Poubel <louise@openrobotics.org>
I have a promising fix for |
Ohhh you know what, this has actually happened to me before. I just forgot about it and always ran |
Codecov Report
@@ Coverage Diff @@
## main #856 +/- ##
==========================================
+ Coverage 64.66% 65.04% +0.38%
==========================================
Files 242 245 +3
Lines 19014 19413 +399
==========================================
+ Hits 12296 12628 +332
- Misses 6718 6785 +67
Continue to review full report at Codecov.
|
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this branch with various worlds, spawning and deleting objects, distributed sim, custom plugins and it seems to be working quite well.
the latest ubuntu build failed due to network issue. I just queued another one.
That's a lot of failing tests on Ubuntu. The sensors ones should be fixed by #617 , but that's only 9 tests and there are 23 failing here |
Signed-off-by: Louise Poubel <louise@openrobotics.org>
I'm seeing 23 failing tests on Ubuntu Jenkins for multiple PRs. I believe this is the fallout from gazebo-tooling/release-tools#494. Let's rely on the other platforms to merge this PR. GitHub actions was happy, as well as homebrew and Windows. I've fixed conflicts and will merge if CI comes back happy for those platforms again. |
Signed-off-by: Ashton Larkin ashton@openrobotics.org
🎉 New feature
Closes #711
Summary
In an ongoing attempt to improve runtime performance, #711 points out that the runtime performance of
EntityComponentManager::Each
gets worse as more components are used. I've changed how views are implemented to improve the runtime ofEntityComponentManager::Each
. The main change here is that we now have a statically-typed view, which is defined by the list of component types that are required by the view. By doing this, a view can store an entity and pointers to the required component types in a std::tuple, and then make use of std::apply when we want to apply an entity and all of its relevant components to a callback inEntityComponentManager::Each
.Using
std::apply
allows for better runtime performance because we can apply the entity and associated components to a callback "all at once". This is quicker than the previous view approach, which involved individually looking up each component for an entity (see #711 (comment)).The main drawback with the approach of a statically-typed view is the added software complexity/maintenance to the system. For example, during testing, I found that tuple creation can be costly - so, in order to avoid re-creating tuples whenever a component was being added/removed, I added an internal
ignore
flag to components to eliminate tuple re-creation. Thisignore
component flag is set totrue
whenever a user requests to remove an entity that was previously created, and is set tofalse
whenever a user requests to add an entity that was previously removed. Theignore
flag is hidden from the user and is used by the ECM and its views to determine which entities and components should be used in a particularEach
call.Another thing to note is that the old ComponentStorage approach is no longer used. This new view approach does not require components of the same type to be packed sequentially in memory, and through various testing that I performed, I found that runtime performance with statically-typed views without components stored sequentially in memory is still noticeably better than the runtime performance of the old views with components stored sequentially in memory. So, hopefully this makes at least the
ComponentStorage
part of our system simpler to maintain 🙂A side effect of removing the old
ComponentStorage
approach is that we also no longer need ComponentId, which means that we also no longer need ComponentKey. I tried to deprecate methods usingComponentKey
wherever possible in order to follow the tick-tock model, but there were several methods that had to be deleted completely and replaced with something else (i.e., methods that returnedComponentKey
or hadComponentKey
as the only method parameter, which meant that there was no longer enough information provided to achieve the desired functionality of the method).Test it
If you'd like to compare the performance of running
Each
with the approach proposed in this PR to the existing view approach, you can run the each benchmark. I have already done this on my system, and have included the results below (I ran a comparison against 8f5103b):Each benchmark test
As seen from the results above, all of the
EachCache
tests are significantly faster with the statically-typed view approach - in fact, if we look at the very last test in that file (Each10ComponentCache/1000
- so, 1000 entities with 10 components each), we go from 150ms to 2ms 🤯 It's also worth noting that we don't see a decrease inEachNoCache
performance either, which makes sense since views are not used forEachNoCache
.Another way to test the changes here is to run a world with 3k shapes. Here are the RTF numbers I'm seeing from testing 3k simple shapes, using both GUI and headless simulation. I used TPE for the physics engine. I also echoed the
/world/shapes/dynamic_pose/info
topic to see how RTF was impacted using these changes (see #743).original ECM (
main
at ca10c77):/world/shapes/dynamic_pose/info
topic: 8.3%/world/shapes/dynamic_pose/info
topic: 4%/world/shapes/dynamic_pose/info
topic: 3.5%/world/shapes/dynamic_pose/info
topic: 2.3%new ECM (this branch):
/world/shapes/dynamic_pose/info
topic: 35% (this is a huge increase compared to 8.3% 🤯)/world/shapes/dynamic_pose/info
topic: 23% (again, a huge increase compared to 4% 🤯)/world/shapes/dynamic_pose/info
topic: 5%/world/shapes/dynamic_pose/info
topic: 3.75%As the numbers show, there's a performance improvement for almost every single test case (there's not much of an improvement for non-static, no GUI). For use cases where plugins are used that make calls to
Each
with a large number of components, I suspect that there will be even greater performance improvements.It was also pointed out in #856 (comment) that GUI startup time for 3k shapes with the changes in this PR is only a few seconds (it used to take up to a few minutes). Perhaps this is because of some
each
calls that are used when initializing the GUI?Another thing to note is that when using the changes in this PR to run 3k shape simulations with a GUI, loading time is a lot faster. In the tests I ran, I found that these new changes allowed the GUI to load and run in a few seconds, while the old ECM/View implementation took a few minutes to load (thanks @iche033 for making this discovery!).
One other scenario that could be worth testing is a benchmark of the time it takes to add/remove components frequently. I did some informal testing locally with the new approach and found that removing components with the statically-typed views and an internal component
ignore
flag is faster than the current ECM/view approach, but perhaps it would be worth adding a benchmark test for this. This PR is already very large, so I decided to hold off on adding a benchmark test for now, but if anyone would be interested in me adding this in, let me know - I can also make another PR that adds this benchmark test if that is more preferable 🙂Takeaways/Next steps
While it's clear that this PR has improved the runtime performance of
EntityComponentManager::Each
, there's not much of an improvement in RTF for the headless tests. While #793 should help improve the RTF for running the GUI, we will need to figure out why we still aren't seeing higher RTF for headless simulation (theoretically, headless RTF for 3k static shapes should be 100%).I would also be interested in figuring out how to generate better testing scenarios/evaluation metrics. Perhaps 3k simple shapes isn't a great way to benchmark performance - we aren't running any plugins other than the physics system, and aren't doing things like adding/removing components. Maybe an external user will try these changes in a context that differs from the 3k simple shapes example and find a nice performance improvement! Considering that most users use various plugins and may or may not add/remove components frequently, I think it'd be worth taking some time to figure out how to benchmark use cases that are frequent among Ignition users. That way, we can get a better feel for whether our changes are being noticed by the community or not.
Also, it would be nice to add some more features/functionality to the
EntityComponentManager
that make use of the new statically-typed views. For example, perhaps we can use iterators (as done in EnTT) for use cases where a lot of components are used as "filters", and we only need specific components of entities that pass all component filter checks. So, if a user wanted all entities that are static models, but only need the pose of these models, then maybe they could do something like this:Related to the idea of providing functionality like iterators as described above, we will eventually need to address #628 and #805.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge