On Fri, Jul 18, 2008 at 5:15 AM, Michael Abbott wrote:

I'm afraid this is going to be a long one, and I don't see any good way to cut down the quoted text either...

Charles, I'm going to plea with you to read what I've just written and think about it. I'm trying to make the case as clear as I can. I think the case is actually extremely simple: the existing @name@_arrtype_name code is broken.

<snip>

Heh. As the macro is undefined after the code is generated, it should probably be moved into the code. I would actually like to get rid of the ifdef's (almost everywhere), but that is a later stage of cleanup.

3. Here's the reference count we're responsible for.

Yep.

4. If obj is NULL we use the typecode 5. otherwise we pass it to PyArray_FromAny. 6. The first early return 7. All paths (apart from 6) come together here.

So at this point let's take stock. typecode is in one of three states: NULL (path 2, or if creation failed), allocated with a single reference count (path 4), or lost (path 5). This is not good.

It still has a single reference after 5 if PyArray_FromAny succeeded, that reference is held by arr and transferred to robj. If the transfer fails, the reference to arr is decremented and NULL returned by PyArray_Return. When arr is garbage collected the reference to typecode will be decremented.

LET ME EMPHASISE THIS: the state of the code at the finish label is dangerous and simply broken.

The original state at the finish label is indeterminate: typecode has either been lost by passing it to PyArray_FromAny (in which case we're not allowed to touch it again), or else it has reference count that we're still responsible for.

There seems to be a fantasy expressed in a comment in a recent update to this routine that PyArray_Scalar has stolen a reference, but fortunately a quick code inspection (of arrayobject.c) quickly refutes this frightening possibility.

No, no, Pyarray_Scalar doesn't do anything to the reference count. Where did you see otherwise?

So, the only way to fix the problem at (7) is to unify the two non-NULL cases. One answer is to add a DECREF at (4), but we see at (11) that we still need typecode at (7) -- so the only solution is to add an extra ADDREF just before (5). This then of course sadly means that we also need an extra DECREF at (6).

PLEASE don't suggest moving the ADDREF until after (6) -- at this point typecode is lost and may have been destroyed, and relying on any possibility to the contrary is a recipe for continued screw ups.

The rest is easy. Once we've established the invariant that typecode is either NULL or has a single reference count at (7) then the two early returns at (8) and (9) unfortunately need to be augmented with DECREFs.

And we're done.

Responses to your original comments:

...

By the time we hit finish, robj is NULL or holds a reference to typecode and the NULL case is taken care of up front.

robj has nothing to do with the lifetime management of typecode, the only issue is the early return. After the finish label typecode is either NULL (no problem) or else has a single reference count that needs to be accounted for.

...

Later on, the reference to typecode might be decremented, That *might* is at the heart of the problem. You can't be so cavalier about managing references.

...

perhaps leaving robj crippled, but in that case robj itself is marked for deletion upon exit. Please ignore robj in ths discussion, it's beside the point.

...

If the garbage collector can handle zero reference counts I think we are alright. No, no, no. This is nothing to do with the garbage collector. If we screw up our reference counts here then the garbage collector isn't going to dig us out of the hole.

The garbage collector destroys the object and should decrement all the references it holds. If that is not the case then there are bigger problems afoot. The finalizer for the object should hold the knowledge of what needs to be decremented.

...

I admit I haven't quite followed all the subroutines and macros, which descend into the hazy depths without the slightest bit of documentation, but at this point I'm inclined to leave things alone unless you have a test that shows a leak from this source.

Part of my point is that proper reference count discipline should not require any descent into subroutines (except for the very nasty case of reference theft, which I think is generally agreed to be a bad thing).

Agreed. But that is not the code we are looking at. My personal schedule for this sort of cleanup/refactoring looks like this. 1) Format the code into readable C. (ongoing) 2) Document the functions so we know what they do. 3) Understand the code. 4) Fix up functions starting from the bottom layers. 5) Flatten the code -- the calls go too deep for my taste and make understanding difficult. My attempts to generate a call graph have all run into problems. At that point consider the reference counting model. There aren't many people working with the C code apart from myself and Travis (who wrote the original), so I want to encourage you to keep looking at the code and working with it. However, we can't just start from scratch and we have to be pretty conservative to keep from breaking things.

