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 with Coordinate::_lockFunction. #510

Merged
merged 5 commits into from
Jul 14, 2015

Conversation

chrisdembia
Copy link
Member

I added a comment to the code to describe why I made this change:

This line was originally
in extendFinalizeFromProperties(), but this caused a memory leak since
extendFinalizeFromProperties() is called once when the model is
constructed from a file, and again when one calls Model::initSystem()
(or whenever finalizing from properties multiple times without recreating
the SimTK System).

By putting the allocation right next to the creation of the related PrescribedMotion, we should be able to avoid the leak.

I detected the leak in the process of trying to get testInitState to pass on OSX. I detected the leak with valgrind and also used valgrind to verify that the leak is now gone.

@chrisdembia
Copy link
Member Author

@sherm1 says:

A nicer way to do this I think would be to use an std::unique_ptr to hold the freshly-allocated Function object until its ownership can be transferred to the PrescribedMotion constraint. It would look something like this:

std::unique_ptr<Function> func(new Function()); // func has ownership
ReferencePtr<Function> funcref(func.get()); // keep a reference to Function object
Constraint::PrescribedMotion(func.release()); // take over ownership from func

@chrisdembia
Copy link
Member Author

To reviewers: please don't merge this until the above change is made. Also, feel free to make the above change for me!

Made reference pointer const rather than mutable, like the indices.
@sherm1
Copy link
Member

sherm1 commented Jul 14, 2015

Also, feel free to make the above change for me!

OK, I did. Please review.

@sherm1
Copy link
Member

sherm1 commented Jul 14, 2015

Hmm. AppVeyor build died immediately; related to this morning's AppVeyor update? Chris, please see separate email.

@sherm1
Copy link
Member

sherm1 commented Jul 14, 2015

Added a commit to allow "deprecated-declaration" warnings to occur without error until we fix issue #528.

@sherm1
Copy link
Member

sherm1 commented Jul 14, 2015

I'm merging this now because it fixes the CI builds.

sherm1 added a commit that referenced this pull request Jul 14, 2015
Fix memory leak with Coordinate::_lockFunction.
@sherm1 sherm1 merged commit ce10cb4 into master Jul 14, 2015
@sherm1 sherm1 deleted the coordinate-lock-function-leak branch July 14, 2015 21:16
@chrisdembia
Copy link
Member Author

Okay; @aseth1 definitely should look at these changes at some point.

@chrisdembia
Copy link
Member Author

Thanks for doing this @sherm1 .

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