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

Memory Leak Fixes #819

Open
KD-92 opened this issue Sep 22, 2024 · 2 comments
Open

Memory Leak Fixes #819

KD-92 opened this issue Sep 22, 2024 · 2 comments

Comments

@KD-92
Copy link

KD-92 commented Sep 22, 2024

Describe the bug
When reinitializing the Simulator repeatedly, e.g., in a Reinforcement Learning (RL) setting, memory growth can be observed. Please find different fixes below (this is not a Pull Request due to the last item in the list).

To reproduce

  1. ReactionWheelStateEffector Memory Leak
    In addition to issue #441, the destructor ReactionWheelStateEffector::~ReactionWheelStateEffector() in src/simulation/dynamics/reactionWheels/reactionWheelStateEffector.cpp uses free() on a data structure allocated with new. Additionally, it may be helpful to delete the elements of rwOutMsgs:
ReactionWheelStateEffector::~ReactionWheelStateEffector()
{
    for (long unsigned int c = 0; c < this->rwOutMsgs.size(); c++) {
        delete this->rwOutMsgs.at(c);
    }

    for (long unsigned int c = 0; c < this->ReactionWheelData.size(); c++) {
        delete this->ReactionWheelData.at(c);
    }
}
  1. Missing Py_DECREF in SWIG Eigen Type Map
    There are missing Py_DECREF in src/architecture/_GeneralModuleFiles/swig_eigen.i in the fragment checkPyObjectIsMatrixLike. The changes are annotated with new decrement.
%fragment("checkPyObjectIsMatrixLike", "header", fragment="getInputSize") {

/*
This method returns an empty optional only if the given input is like the template type T.
Otherwise, the returned optional contains a relevant error message.
The size of the input is compared to the expected size of T (where dynamically
sized matrix T allow any number of rows/columns). Moreover, an error is raised for
ragged nested sequences (rows with different numbers of elements).
*/
template<class T>
std::optional<std::string> checkPyObjectIsMatrixLike(PyObject *input)
{
    auto maybeSize = getInputSize(input);

    if (!maybeSize)
    {
        return "Input is not a sequence";
    }

    auto [numberRows, numberColumns] = maybeSize.value(); 

    if (T::RowsAtCompileTime != -1 && T::RowsAtCompileTime != numberRows)
    {
        std::string errorMsg = "Input does not have the correct number of rows. Expected " 
            + std::to_string(T::RowsAtCompileTime) + " but found " + std::to_string(numberRows);
        return errorMsg;
    }

    if (T::ColsAtCompileTime != -1 && T::ColsAtCompileTime != numberColumns)
    {
        std::string errorMsg = "Input does not have the correct number of columns. Expected " 
            + std::to_string(T::ColsAtCompileTime) + " but found " + std::to_string(numberColumns);
        return errorMsg;
    }

    for(Py_ssize_t row=0; row<numberRows; row++)
    {
        PyObject *rowPyObj = PySequence_GetItem(input, row);
        Py_ssize_t localNumberColumns = PySequence_Check(rowPyObj) ? PySequence_Length(rowPyObj) : 1;
        if (localNumberColumns != numberColumns)
        {
            Py_DECREF(rowPyObj); // new decrement
            return "All rows must be the same length! Row " + std::to_string(row) + " is not.";
        }
        Py_DECREF(rowPyObj); // new decrement
    }
    
    return {};
}
}
  1. Check for Msg disown()
    It may be of interest to verify whether <type>Msg.this.disown() is necessary in:
  • src/utilities/fswSetupRW.py (writeConfigMessage(), rwConfigMsg.this.disown(), line 87)
  • src/utilities/simIncludeRW.py (getConfigMessage(self), rwConfigMsg.this.disown(), line 379)

Expected behavior
With these changes, memory growth became unnoticeable for us even after millions of iterations.

@schaubh
Copy link
Contributor

schaubh commented Sep 27, 2024

Howdy @KD-92 , thanks for the feedback. Something to look at in more detail. Regarding item 3., the reason we use disown() in the factory classes to ensure that the message memory is not released after being created in a sub-routine. There are likely better ways to do this that avoids creating msg objects which never get released

@sassy-asjp
Copy link
Contributor

@schaubh @KD-92 @juan-g-bonilla

I think the conflict between to disown() or not to disown() is related to #676 . Developing a proper fix for that bug would allow the lifetime of the msg objects to be properly handled, instead of having to choose between mistakenly garbage collected in some situations vs intentionally leaked in all situations.

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

No branches or pull requests

3 participants