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

Fixed memory leaks by calling clearOrphan() on removed XML nodes. #3592

Closed
wants to merge 0 commits into from

Conversation

fcanderson
Copy link
Contributor

@fcanderson fcanderson commented Oct 30, 2023

XML-related memory leaks were reported when loading OpenSim models. Specifically, deserializing Rajagopal2015.osim causes numerous memory leaks. See issue #3537.

I used WinCRT to help find the leaks. Since WinCRT is platform specific (i.e., runs on Windows) and requires some changes to the source code to leak test effectively, this PR only includes the fixes (not the modified code that I used to identify the leaks and verify the fixes).

None of the observed leaks associated with deserializing Rajagopal2015.osim were due to coding errors in SimTK::Xml. SimTK::Xml looks like it is working fine. (@adamkewley and I did find a constructor in SimTK::Xml that should be revised, but it is not currently the source of leaks that I can identity).

The reported memory leaks were all occurring in OpenSim in various updateFromXMLNode() calls. In particular, updateFromXMLNode() for OpenSim::Body, OpenSim::Coordinate, OpenSim::Muscle, OpenSim::PhysicalFrame, and OpenSim::WrapObject.

All of the leaks were in code related to converting from an older version of OpenSim (i.e., from v3.0) to the current version. Thus, the memory leaks can be sidestepped by converting old models to the current version (by deserializing them) and then saving to a new .osim model file.

This sort of sidestepping doesn't address a situation in which a old version model is repeatedly deserialized, however.

Fortunately, the leaks were not too difficult to find. In each case, an element was removed from the XML document and was not inserted elsewhere in the model hierarchy. In all these cases, the fix was simply to call clearOrphan() on the element that was removed.

Rajagopal2015.osim, whether represented in v3.0 or in the current version, is now deserializing without memory leaks.

Fixes issue #3537

Brief summary of fixes

In the files listed below, I inserted a call to clearOrphan(). To make these calls easier to find, amidst all the trailing whitespaces that were removed, I've included the line numbers of the inserted calls.

  1. SimbodyEngine/Body.cpp : line 397
  2. SimbodyEngine/Coordinate.cpp : line 705
  3. Model/Muscle.cpp : lines 110, 121
  4. Model/PhysicalFrame.cpp : line 190
  5. Wrap/WrapObject.cpp : lines 218, 238, 260

Testing I've completed

  • In Simbody, I mimicked the operations that are commonly performed in OpenSim during deserialization and verified that no leaks occur in SimTK::Xml.
  • For a few simple models (those that I am using to test my exponential contact model), I verified that no leaks occur during deserialization.
  • For the new StatesDocument class that I've been developing, which is XML based, I verified that no leaks occur when a complete states trajectory is de/serialized.
  • For Rajagopal2015.osim (v3.0), I verified that deserialization generated leaks before the above fixes and that no leaks were generated after.
  • For Rajagopal2015.osim, I verified that serialization of the model to a new file produced identical output before and after the above fixes.

CHANGELOG.md (choose one)

  • updated

This change is Reviewable

@aymanhab
Copy link
Member

@fcanderson Excellent job, definitely exciting news. Can you take a look at the build failures if related? Thanks much 👍

@fcanderson
Copy link
Contributor Author

@aymanhab Yes! I will follow up on the build errors.

@fcanderson
Copy link
Contributor Author

@aymanhab The build errors are due to some unresolved function-based path methods.
A few updates to opensim-org:main have landed during the continuous-integration builds of this PR. Perhaps those updates will fix the build errors I'm seeing. I'm now seeing conflicts that prevent a synch however. I will try to merge opensim-org:main into this PR tomorrow. Worst case, I can cancel this PR and resubmit it once opensim-org:main has stabilized.

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.

2 participants