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

Unload render engine when the sensors system exits #1960

Merged
merged 10 commits into from
May 1, 2023

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Apr 7, 2023

🦟 Bug fix

Fixes #1410

Depends on:

Summary

The render engine is never destroyed / unloaded when the server shuts down. This leads to issues such as memory leak and not being able to relaunch the server with the sensors system, e.g. as seen in #1410, rendering tests n gz-sim can not be run one after another in the same process.

This PR adds a function in RenderUtil to destroy the scene and unload the engine.

To test:

Run the PERFORMANCE_sensors_system test that repeatedly launches the server with the sensors system.

Note that to run the tests, you'll need the above PRs in gz-rendering and gz-sensors otherwise gz will crash.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@iche033 iche033 requested a review from mjcarroll as a code owner April 7, 2023 01:07
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Apr 7, 2023
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
@iche033 iche033 added the needs upstream release Blocked by a release of an upstream library label Apr 7, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #1960 (e26e577) into gz-sim7 (0742968) will decrease coverage by 0.07%.
The diff coverage is 77.30%.

@@             Coverage Diff             @@
##           gz-sim7    #1960      +/-   ##
===========================================
- Coverage    65.08%   65.02%   -0.07%     
===========================================
  Files          352      352              
  Lines        28312    28393      +81     
===========================================
+ Hits         18428    18463      +35     
- Misses        9884     9930      +46     
Impacted Files Coverage Δ
include/gz/sim/rendering/MarkerManager.hh 100.00% <ø> (ø)
include/gz/sim/rendering/RenderUtil.hh 100.00% <ø> (ø)
include/gz/sim/rendering/SceneManager.hh 100.00% <ø> (ø)
src/SimulationRunner.hh 100.00% <ø> (ø)
...rc/systems/ackermann_steering/AckermannSteering.hh 100.00% <ø> (ø)
include/gz/sim/EventManager.hh 75.75% <66.66%> (-3.41%) ⬇️
src/systems/sensors/Sensors.cc 62.74% <70.93%> (-0.27%) ⬇️
...rc/systems/ackermann_steering/AckermannSteering.cc 80.09% <73.52%> (-0.45%) ⬇️
src/SimulationRunner.cc 91.63% <100.00%> (-0.27%) ⬇️
src/rendering/MarkerManager.cc 14.81% <100.00%> (+1.78%) ⬆️
... and 2 more

... and 1 file with indirect coverage changes

Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
@iche033 iche033 removed the needs upstream release Blocked by a release of an upstream library label Apr 21, 2023
@mjcarroll
Copy link
Contributor

I think this is still failing PERFORMANCE_sensors_system on the buildfarm, so it may need another look.

Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor Author

iche033 commented Apr 28, 2023

I think this is still failing PERFORMANCE_sensors_system on the buildfarm, so it may need another look.

removed bad ifdef logic in e26e577, which should hopefully fix it.

@iche033 iche033 changed the title Unload render engine when the sensors system exists Unload render engine when the sensors system exits Apr 28, 2023
@iche033
Copy link
Contributor Author

iche033 commented Apr 28, 2023

ubuntu build fixed

@mjcarroll mjcarroll merged commit 13aedfd into gz-sim7 May 1, 2023
@mjcarroll mjcarroll deleted the sensors_system_destroy branch May 1, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Rendering engines are not unloaded or cleaned up when Server is destroyed
2 participants