mixing c++ and python memory models.

Here's the place for discussion related to CAM/CNC and the development of the Path module.
User avatar
freman
Posts: 1363
Joined: Tue Nov 27, 2018 10:30 pm

mixing c++ and python memory models.

Postby freman » Sat Sep 26, 2020 11:57 am

There was some rather inconclusive discussion recently on massive memory leakage in FreeCAD path work bench.

Basically memory is not getting released because there are thousands refcounts building up inside various loops which prevents the usual garbage collection when variables go out of scope.

Much of this seems to be caused by the rather schizophrenic hacking together of python and C++ which have fundamentally different memory management techniques. A lot of these memory allocations are being done in C++ code, that requires explicit deallocation. This is not happening from python code which relies on implicit garbage collection and reference counting.

It seems to me that in order to trigger the destructors in the C++ code the python objects need to provide __del__ methods which would then be able to call the C++ destructors. Yet when I grep the code base there are only a handful of occurrences of "__del__"

Is this is something which can be addressed, or is it a bastardised mix languages which is never going to work cleanly?

I don't have direct experience of trying to marry these two together like this but it seems that the mechanism is there via __del__ methods.

Can anyone comment on whether that is how it should be done and whether it could help tidy up the memory leakage issue?

TIA.
User avatar
sliptonic
Posts: 2073
Joined: Tue Oct 25, 2011 10:46 pm
Location: Columbia, Missouri
Contact:

Re: mixing c++ and python memory models.

Postby sliptonic » Sun Sep 27, 2020 12:50 pm

Interesting. Can you point me to the earlier discussions?

Python and c++ are used in conjunction all over freecad with no consistent effort to manage memory and no systemic problem.

When we've had memory problems in Path in the past we've been able to trace them back to specific operations and resolve them. Let's try to narrow this down a bit.
User avatar
freman
Posts: 1363
Joined: Tue Nov 27, 2018 10:30 pm

Re: mixing c++ and python memory models.

Postby freman » Sun Sep 27, 2020 3:49 pm

Hi, I think this may be it. Someone was finding grbl_post was causing memory to to be hogged. ( allocated blocks never getting freed.) I did quite a bit of testing with tracing tools and found literally thousands of refcounts to the same objects accumulating and causing memory never be released. There may have been another thread but I can't find it. The main issue I looked at was this thing in grbl_post.

https://forum.freecadweb.org/viewtopic. ... 4&start=40

At the end of that thread I posted a mod which I think prevents the needless reloading of the post processor on each call. Currently it's creating a new name space on each call to post proc, which makes new copies of the variables and prevents old ones getting cleared. It's in PathPostProcessor.py that concerns all post processing, not just grbl. It seems like this needed to be changed to work with py3 and comments indicate the current solution a bit of a hack just to get it flying. It may have been forgotten since, because it is functional.

Please look at whether that code is correct. If you think it looks OK, I'll submit a PR.

There are more issues here but let's deal with one at a time. This will help memory issues if this code is acceptable.

Maybe you could reply on the other thread about that issue and we'll get back to addressing other memory hogging issues here.

[EDIT] this was not the one concerning mix of C++ and py but was one case causing large memory consumption on complex paths like 3Dsurface, it merits attention. I'll try to find the other issue. I was discussing with etromby but I can't find it just now.
User avatar
freman
Posts: 1363
Joined: Tue Nov 27, 2018 10:30 pm

Re: mixing c++ and python memory models.

Postby freman » Sat Oct 03, 2020 10:37 am

CommandPy.cpp

Code: Select all

// Parameters() callback and implementer
// PyObject*  CommandPy::Parameters(PyObject *args){};
// has to be implemented in CommandPyImp.cpp
PyObject * CommandPy::staticCallback_getParameters (PyObject *self, void * /*closure*/)
{
    if (!static_cast<PyObjectBase*>(self)->isValid()){
        PyErr_SetString(PyExc_ReferenceError, "This object is already deleted most likely through closing a document. This reference is no longer valid!");
        return NULL;
    }

    try {
        return Py::new_reference_to(static_cast<CommandPy*>(self)->getParameters());
    } catch (const Py::Exception&) {
        // The exception text is already set
        return NULL;
    } catch (...) {
        PyErr_SetString(Base::BaseExceptionFreeCADError, "Unknown exception while reading attribute 'Parameters' of object 'Command'");
        return NULL;
    }
}
Wow, python calling c++ which is accessing python structures to create new references to python objects.