As for the test case, try this one (you'll need a debug build):

import numpy import sys refs = 0 r = range(100) refs = sys.gettotalrefcount() for i in r: float32() print sys.gettotalrefcount() - refs

Simpler test case: import sys, gc import numpy as np def main() : t = np.dtype(np.float32) print sys.getrefcount(t) for i in range(100) : np.float32() gc.collect() print sys.getrefcount(t) if __name__ == "__main__" : main() Result $[charris@f8 ~]$ python debug.py 5 105 So there is a leak. The question is the proper fix. I want to take a closer look at PyArray_Return and also float32() and relations. Chuck

On Fri, Jul 18, 2008 at 2:41 PM, Charles R Harris wrote: <snip> There are actually two bugs here which is confusing things. Bug 1) Deleting a numpy scalar doesn't decrement the descr reference count. Bug 2) PyArray_Return decrements the reference to arr, which in turn correctly decrements the descr on gc. So calls that go through the default (obj == NULL) correctly leave typecode with a reference, it just never gets decremented when the return object is deleted. On the other hand, if the function is called with something like float32(0), then arr is allocated, stealing a reference to typecode. PyArray_Return then deletes arr (which decrements the typecode reference), but that doesn't matter since typecode is a singleton. In this case there is no follow on stack dump when the returned object is deleted because the descr reference is not correctly decremented. BTW, both cases get returned in the first if statement after the finish label, i.e., robj is returned. Oy, what a mess. Here is a short program to follow all the reference counts. import sys, gc import numpy as np def main() : typecodes = "?bBhHiIlLqQfdgFDG" for typecode in typecodes : t = np.dtype(typecode) refcnt = sys.getrefcount(t) t.type() gc.collect() print typecode, t.name, sys.getrefcount(t) - refcnt if __name__ == "__main__" : main() Which gives the following output: $[charris@f8 ~]$ python debug.py ? bool 0 b int8 1 B uint8 1 h int16 1 H uint16 1 i int32 0 I uint32 1 l int32 0 L uint32 1 q int64 1 Q uint64 1 f float32 1 d float64 0 g float96 1 F complex64 1 D complex128 1 G complex192 1 Note that the python types, which the macro handles, don't have the reference leak problem. Chuck

On Fri, Jul 18, 2008 at 9:30 PM, Travis E. Oliphant wrote:

Charles R Harris wrote:

...

The reference leak seems specific to the float32 and complex64 types called with default arguments.

In [1]: import sys, gc

In [2]: t = float32

In [3]: sys.getrefcount(dtype(t)) Out[3]: 4

In [4]: for i in range(10) : t(); ...:

In [5]: sys.getrefcount(dtype(t)) Out[5]: 14

In [6]: for i in range(10) : t(0); ...:

In [7]: sys.getrefcount(dtype(t)) Out[7]: 14

In [8]: t = complex64

In [9]: sys.getrefcount(dtype(t)) Out[9]: 4

In [10]: for i in range(10) : t(); ....:

In [11]: sys.getrefcount(dtype(t)) Out[11]: 14

In [12]: t = float64

In [13]: sys.getrefcount(dtype(t)) Out[13]: 19

In [14]: for i in range(10) : t(); ....:

In [15]: sys.getrefcount(dtype(t)) Out[15]: 19

This shouldn't actually leak any memory as these types are singletons, but it points up a logic flaw somewhere.

That is correct. There is no memory leak, but we do need to get it right. I appreciate the extra eyes on some of these intimate details. What can happen (after a lot of calls) is that the reference count can wrap around to 0 and then cause a funny dealloc (actually, the dealloc won't happen, but a warning will be printed).

Fixing the reference counting issues has been the single biggest difficulty in converting PyArray_Descr from a C-structure to a regular Python object. It was a very large pain to begin with, and then there has been more code added since the original conversion some of which does not do reference counting correctly (mostly my fault).

I've looked over ticket #848 quite a bit and would like to determine the true cause of the growing reference count which I don't believe is fixed by the rest of the patch that is submitted there.

I've attached a test script. Chuck

On Fri, Jul 18, 2008 at 11:35:50PM -0500, Travis E. Oliphant wrote:

...

I've attached a test script. Thank you! It looks like with that added DECREF, the reference count leak is gone. While it was a minor issue (it should be noted that reference counting errors on the built-in data-types won't cause issues), it is nice to clean these things up when we can.

