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 ResetKernel #2520

Merged
merged 3 commits into from
Nov 10, 2022
Merged

Conversation

hakonsbm
Copy link
Contributor

@hakonsbm hakonsbm commented Nov 2, 2022

Nodes are created with new in GenericModel:

GenericModel< ElementT >::create_()
{
Node* n = new ElementT( proto_ );
return n;
}

Then the node pointers are stored in memory_ in Model. However the nodes are never deleted, which leads to a memory leak when calling ResetKernel. With only a few thousand nodes, calling ResetKernel multiple times can add up to hundreds of MB of leaked memory.

This PR puts Node pointers in smart pointers within memory_, so that the Nodes are automatically deleted when the model is destroyed. The explicit call to the node destructor in NodeManager is removed because it interferes with the deletion of the nodes at the end of Models lifetime. This fixes #2519. @ChenK19 Can you check if #2512 is fixed by this as well?

@hakonsbm hakonsbm added T: Bug Wrong statements in the code or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Nov 2, 2022
Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

LGTM!

@heplesser
Copy link
Contributor

Is there any performance effect of replacing a normal with a smart pointer?

@hakonsbm
Copy link
Contributor Author

hakonsbm commented Nov 2, 2022

@heplesser Create with this branch appears to be initially slightly slower (~4 % for 1e6 nodes), but faster after the first ResetKernel (~55 % s for 1e6 nodes).

@heplesser
Copy link
Contributor

@heplesser Create with this branch appears to be initially slightly slower (~4 % for 1e6 nodes), but faster after the first ResetKernel (~55 % s for 1e6 nodes).

Are there any effects on simulation time? I assume that smart pointers take more memory (reference counting needs to happen somewhere) and thus worry that they are cached less effectively, so that neuron access during spike delivery might become less efficient. And since neurons are created in a single place and deleted in a single place, I don't think we really need smart pointers.

I think in #2316 we just overlooked the need for destruction when we went away from the memory pool. In the longer run, we should keep this comment by @jougs in mind.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

I think we can solve this without smart pointers and the performance and memory cost they imply.

nestkernel/model.h Outdated Show resolved Hide resolved
nestkernel/node_manager.cpp Outdated Show resolved Hide resolved
@heplesser heplesser removed the request for review from jougs November 3, 2022 07:43
@heplesser
Copy link
Contributor

I just did a little test, comparing a program filling a vector with 2^10 *int vs std::shared_ptr<int>, using g++12 and clang++14. In both cases, the shared pointer takes very close to 40B extra memory per vector element compared to a plain pointer.

For more on the implementation in the gcc case, see https://gcc.gnu.org/onlinedocs/libstdc++/manual/memory.html#std.util.memory.shared_ptr.

@heplesser heplesser merged commit 3bb6e90 into nest:master Nov 10, 2022
@hakonsbm hakonsbm deleted the model_leak_fix branch November 10, 2022 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Repeat simulation with memory leakage
3 participants