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 GetString memory leak #70

Merged
merged 6 commits into from
Feb 20, 2020
Merged

Conversation

markaren
Copy link
Member

@markaren markaren commented Feb 19, 2020

GetString eats memory. This PR fixes it.

Closes #69

@markaren markaren changed the title Fix memory leak Fix GetString memory leak Feb 19, 2020
@markaren
Copy link
Member Author

Mhh. Indeed this fails with PyFMI, but works with FMI4j... Don't know what to do, because it does leak. PyUnicode_AsEncodedString returns a new reference, which is never decrefed.

@markaren
Copy link
Member Author

markaren commented Feb 19, 2020

I'm starting to think this could be a FMI4j bug? I need to check the standard on string handling.

For refernce, this work with PyFMI

PyObject* value = PyList_GetItem(refs, i);
PyObject* strValue = PyUnicode_AsEncodedString(value, "utf-8", nullptr);
char* src = PyBytes_AsString(strValue);
char* dst = reinterpret_cast<char*>(malloc(strlen(src) + 1));
strcpy_s(dst, strlen(src) + 1, src);
values[i] = dst;
Py_DECREF(strValue);

But I dunno if thats correct. It uses less memory in FMI4j, but still seams to leak. Is the caller responsible for deleting dst ?

@fcollonval
Copy link
Collaborator

fcollonval commented Feb 19, 2020

Hey I don't think this is a bug of FMI4J.

I have seen there is a built-in package for tracing Python allocation. Maybe you can get more inside with it?

https://docs.python.org/3/library/tracemalloc.html

Not: there is another one to correct too 😉

oss << PyBytes_AsString(PyUnicode_AsEncodedString(pRepr, "utf-8", nullptr));

@fcollonval
Copy link
Collaborator

From the specification:

The strings returned by fmi2GetString must be copied in the target environment because the allocated memory for these strings might be deallocated by the next call to any of the fmi2 interface functions or it might be an internal string buffer that is reused.

So from your latest proposal, could it be that the copied string dst are not released neither?

@markaren
Copy link
Member Author

dst leaks for sure.

So how I read your excerpt is that we must deallocate the string in another FMI call. Say the next invocation of GetString. Ok. Makes the code more complex, but doable.

@markaren
Copy link
Member Author

This should do it !

@markaren
Copy link
Member Author

oss << PyBytes_AsString(PyUnicode_AsEncodedString(pRepr, "utf-8", nullptr));

I'll fix that once #71 is merged

Lars Ivar Hatledal added 2 commits February 20, 2020 07:39
# Conflicts:
#	pythonfmu/pythonfmu-export/cpp/PySlaveInstance.cpp
#	pythonfmu/pythonfmu-export/headers/pythonfmu/PySlaveInstance.hpp
@markaren
Copy link
Member Author

We should probably release a patch including the two latest bugfixes soon.

Copy link
Collaborator

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks for catching and fixing this one.

We are almost good. I have just one concern about the buffer clearance not being called within a GIL captured lambda (see comments).

pythonfmu/pythonfmu-export/cpp/PySlaveInstance.cpp Outdated Show resolved Hide resolved
pythonfmu/pythonfmu-export/cpp/PySlaveInstance.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thank you!

@fcollonval fcollonval merged commit 776adb7 into master Feb 20, 2020
@markaren markaren deleted the bugfix/69-getstring-mem-leak branch February 20, 2020 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

GetString leaks memory
2 participants