Yes. I think it is worth thanking all of you who are currently putting a large effort on QA. This effort is very valuable to all of us, as having a robust underlying library on which you can unquestionably rely is priceless. Gaël

On Sat, Jul 19, 2008 at 8:57 AM, Michael Abbott wrote:

On Fri, 18 Jul 2008, Travis E. Oliphant wrote:

...

It looks like with that added DECREF, the reference count leak is gone.

I've looked at the latest head, and I agree that the problem is now solved.

There is an important difference from my original solution: typecode is no longer reused after the finish label (instead it is always created anew). This makes all the difference in the world.

I'm not actually convinced by the comment that's there now, which says /* typecode will be NULL */ but in truth it doesn't matter -- because of the correcly placed DECREF after the PyArray_Scalar calls the routine no longer owns typecode.

If I can refer to my last message, I made the point that there wasn't a good invariant at the finish label -- we didn't know how many references to typecode we were responsible for at that point -- and I offered the solution to keep typecode. Instead you have chosen to recreate typecode, which I hadn't realised was just as good.

This code is still horrible, but I don't think I want to try to understand it anymore. It'd be really nice (it'd make me feel a lot better) if you'd agree that my original patch was in fact correct. I'm not disputing the correcness of the current solution (except I think that typecode can end up being created twice, but who really cares?) but I've put a lot of effort into arguing my case, and the fact is my original patch was not wrong.

Yep, the original patch looks good now.

Travis, thank you for your encouraging words. On Sat, 19 Jul 2008, Travis E. Oliphant wrote:

Really this code "grew" from a simple thing into a complicated thing as more "features" were added. This is a common issue that happens all over the place. Aye.

The reason I say I'm not sure is that I don't remember seeing a DECREF after the PyArray_Scalar in the obj = NULL part of the code in my looking at your patches. But, I could have just missed it. There wasn't -- instead, I was trying to retain the typecode (and paying the price of releasing it on all the early returns!)

From a "generic" reference counting point of view you did correctly emphasize the problem of having a reference count creation occur in an if-statement but a DECREF occur all the time in the finish: section of the code. Yah -- I think the core idea I was trying to get over is that of an "invariant" property at each point in the code to capture what needs to be true for the code to be correct.

It was really that statement: "the fantasy that PyArray_Scalar steals a reference" that tipped me off to what I consider one of the real problems to be. The fact that it was masked at the end of a long discussion about other reference counting and a "stealing" discussion that were not the core problem was distracting and ultimately not very helpful. That was the really hard bit for me. To me the issue was actually very obvious (though I didn't realise that typecode could be regenerated, which simplifies things enormously), so the problem was trying to figure out what you and Charles were not seeing. I think that's why I ended up throwing everything into the pot!

I'm very impressed with your ability to follow these reference count issues. Especially given that you only started learning about the Python C-API a few months ago (if I remember correctly). Alas no. I'm a bit of an old lag really, I did dabble with the Python C API quite a few years ago (2001ish maybe?). Myy roots are in computer science and then assembler (graduated 1980) before Pascal (seriously) then C, then C++ (which I now regard as a serious mistake) and finally shell script plus Python, all largely on embedded applications. I'd love the opportunity to learn and use Haskell now.

I'm also very glad you are checking these corner cases which have not received the testing that they deserve. I hope we have not discouraged you too much from continuing to help. Your input is highly valued.

Maybe I'll have a further play. The memory leak issue was a direct consequence of using numpy in an embedded application, and that issue's closed now, but I ought to see if this painful code can be revisited. I'm learning my way around git and have just used `git svn` to grab (and update) the numpy repository. I'm hugely impressed by it, though it is very expensive to run the first time -- it fetches every single svn revision! Hopefully that didn't overload the web server... This will make working on patches much easier.

On Sun, 20 Jul 2008, Stéfan van der Walt wrote:

2008/7/20 Michael Abbott :

...

C, then C++ (which I now regard as a serious mistake) and finally shell It's scary how many of us were scarred for life by C++.

What's really annoying for me is that my most recent big project (http://sourceforge.net/projects/libera-epics) was written in C++, entirely at my own choice. It seemed like a good idea at the time.

If you have ten minutes free sometime, would you please consider writing up your reference counting explanation on the wiki

Good idea. Do you mean at http://scipy.org/scipy/numpy ? Somewhere under CodingStyleGuidelines?

2008/7/20 Michael Abbott :

On Sun, 20 Jul 2008, Stéfan van der Walt wrote:

...

2008/7/20 Michael Abbott :

...

C, then C++ (which I now regard as a serious mistake) and finally shell It's scary how many of us were scarred for life by C++.

What's really annoying for me is that my most recent big project (http://sourceforge.net/projects/libera-epics) was written in C++, entirely at my own choice. It seemed like a good idea at the time.

...

If you have ten minutes free sometime, would you please consider writing up your reference counting explanation on the wiki

Good idea. Do you mean at http://scipy.org/scipy/numpy ? Somewhere under CodingStyleGuidelines?

Or here: http://projects.scipy.org/scipy/numpy/ under `guidelines`. Thanks! Stéfan

Charles R Harris wrote:

On Fri, Jul 18, 2008 at 9:15 PM, Travis E. Oliphant mailto:oliphant@enthought.com> wrote:

Michael Abbott wrote: > Only half of my patch for this bug has gone into trunk, and without the > rest of my patch there remains a leak. > Thanks for your work Michael. I've been so grateful to have you and Chuck and others looking carefully at the code to fix its problems.

In this particular case, I'm not sure I see how (the rest of) your patch fixes any remaining leak. We do seem to be having a disagreement about whether or not the reference to typecode can be pre-maturely destroyed, but this doesn't fit what I usually call a "memory leak." I think there may be some other cause for remaining leaks.

Travis,

There really is (at least) one reference counting error in PyArray_FromAny. In particular, the obj == NULL case leaves a reference to typecode, then exits through the first return after finish. In this case robj doesn't steal a reference to typecode and the result can be seen in the python program above or by printing out the typecode->ob_refcnt from the code itself. So that needs fixing. I would suggest a DECREF in that section and a direct return of robj.

The next section before finish is also a bit odd. The direct return of an array works fine, but if that isn't the branch taken, then PyArray_Return decrements the refcnt of arr, which in turn decrements the refcnt of typecode. I don't know if the resulting scalar holds a reference to typecode, but in any case the situation there should also be clarified.

Thank you. I will direct attention there and try to clear this up tonight. I know Michael is finding problems that do need fixing. -Travis

On Fri, Jul 18, 2008 at 10:04 PM, Travis E. Oliphant wrote:

Charles R Harris wrote:

...

On Fri, Jul 18, 2008 at 9:15 PM, Travis E. Oliphant mailto:oliphant@enthought.com> wrote:

Michael Abbott wrote: > Only half of my patch for this bug has gone into trunk, and without the > rest of my patch there remains a leak. > Thanks for your work Michael. I've been so grateful to have you and Chuck and others looking carefully at the code to fix its problems.

In this particular case, I'm not sure I see how (the rest of) your patch fixes any remaining leak. We do seem to be having a disagreement about whether or not the reference to typecode can be pre-maturely destroyed, but this doesn't fit what I usually call a "memory leak." I think there may be some other cause for remaining leaks.

Travis,

There really is (at least) one reference counting error in PyArray_FromAny. In particular, the obj == NULL case leaves a reference to typecode, then exits through the first return after finish. In this case robj doesn't steal a reference to typecode and the result can be seen in the python program above or by printing out the typecode->ob_refcnt from the code itself. So that needs fixing. I would suggest a DECREF in that section and a direct return of robj.

agreed! I'll commit the change.

...

The next section before finish is also a bit odd. The direct return of an array works fine, but if that isn't the branch taken, then PyArray_Return decrements the refcnt of arr, which in turn decrements the refcnt of typecode. I don't know if the resulting scalar holds a reference to typecode, but in any case the situation there should also be clarified.

This looks fine to me. At the PyArray_Return call, the typecode reference is held by the array. When it is decref'd the typecode is decref'd appropriately as well. The resulting scalar does *not* contain a reference to typecode. The scalar C-structure has no place to put it (it's just a PyObject_HEAD and the memory for the scalar value).

I was thinking of just pulling the relevant part out of PyArray_Return and including it in the function, which would make what was going on quite explicit to anyone reading the code. Then maybe a direct return of robj as I think it is always going to be a scalar at that point.

Michael is correct that PyArray_Scalar does not change the reference count of typecode (as the comments above that function indicates). I tried to be careful to put comments near the functions that deal with PyArray_Descr objects to describe how they affect reference counting. I also thought I put that in my book.

Yep, it was a brain fart on my part. Chuck