I can see how the garbage collector may be having problems.

This may or may not be the actual cause of the refcount problems but I can see it is going to be pretty tricky to unpick exactly what is happening here.

Maybe someone could comment on how this additional reference gets decremented when no longer used ?
wmayer
Site Admin
Posts: 16841
Joined: Thu Feb 19, 2009 10:32 am

Re: mixing c++ and python memory models.

Postby wmayer » Sun Oct 04, 2020 9:27 am

Much of this seems to be caused by the rather schizophrenic hacking together of python and C++ which have fundamentally different memory management techniques. A lot of these memory allocations are being done in C++ code, that requires explicit deallocation.
There is no general problem that would disallow to make this behaving correctly.

When there is a concrete memory leak of the Python wrapper then most likely because the functions of the Python C API are not used correctly. One always has to look in the documentation to see if a function returns a new reference or a borrowed reference and the calling instance must consider it.
It seems to me that in order to trigger the destructors in the C++ code the python objects need to provide __del__ methods which would then be able to call the C++ destructors. Yet when I grep the code base there are only a handful of occurrences of "__del__"
What are you talking about? Python classes that internally have C++ objects are not written in Python but C++. And for our binding generation framework the very base class is PyObjectBase and this implements the tp_dealloc slot of the type object. There is no need that this must be re-implemented in any subclass.
Is this is something which can be addressed, or is it a bastardised mix languages which is never going to work cleanly?
Of course, this can work cleanly. As said above when using the C API of Python one has to be careful about the type of reference returned. To limit such kind of problems I strongly recommend to use the PyCXX API which handles the reference mechanism automatically in most cases.
CommandPy.cpp
Wow, python calling c++ which is accessing python structures to create new references to python objects.
OK, can you tell me what's wrong with it?
FYI, this is the generated code of a Python wrapper and in this case it invokes a function which is declared as:

Code: Select all

Py::Dict CommandPy::getParameters() const
Assuming this function is implemented correctly then it returns a Py::Dict object with a reference count of 1. The call of Py::new_reference_to() increments it to 2 but then the destructor of Py::Dict (actually the destructor of the base class Py::Object) decrements it back to 1. So, to the Python interpreter an object with a reference count of 1 is passed.

But, the point here is that Py::Dict CommandPy::getParameters() is NOT implemented correctly. The code is:

Code: Select all

Py::Dict CommandPy::getParameters(void) const
{
    PyObject *dict = PyDict_New();
    for(std::map<std::string,double>::iterator i = getCommandPtr()->Parameters.begin(); i != getCommandPtr()->Parameters.end(); ++i) {
#if PY_MAJOR_VERSION >= 3
        PyDict_SetItem(dict,PyUnicode_FromString(i->first.c_str()),PyFloat_FromDouble(i->second));
#else
        PyDict_SetItem(dict,PyString_FromString(i->first.c_str()),PyFloat_FromDouble(i->second));
#endif
    }
    return Py::Dict(dict);
}
which creates a Py::Dict with a reference count of 2. PyDict_New() creates an object with reference count 1 but the constructor of Py::Dict increases it to 2. So, this is a good example to avoid the plain C API and only use the PyCXX API instead. This would guarantee correct behaviour and one hasn't to distinguish between Py2 and Py3.

So, the implementation could be:

Code: Select all


Py::Dict CommandPy::getParameters(void) const
{
    Py::Dict dict;
    for(std::map<std::string,double>::iterator i = getCommandPtr()->Parameters.begin(); i != getCommandPtr()->Parameters.end(); ++i) {
        dict.setItem(i->first, Py::Float(i->second));
    }
    return dict;
}
User avatar
freman
Posts: 1363
Joined: Tue Nov 27, 2018 10:30 pm

Re: mixing c++ and python memory models.

Postby freman » Sun Oct 04, 2020 11:56 am

Code: Select all

But, the point here is that Py::Dict CommandPy::getParameters() is NOT implemented correctly. The code is: 
Many thanks for your wise and experience soaked comments.

I suspected there was something wrong with getParameters and was actively compiling test results to demonstrate it in the hope that someone with better knowledge of the code would take a look. I'd been looking at new_reference_to() and the use of _XINCREF and _XDECREF.

It seems you've gone straight to it.

As you say, it needs some pretty tight and well written code to get this to work properly.

Here is tracemalloc output from a test producing 16k lines of .nc , it's losing 100MB of memory on one call to the post proc. This is a non trivial bug.

Code: Select all

12:38:15  Done postprocessing.
12:38:15  [ gc done ]
12:38:15  [ Top 15 differences ]
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:403: size=91.3 MiB (+91.3 MiB), count=1468255 (+1468255), average=65 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:414: size=12.0 MiB (+12.0 MiB), count=217281 (+217281), average=58 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:422: size=5067 KiB (+4437 KiB), count=104216 (+77343), average=50 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:426: size=4437 KiB (+4437 KiB), count=77343 (+77343), average=59 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:424: size=4437 KiB (+4437 KiB), count=77343 (+77343), average=59 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:425: size=4200 KiB (+4200 KiB), count=74064 (+74064), average=58 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:423: size=4200 KiB (+4200 KiB), count=74064 (+74064), average=58 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:427: size=3925 KiB (+3925 KiB), count=69153 (+69153), average=58 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:406: size=3688 KiB (+3688 KiB), count=65874 (+65874), average=57 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:415: size=1736 KiB (+1736 KiB), count=74066 (+74066), average=24 B
12:38:15  /~/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:407: size=515 KiB (+515 KiB), count=21959 (+21958), average=24 B
12:38:15  /usr/lib64/python3.7/tracemalloc.py:185: size=960 B (-45.6 KiB), count=15 (-730), average=64 B
12:38:15  /usr/lib64/python3.7/tracemalloc.py:113: size=0 B (-24.6 KiB), count=0 (-262)
12:38:15  /usr/lib64/python3.7/argparse.py:819: size=3192 B (-6384 B), count=19 (-38), average=168 B
12:38:15  /usr/lib64/python3.7/argparse.py:611: size=792 B (-4176 B), count=11 (-58), average=72 B
I had produced a modification which mitigated the problem by using getParameters outside the iteration loops, but now we know the exact cause maybe we can get a definitive bug fix.

Having applied the above suggested code fix it looks a lot better.

Code: Select all

13:16:40  [ gc done ]
13:16:40  [ Top 15 differences ]
13:16:40  /svn/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:326: size=2125 B (+2125 B), count=30 (+30), average=71 B
13:16:40  /svn/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:259: size=1736 B (+1736 B), count=8 (+8), average=217 B
13:16:40  /svn/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:146: size=952 B (+952 B), count=2 (+2), average=476 B
13:16:40  /usr/lib64/python3.7/sre_parse.py:420: size=856 B (+856 B), count=1 (+1), average=856 B
13:16:40  /usr/lib64/python3.7/shlex.py:310: size=848 B (+848 B), count=2 (+2), average=424 B
13:16:40  /usr/lib64/python3.7/argparse.py:1787: size=760 B (+760 B), count=1 (+1), average=760 B
13:16:40  /svn/freecad-build/Mod/Path/PathScripts/PostUtils.py:74: size=656 B (+656 B), count=11 (+11), average=60 B
13:16:40  /svn/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:295: size=656 B (+656 B), count=1 (+1), average=656 B
13:16:40  /usr/lib64/python3.7/argparse.py:1993: size=624 B (+624 B), count=1 (+1), average=624 B
13:16:40  /svn/freecad-build/Mod/Path/PathScripts/post/grbl_post.py:324: size=584 B (+584 B), count=2 (+2), average=292 B
13:16:40  /usr/lib64/python3.7/shlex.py:46: size=576 B (+576 B), count=1 (+1), average=576 B
13:16:40  /usr/lib64/python3.7/argparse.py:1778: size=576 B (+576 B), count=1 (+1), average=576 B
13:16:40  /usr/lib64/python3.7/sre_parse.py:810: size=568 B (+568 B), count=1 (+1), average=568 B
13:16:40  /usr/lib64/python3.7/argparse.py:2018: size=536 B (+536 B), count=1 (+1), average=536 B
13:16:40  /usr/lib64/python3.7/argparse.py:1996: size=536 B (+536 B), count=1 (+1), average=536 B

Dangling recounts down from 1468255 to 30 !!
The offending line 326 there involves GCodeEditorDialog().
wmayer
Site Admin
Posts: 16841
Joined: Thu Feb 19, 2009 10:32 am

Re: mixing c++ and python memory models.

Postby wmayer » Sun Oct 04, 2020 12:38 pm

Here is tracemalloc output from a test producing 16k lines of .nc , it's losing 100MB of memory on one call to the post proc. This is a non trivial bug.
Wow! 100MB that's huge!
Dangling recounts down from 1468255 to 30 !!
So, this fixes 99.998% of all refcount issues of your test. Does it also fix the memory leak in the same ratio so that ~2KB are still leaking?
What are the remaining 30 refcount reports?
User avatar
freman
Posts: 1363
Joined: Tue Nov 27, 2018 10:30 pm

Re: mixing c++ and python memory models.

Postby freman » Sun Oct 04, 2020 12:57 pm

Yes, it was a bit of problem! With the new 3D tools producing several orders of magnitude more gcode than the traditional 2.5D tools, it's blow up. This has presumably been there since the very beginning of Path WB but never got audited for leaks and it was not big enough to be noticeable on the traditional tools.

I'm glad we have a fix at last. That pleases me more than one suggestion that I buy a newer PC ;)

Please note that the remaining problem is not in same part of this file. Your suggested fix gets rid of 100% of that particular leak.

line 326 is the call to the codeEditor exec_()

Code: Select all

  # show the gCode result dialog
  if FreeCAD.GuiUp and SHOW_EDITOR:
    dia = PostUtils.GCodeEditorDialog()
    dia.editor.setText(gcode)
    result = dia.exec_()
    if result:
      final = dia.editor.toPlainText()
    else:
      final = gcode
  else:
    final = gcode

tracing in caller shows it's leaking a instance of the dlg object each time post proc is called. See link below.

line 259:

Code: Select all

    if not hasattr(obj, "Path"):
That may be a similar low level issue.



What do you make of the technique of reload() each time the post proc. is called. It seems a bit clunky to me. As I linked here from the doc,: https://forum.freecadweb.org/viewtopic. ... 40#p435631 , it seems that this may not be clear existing memory allocations. Maybe I'm misinterpreting that. I think it would be cleaner if this code was wrapped as a class and the "global" variables reset through __init__ , if that is the only purpose of that.

I've got class wrapper but have not removed the reload code to test it yet.
User avatar
sliptonic
Posts: 2073
Joined: Tue Oct 25, 2011 10:46 pm
Location: Columbia, Missouri
Contact:

Re: mixing c++ and python memory models.

Postby sliptonic » Sun Oct 04, 2020 1:31 pm

Fantastic work, guys!

Thank you Werner. I learn something every time you post.

Freman, great job bulldogging a nasty bug!
User avatar
freman
Posts: 1363
Joined: Tue Nov 27, 2018 10:30 pm

Re: mixing c++ and python memory models.

Postby freman » Sun Oct 04, 2020 5:38 pm

Thanks, I'm glad the effort is appreciated and bore fruit.

Lots of thanks to Werner for jumping. I was looking at exactly that code last night but I would have spent quite a lot more time unpicking exactly how it was supposed to work and how the refcounting was going wrong. At least I have a lot more confidence about this being an isolated bug rather than a structural problem now I've got into the guts of it.

Is anyone doing a PR for this?