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

Fix sensors system parallel updates #2201

Merged
merged 5 commits into from
Oct 16, 2023
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Oct 11, 2023

🦟 Bug fix

depends on #2200

Summary

While profiling the Sensors system, I noticed that the render updates are no longer happening in parallel but instead it's blocking the main thread.

The remotery graph below illustrate the issue. The SimulationRunner is being blocked for the entire duration of the sensors system's update

sensors_system_remotery_before

With changes in this PR, the SimulationRunner will now continue to step while sensor updates take place:

sensors_system_remotery_after

More info:

It came down to this lock that blocks the PostUpdate thread longer than it should. The fix is to use a new mutex (renderUtilMutex) that's specific RenderUtil updates.

After reducing the scope of the lock, I noticed a race condition that sometimes the RenderUtil's states are overriden (in the PostUpdate thread) before the changes are applied to the scene (in the rendering thread). An updateTimeCv condition variable is added to make sure that does not happen.

To test:

Run the INTEGRATION_sensors_system_update_rate test multiple times to verify the sensors' timestamps are correct.

On my desktop machine, the RTF values (displayed in the GUI) for the sensors_demo.sdf world are:

  • Before: 77-78%
  • After: 95-98%

Here is another test world consisting of 2 1980x1280 cameras @ 10Hz.

The raw RTF values over 2000 iterations are plotted in the graphs below.

Before:

RTF dips on every camera update

sensors_demo_parallel_before

After:

There is still jitter but less than before

sensors_demo_parallel_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.

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #2201 (c90f053) into gz-sim7 (8a80800) will increase coverage by 0.02%.
Report is 1 commits behind head on gz-sim7.
The diff coverage is 88.00%.

❗ Current head c90f053 differs from pull request most recent head 68e4278. Consider uploading reports for the commit 68e4278 to get more accurate results

@@             Coverage Diff             @@
##           gz-sim7    #2201      +/-   ##
===========================================
+ Coverage    64.75%   64.77%   +0.02%     
===========================================
  Files          357      357              
  Lines        29134    29144      +10     
===========================================
+ Hits         18865    18878      +13     
+ Misses       10269    10266       -3     
Files Coverage Δ
src/systems/sensors/Sensors.cc 63.14% <88.00%> (+0.46%) ⬆️

... and 2 files with indirect coverage changes

Base automatically changed from backport_sensors_deadlock to gz-sim7 October 11, 2023 17:18
Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor Author

iche033 commented Oct 11, 2023

For some reason, github is stuck processing updates from my last push. Going to close and reopen the PR.
github_processing_updates

@iche033 iche033 closed this Oct 11, 2023
@iche033 iche033 reopened this Oct 11, 2023
@iche033 iche033 requested a review from mjcarroll as a code owner October 12, 2023 23:17
@iche033 iche033 merged commit 5d1c505 into gz-sim7 Oct 16, 2023
4 checks passed
@iche033 iche033 deleted the sensors_parallel_rendering branch October 16, 2023 21:08
@iche033 iche033 mentioned this pull request Oct 16, 2023
8 tasks
@iche033 iche033 added the 🌱 garden Ignition Garden label Nov 2, 2023
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.

2 participants