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

Use spin() in component_manager_isolated.hpp #1881

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

alsora
Copy link
Collaborator

@alsora alsora commented Feb 1, 2022

Replace spin_until_future_complete with spin in component_manager_isolated.hpp.

spin_until_future_complete() is more inefficient as it needs to check the state of the future and the timeout after every work iteration.

This PR addresses the race-condition issues mentioned in #1781 by using the recently added is_spinning() API

Tests on the navigation stack show 10% less CPU usage when using spin() rather than spin_until_future_complete()

FYI @SteveMacenski @gezp @ivanpauno

…lated.hpp

spin_until_future_complete() is more inefficient as it needs to check the state of the future and the timeout after every work iteration

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@fujitatomoya
Copy link
Collaborator

@alsora

Tests on the navigation stack show 10% less CPU usage when using spin() rather than spin_until_future_complete()

sounds great, would you mind sharing the platform you benchmarked this?

@alsora
Copy link
Collaborator Author

alsora commented Feb 1, 2022

@fujitatomoya the tests have been carried out by @gezp, he can definitely provide you more details but I think the 10% less cpu was on a raspberry pi 4, while on x86 the improvement was ~5%

@fujitatomoya
Copy link
Collaborator

Rasp4 is one of the common benchmark platform we have too, this is good information. thanks!

@ivanpauno
Copy link
Member

LGTM with green CI

@alsora
Copy link
Collaborator Author

alsora commented Feb 4, 2022

CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@alsora
Copy link
Collaborator Author

alsora commented Feb 5, 2022

Hi, can someone give a look at the windows CI above? It says that there are more than 70 test failures.

They all seem unrelated tests to what I'm doing here, but they are really too many to ignore.

Looking at the logs it may also seem an infrastructure issue, with some tests unable to start.
Do you have any insight?

@ivanpauno
Copy link
Member

@ivanpauno
Copy link
Member

Ignoring the windows pytest issues that also happen on nightlies, going in!

@ivanpauno ivanpauno merged commit 43db06d into master Feb 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the asoragna/spin-component-isolated branch February 9, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants