Skip to content

fix memory leak#1554

Open
Andrea-Havron-NOAA wants to merge 2 commits into
mainfrom
fix-memory-leak
Open

fix memory leak#1554
Andrea-Havron-NOAA wants to merge 2 commits into
mainfrom
fix-memory-leak

Conversation

@Andrea-Havron-NOAA

@Andrea-Havron-NOAA Andrea-Havron-NOAA commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

What is the feature?

  • Fixes memory leak in R caused by not clearing pointers

How have you implemented the solution?

  • add recruitment shared pointers to clear function
  • add functionality in FIMSObject to track objects for memory leak test and identify source module for memory leak
  • add memory leak test

Does the PR impact any other area of the project, maybe another repo?

  • No

I was able to track down which pointers were not being cleared using a new memory test that adds module registration for memory tracking purposes. The clear function now will throw an error if there is an uncleared pointer and will list the base class and module id. For example, I used the following warning to track down the uncleared pointers were coming from RecruitmentBase:

⚠️ WARNING: FIMS Memory Leak Detected after clear()!

The following 4 C++ heap object(s) were NOT destroyed:
❌ Leaked: fims_popdy::RecruitmentBaseTMBad::global::ad_aug [id=0]
❌ Leaked: fims_popdy::RecruitmentBaseTMBad::global::ad_aug [id=1]
❌ Leaked: fims_popdy::RecruitmentBase [id=0]
❌ Leaked: fims_popdy::RecruitmentBase [id=1]

Ensure all std::shared_ptrs to these types are being reset.


Instructions for code reviewer

👋Hello reviewer👋, thank you for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete but feel free to skip over items that are not relevant!
  • See the GitHub documentation for how to comment on a PR to indicate where you have questions or changes are needed before approving the PR.
  • Please use standard conventional messages for both commit messages and comments
  • PR reviews are a great way to learn so feel free to share your tips and tricks. However, when suggesting changes to the PR that are optional please include nit: (for nitpicking) as the comment type. For example, nit: I prefer using a data.frame() instead of a matrix because ...
  • Engage with the developer. Make it clear when the PR is approved by selecting the approved status, and potentially commenting on the PR with something like This PR is now ready to be merged.

Checklist

  • The code is well-designed
  • The code is designed well for both users and developers
  • Code coverage remains high- [ ] Comments are clear, useful, and explain why instead of what
  • Code is appropriately documented (doxygen and roxygen)

- add recruitment shared pointers to clear function
- add functionality in FIMSObject to track objects for memory leak test and identify source module for memory leak
- add memory leak test
@Andrea-Havron-NOAA Andrea-Havron-NOAA linked an issue Jun 18, 2026 that may be closed by this pull request
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.08108% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.48%. Comparing base (3e9b7ab) to head (3cb21cb).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
inst/include/interface/rcpp/rcpp_interface.hpp 8.33% 11 Missing ⚠️
inst/include/common/information.hpp 91.66% 2 Missing ⚠️
inst/include/common/model_object.hpp 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1554      +/-   ##
==========================================
+ Coverage   83.52%   87.48%   +3.96%     
==========================================
  Files          55       98      +43     
  Lines        2197     8813    +6616     
  Branches      516      540      +24     
==========================================
+ Hits         1835     7710    +5875     
- Misses        292     1065     +773     
+ Partials       70       38      -32     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Andrea-Havron-NOAA

Copy link
Copy Markdown
Collaborator Author

I noticed in the build metrics reports: sampled process tree RSS peak increased by 22.3%. @msupernaw, would this increase be due to my include statemens like #include <cxxabi.h>? About half of the code added in this PR is to help produce readable error messages to help track down the location of the pointer that is not being cleared. But thinking about it more, I'm not sure all this extra code is neccessary. It was useful to find the right modules in the case where I had no idea where to look, but moving forward, this error will only be thrown when new pointers are added that haven't been cleared. It will be obvious to the developer which pointers to check. Should I strip the PR of the extra code to produce readable error messages and see if the build metrics improve?

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor

@Andrea-Havron-NOAA the build metric has been highly variable for that one, with very small changes leading to 25% differences so I think it is just the instability of the runner. We need to change the threshold for that metric as Matthew pointed out to me before. But, I am curious to hear what @msupernaw thinks about the code to help debug that is in this PR.

@msupernaw

Copy link
Copy Markdown
Contributor

@Andrea-Havron-NOAA and @kellijohnson-NOAA That metric is vulnerable to all kinds of influence like system state. It only becomes informative when peak RSS increases, which is reported directly from the kernel. I would not expect peak RSS to move much unless we create a TU larger than TMB.o.

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.

Fix memory leak from not clearing pointers

3 participants