Optimize render updates and use of thread mutexes in Sensors system

[iche033]

Marked as draft to debug test failures Fixed

🦟 Bug fix

Related to: gazebosim/gz-sensors#332

Summary

While profiling the sensors system, I noticed that it spends a quite bit of time

• waiting in the Sensors::Update function for rendering operation to finish and release the renderMutex. This is not necessary since the RenderUtil calls inside the Update function should be thread-safe (protected by mutex inside RenderUtil).
• doing scene tree updates (scene->PreRender(), scene->PostRender, renderUtil.Update()) when there are no sensor or render event subscribers

This PR optimizes the threading and render updates in the sensors system by:

• removing reducing the scope of the renderMutex lock in Sensors::Update function
  * use a separate mutex for the render condition variable to reduce places that need to lock the renderMutex
• changing a few bool variables to atomic, also to reduce the need to lock mutexes
• performing scene tree updates only when necessary

Test results

Here are the RTF changes that I got from running different worlds (i7 3.4GHz 32GB RAM, NVIDIA GeForce GTX 1070 8GB VRAM). I added a best fit line to the RTF values. There is an improvement in RTF and noticeably less jitter.

• sensors_demo.sdf world (with lidar visualization on)

  • Before
    

  • After
    

• profile_lidar.sdf world (5 lidars inside Depot model. All lidars are subscribed)

  • Before
    

  • After
    

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.

[mjcarroll]

Overall approach looks good, I'm going to allocate a little more time to review this afternoon as this is pretty tricky logic.

[mjcarroll]

Finally got to think about this one enough, and I'm happy with the approach. Thanks for tackling this tricky bit of logic!

[codecov]

Codecov Report

Merging #1938 (73c4ad7) into gz-sim7 (3dc8c7a) will decrease coverage by 0.18%.
The diff coverage is 66.66%.

❗ Current head 73c4ad7 differs from pull request most recent head 220df1f. Consider uploading reports for the commit 220df1f to get more accurate results

@@             Coverage Diff             @@
##           gz-sim7    #1938      +/-   ##
===========================================
- Coverage    64.68%   64.51%   -0.18%     
===========================================
  Files          343      347       +4     
  Lines        27430    27872     +442     
===========================================
+ Hits         17743    17981     +238     
- Misses        9687     9891     +204     

Impacted Files	Coverage Δ
include/gz/sim/ServerConfig.hh	100.00% <ø> (ø)
include/gz/sim/Util.hh	100.00% <ø> (ø)
include/gz/sim/detail/EntityComponentManager.hh	93.86% <ø> (ø)
src/gui/plugins/spawn/Spawn.cc	9.88% <0.00%> (-0.40%)	⬇️
...rc/systems/ackermann_steering/AckermannSteering.hh	100.00% <ø> (ø)
src/systems/joint_controller/JointController.hh	100.00% <ø> (ø)
...int_position_controller/JointPositionController.hh	100.00% <ø> (ø)
src/systems/magnetometer/Magnetometer.cc	51.77% <10.86%> (-20.86%)	⬇️
src/gui/plugins/component_inspector/Inertial.cc	11.76% <11.76%> (ø)
src/SdfGenerator.cc	90.33% <40.00%> (-0.62%)	⬇️
... and 20 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ahcorde]

@iche033 do you have any plans to backport this to fortress?

[iche033]

@iche033 do you have any plans to backport this to fortress?

yes I'll backport this to fortress