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 memory leak in ImageDisplay.cc #287

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Sep 19, 2021

Summary

Note: This is a big memory leak in the order of 30 MB/s or more depending on many conditions; thus it should be prioritized. A 32GB RAM machine can run out of memory after approximately 20 min of real time simulation.

I don't like the interface of common::Image outputting a raw pointer that is deleted if the input is non-nullptr but it is up to the caller to the delete if it's not called again. And it's not documented either.
I don't like the amount of unnecessary copies required by ImageDisplay::ProcessImage either. Or the fact that we do one allocation per frame (instead of trying to keep an allocated region while in use).

But all those are problems for another day.

Don't forget to merge this to ign-gui5 and later :)

🦟 Bug fix

Fixes gazebosim/gz-sim#1011

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Fixes gazebosim/gz-sim#1011

Signed-off-by: Matias N. Goldberg <[email protected]>
@codecov
Copy link

codecov bot commented Sep 20, 2021

Codecov Report

Merging #287 (6ed0d91) into ign-gui4 (33edc9e) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 6ed0d91 differs from pull request most recent head 6d7e66d. Consider uploading reports for the commit 6d7e66d to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui4     #287      +/-   ##
============================================
- Coverage     66.39%   66.37%   -0.03%     
============================================
  Files            26       26              
  Lines          3205     3206       +1     
============================================
  Hits           2128     2128              
- Misses         1077     1078       +1     
Impacted Files Coverage Δ
src/plugins/image_display/ImageDisplay.cc 30.08% <0.00%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33edc9e...6d7e66d. Read the comment docs.

@chapulina chapulina added bug Something isn't working performance Runtime performance labels Sep 20, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@chapulina chapulina merged commit 8b00026 into gazebosim:ign-gui4 Sep 20, 2021
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🔮 dome Ignition Dome performance Runtime performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants