ENH: Add cyclic garbage collection to NumPy #15065

seberg · 2019-12-06T18:34:01Z

This PR adds cyclic garbage collection to all (or at least most) objects within NumPy that can in theory form such cycles. It builds, and replaces, replaces gh-8303. Some thought may still be warrented in:

Give a bit of thoughts that all of these objects actually require cyclic garbage collection.
Ensure that the performance hit is insignificant (I assume it is, but should maybe time it).
- Full test suit run seems unaffected on my machine. So I will assume that the performance hit is moderate to non-existing for most cases. (Which does not mean that it cannot be real for some use cases I suppose)
Probably a few more tests should be added to ensure that the cycle resolution always works as advertised.

numpy/core/src/multiarray/refcount.c.src

numpy/core/src/multiarray/arrayobject.c

mattip

This seems somewhere between a cleanup and an enhancement. There are a lot of moving parts. I ran coverage locally and marked the new code paths that were not hit. I think refcount.c.src might need a little more thought: it is very dense and hard to reason about now.

numpy/core/tests/test_multiarray.py

mattip · 2019-12-09T17:14:40Z

numpy/core/src/multiarray/arrayobject.c

+     * or if it doesn't have any objects
+     */
+    if (!descr) {
+        return 0;


According to cov, the !descr path is never hit. Is it reasonable to remove the check?

Changed it PyArray_DATA(self) == NULL for now, the issue is that allocation adds the object to the GC tracker too early on in a sense. It probably cannot happen that the traverse ever kicks in, since we are busy within C if it fails at this point. But that is what this seems to have been for (the change was by @J-Sand, maybe he has a quick note on it).

It is likely that this is just a band-aid solution, and the correct thing is to ensure that tracking is only enabled after everything is initialized. This might be a major rough edge :/ thought this was further along.

numpy/core/src/multiarray/iterators.c

numpy/core/src/multiarray/refcount.c.src

mattip · 2019-12-09T17:43:35Z

numpy/core/src/multiarray/refcount.c.src

+                     visitproc visit, void *arg)
+#else
+    @inner_function@(char *data, PyArray_Descr *descr)
+#endif


This is really hard to reason about. Maybe it would be better to expand some of these templates at the cost of some code repitition.

changed slightly, not sure it is much better with inline #defines for the arguments though.

mattip · 2019-12-09T17:44:57Z

numpy/core/src/multiarray/refcount.c.src

-                NPY_COPY_PYOBJECT_PTR(&temp, data);
-                Py_XDECREF(temp);
+            for( i = 0; i < n; i++, data++) {
+                HANDLE_ITEM_AT_UNALIGNED(data);


I am pretty sure this is never hit

mattip · 2019-12-09T17:45:24Z

numpy/core/src/multiarray/refcount.c.src

-            Py_XDECREF(temp);
+        while (it.index < it.size) {
+            data = (PyObject **)it.dataptr;
+            HANDLE_ITEM_AT_UNALIGNED(data);
            PyArray_ITER_NEXT(&it);


I don't think this is ever hit

Probably not, it is pretty rare to create unaligned object arrays. Same goes for object arrays actually holding NULL instead of None. Although, I think the first one at least may be simple enough with a structured dtype, which would indeed be a good test.

nvm, made a test for this, I think.

mattip · 2019-12-09T17:47:26Z

numpy/core/src/multiarray/refcount.c.src

+        n = PyArray_SIZE(arr);
+        if (obj == NULL) {
+            for (i = 0; i < n; i++) {
+                *optr++ = NULL;


The code path obj == NULL is never hit

Yes, but it is exposed API, note that this function was not touched, the file was just modified so much it appears to be changed. Also it could just as well use XINCREF and remove the branching, I doubt it is worth to micro-optimize...

eric-wieser · 2019-12-16T21:55:51Z

numpy/core/src/multiarray/arrayobject.c

+        /* if the array doesn't own its data, visiting fa->base is sufficient */
+        return 0;
+    }
+    /* NOTE: In the future, user dtypes could in principle be visited */


Shouldn't the dtype be visited anyway, since it has metadata that could refer back to the array? No need to implement the visiting in the dtype object yet, but I think we should call visit here to play it safe for when we fix that.

It is pretty esoteric, but yes. We should visit the descriptor maybe. That would mean we cannot untrack arrays though, since any array has a dtype. (at least unless we also check that the dtype has no metadata). I think I meant the comment as visiting a user-dtypes items (which is much further off).

Not sure what to best do. Could just not do the untracking, or do it anyway, but visit. Either way, this would fix the vast majority of use-cases.

eric-wieser · 2019-12-16T21:57:32Z

numpy/core/src/multiarray/ctors.c

+        /*
+         * tp_alloc will have enabled GC tracking, but it's only necessary
+         * for object arrays. Subtypes may also define additional fields, so
+         * a subtype would have to handle this by itself.
+         */
+        PyObject_GC_UnTrack(fa);


Are you sure this is correct? What does cpython do about dict subclasses, for instance?

To be honest, I have no thought to much about the untracking, it is the method I took from the original PR. But I am not sure how you think this can be incorrect?

OK, I actually looked a bit at the Python code, and it seems to me that this is not correct and I would have expected it to run into asserts on a debug python build (under the assumption that such an array is added to container that implements cyclic gc). I am a bit surprised I did not notice that (although without checking I am not 100% we have CI which runs a python debug build).

There would be solution to this: One can implement tp_is_gc which returns False after untracking/for arrays which are not added to the GC. (There is probably only a single use case of this: Python uses it to distinguish between HEAP and static types.)

Right now, I think this is arguably a bug, and chances are that having a few more arrays does not matter (there must be a huge number of other python containers around that need to do more traversing).

Still planning to try out the speed penalty for keeping a huge object array around, but even if it is noticable, I am not sure it matters too much as to whether or not we should do this.

Why is the object being tracked at this point already? From the docs:

PyObject_GC_Track should be called once all the fields followed by the tp_traverse handler become valid, usually near the end of the constructor.

Could we just call PyObject_GC_Track here instead? That way, the subtype will just call PyObject_GC_Track itself if it adds new members, which ought to be safe, i think

In fact, aren't we supposed to call PyObject_GC_Track ourselves anyway? I'm not seeing the bit of the docs that imply its called for us.

The default tp_alloc calls it for you when Py_TPFLAGS_HAVE_GC. Strictly speaking: probably yes. But I also do not think it matters, since there we own the GIL the whole time here, and there is no call back into python before everything is finished up.

Anyway, if someone wants to review+merge this, I am happy to look at this, otherwise, for now we can also close until someone actually complains due to a memory leak. The new changes in python probably need a bit of cleaning, and the release notes may need to warn hypothetical hacks that subclass from C at least.

numpy/core/src/multiarray/iterators.c

numpy/core/src/multiarray/nditer_pywrap.c

numpy/core/src/multiarray/refcount.c.src

seberg · 2020-01-09T23:08:33Z

Thought I might be able to unstick this a bit by limiting it to ndarray for now (the other part is preserved https://github.com/seberg/numpy/tree/cyclic-gc-iterators). But in any case the untracking is a pattern I have not found in cpython itself. However, it is completely fine to untrack (and even retrack), and it seems even fine to untrack multiple times.
Otherwise not much changed, but squashed (the untrack logic in array_dealloc is slightly adapted).

If this remains stuck, just as well...

seberg · 2020-01-10T14:53:55Z

I suppose, the one real change to consider here is that might make sense to not bother doing this (possibly weird) untracking some arrays and just keep it tracked since the traverse will be very short.

This commit is based on work of James Sanders in: numpy#8303 Closes numpy#6581. Co-authored-by: James Sanders <J-Sand@users.noreply.github.com>

mattip · 2020-01-16T22:08:34Z

I am -1 on spending more effort on this now. I don't see a clear use case for adding this complexity, and it does not seem to be causing users much pain. We can revisit it if a user comes forward with a clear case where the perceived memory leak is meaningful.

seberg · 2020-01-16T22:20:43Z

OK, fair enough.

This is probably right, but not really finished up. Just pushing since it was almost finished, since if it seems like unless this comes up again, we do not care enough about fixing this lingering bug.

numpy/core/src/multiarray/arrayobject.c

charris · 2021-04-21T22:37:52Z

@seberg Needs rebase. Do you want to keep it open.

seberg · 2021-04-21T22:40:19Z

Well, if we can find someone to push it through, I am happy to rebase it. It needs a little bit of thought, since I am not quite sure about those attempts to untrack arrays that don't (really) need it – i.e. the ones that don't contain objects. Although, I know a thing or two more about it now, so maybe I can make sense of it now...

In general, I think it fixes a couple of impossible to find memory leaks even though they seem reported rarely...

seberg · 2022-04-11T17:59:41Z

This would also be nice to revive. I am not sure how much work it even is, the main thing to figure out (besides from trying to clean up the code a bit) is really how to best support subclasses.

seberg · 2022-04-11T18:05:26Z

Wrong comment: This should be revived IMO, it is a pretty big bug in most ways you can look at it. I think it could be revived almost as-is (with some thoughts on whether we should even bother to selectively enable/disable tracking and how to do it correctly).

But, it may also be that sometime soon, and with the HPy project making progress the approach should change here anyway (i.e. the current decref/incref code paths being replaced).

seberg added 01 - Enhancement triage review Issue/PR to be discussed at the next triage meeting component: numpy._core and removed triage review Issue/PR to be discussed at the next triage meeting labels Dec 6, 2019

seberg mentioned this pull request Dec 6, 2019

ENH: add cyclic GC support to object arrays. Closes #6581. #8303

Closed

seberg commented Dec 6, 2019

View reviewed changes

numpy/core/src/multiarray/refcount.c.src Outdated Show resolved Hide resolved

seberg commented Dec 6, 2019

View reviewed changes

numpy/core/src/multiarray/arrayobject.c Outdated Show resolved Hide resolved

mattip reviewed Dec 9, 2019

View reviewed changes

eric-wieser reviewed Dec 16, 2019

View reviewed changes

numpy/core/src/multiarray/iterators.c Outdated Show resolved Hide resolved

eric-wieser reviewed Dec 16, 2019

View reviewed changes

numpy/core/src/multiarray/iterators.c Outdated Show resolved Hide resolved

eric-wieser reviewed Dec 16, 2019

View reviewed changes

numpy/core/src/multiarray/nditer_pywrap.c Outdated Show resolved Hide resolved

eric-wieser reviewed Dec 16, 2019

View reviewed changes

numpy/core/src/multiarray/refcount.c.src Outdated Show resolved Hide resolved

seberg force-pushed the cyclic-gc branch 2 times, most recently from 694b4cc to 777a38d Compare January 9, 2020 23:05

mattip added the triage review Issue/PR to be discussed at the next triage meeting label Jan 13, 2020

ENH: add cyclic GC support to ndarrays containing objects

9e48160

This commit is based on work of James Sanders in: numpy#8303 Closes numpy#6581. Co-authored-by: James Sanders <J-Sand@users.noreply.github.com>

Fixup: Remove untracking and use new macros when available

69f86de

This is probably right, but not really finished up. Just pushing since it was almost finished, since if it seems like unless this comes up again, we do not care enough about fixing this lingering bug.

seberg force-pushed the cyclic-gc branch from 777a38d to 69f86de Compare January 16, 2020 22:32

eric-wieser reviewed Jan 16, 2020

View reviewed changes

numpy/core/src/multiarray/arrayobject.c Outdated Show resolved Hide resolved

numpy/core/src/multiarray/arrayobject.c Outdated Show resolved Hide resolved

seberg removed the triage review Issue/PR to be discussed at the next triage meeting label Jan 29, 2020

Small fixups to address review comments

530ec02

seberg commented Feb 1, 2020

View reviewed changes

numpy/core/src/multiarray/arrayobject.c Outdated Show resolved Hide resolved

Update numpy/core/src/multiarray/arrayobject.c

383f698

Base automatically changed from master to main March 4, 2021 02:04

ltalirz mentioned this pull request Sep 24, 2021

Memory leak when array contains circular references #6581

Open

seberg mentioned this pull request Oct 20, 2021

Port creation and destruction of ndarray objects to HPy hpyproject/hpy#252

Closed

jpivarski mentioned this pull request Jan 14, 2022

Fixed one memory leak in complex objects scikit-hep/uproot5#537

Merged

seberg closed this Apr 11, 2022

seberg added the 64 - Good Idea Inactive PR with a good start or idea. Consider studying it if you are working on a related issue. label Apr 11, 2022

ngoldbaum mentioned this pull request Nov 1, 2023

Arena allocator refactor numpy/numpy-user-dtypes#92

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add cyclic garbage collection to NumPy #15065

ENH: Add cyclic garbage collection to NumPy #15065

seberg commented Dec 6, 2019 •

edited

mattip left a comment

mattip Dec 9, 2019

seberg Dec 16, 2019

mattip Dec 9, 2019

seberg Dec 17, 2019

mattip Dec 9, 2019

mattip Dec 9, 2019

seberg Dec 10, 2019

seberg Dec 16, 2019

mattip Dec 9, 2019

seberg Dec 16, 2019

eric-wieser Dec 16, 2019

seberg Dec 16, 2019

eric-wieser Dec 16, 2019

seberg Dec 16, 2019

seberg Jan 16, 2020

eric-wieser Jan 16, 2020 •

edited

eric-wieser Jan 16, 2020

seberg Jan 16, 2020

seberg commented Jan 9, 2020

seberg commented Jan 10, 2020

mattip commented Jan 16, 2020

seberg commented Jan 16, 2020

charris commented Apr 21, 2021

seberg commented Apr 21, 2021

seberg commented Apr 11, 2022

seberg commented Apr 11, 2022

ENH: Add cyclic garbage collection to NumPy #15065

ENH: Add cyclic garbage collection to NumPy #15065

Conversation

seberg commented Dec 6, 2019 • edited

mattip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Jan 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Jan 9, 2020

seberg commented Jan 10, 2020

mattip commented Jan 16, 2020

seberg commented Jan 16, 2020

charris commented Apr 21, 2021

seberg commented Apr 21, 2021

seberg commented Apr 11, 2022

seberg commented Apr 11, 2022

seberg commented Dec 6, 2019 •

edited

eric-wieser Jan 16, 2020 •

edited