MAINT: Refactor clearing up of array data #22924

seberg · 2023-01-03T22:05:11Z

Replacement for gh-22692 and the "other option" here which does not reuse ArrayMethod as that seems a bit ridiculous for a single array operation.

This does a few things:

Creates a new get_clear_function() slot, similar to the typical ArrayMethod strided loop getting, but a bit reduced.
- Returns a simplified strided loop function (only one operand).
- I have included a NULL initial argument, to have space for something like a "context"...
Moves the current clearing functions that are based on the casting code out into its own file, this copies a little and moves some other things.
- Recreates some helper infrastructure from the casting one...
Introduces a new PyArray_ClearBuffer and PyArray_ClearArray and uses them.
- Effectively replaces the old DECREF API, but I didn't dare delete it since it is in principle public. We also use the single-item DECREF in a few places still.
- (The buffer version is almost unused now, after refactoring npyiter, but that seems OK)
Fixes up NpyIter to track the clear function (may never be used, but it is actually nice to have it already in case of an error).

I suspect that this should be ready (modulo cleaning up the code a bit with review). It does not yet expose this to user DTypes and a few code paths still use PyArray_Item_XDECREF which need fixing (I bet half of them are dubious anyway, but a different issue).

seberg · 2023-01-03T22:07:30Z

@ngoldbaum I don't know if you dare review this, but if you do that would be great and we could also go through together or so. I think the main point is deciding that final API is fine, the rest is tricky code, but should be OK.

ngoldbaum

The new API looks fine to me, I think. I can't think of a reason why you'd want to use the auxdata for clearing the array, but it doesn't seem harmful to allow it to be passed in if someone wants it.

Currently I think this only works with embedded references to python objects, but in principle it should work with arbitrary embedded references. Does that just mean expanding the checks to include NPY_ITEM_IS_POINTER or are there any other possible issues?

Presumably user dtypes will also need a way to set the appropriate flags to indicate that they have embedded references?

numpy/core/src/multiarray/array_method.c

numpy/core/src/multiarray/dtype_traversal.c

numpy/core/src/multiarray/dtype_traversal.h

ngoldbaum · 2023-01-04T18:28:15Z

numpy/core/src/multiarray/dtypemeta.h

+    PyArrayMethodObject *within_dtype_castingimpl;
+    /*
+     * Implementation which clears a dtype or NULL.  If not given, setting
+     * HASREF on the dtype is invalid.  We only use the loop getting, not


As far as I can see, HASREF isn't a thing, so this comment might be a bit confusing. It looks like PyDataType_REFCHK checks NPY_ITEM_REFCOUNT, so you could use that? Although maybe NPY_ITEM_IS_POINTER is invalid too?

Yeah, NPY_ITEM_REFCOUNT is it, except that we may want to rename/alias it to NPY_ITEM_HASREF or something...

To be honest, I am not sure that NPY_ITEM_IS_POINTER is ever used (or useful) except as having the identical meaning to NPY_ITEM_REFCOUNT...

Ah that makes sense. I brought up NPY_ITEM_IS_POINTER because it might be nice to indicate that a dtype uses embedded references but doesn't need the GIL.

numpy/core/src/multiarray/item_selection.c

numpy/core/src/multiarray/refcount.c

ngoldbaum · 2023-01-04T18:36:35Z

numpy/core/src/multiarray/refcount.c

+
+  The use of this function is strongly discouraged, within NumPy
+  use PyArray_Clear, which DECREF's and sets everything to NULL and can
+  work with any dtype.


Should (can?) PyArray_XDECREF be deprecated?

Probably, not sure there is a need for it to begin in reasonable code... But seems hard to do, might be a "nuke it with NumPy 2.0" thing...

ngoldbaum · 2023-01-04T18:39:19Z

numpy/core/src/multiarray/dtype_traversal.h

+ * We assume for now that this context can be just passed through in the
+ * the future (for structured dtypes).
+ */
+typedef int (simple_loop_function)(


nit: maybe these two new function signatures should be called traverse_loop_function and get_traverse_loop_function to indicate that it's a loop specifically for array traversal

Fair point, "simple" is always a bad name anyway ;).

seberg · 2023-01-05T11:18:55Z

Updated based on the comments, thanks! auxdata is already used for structured dtypes (we need to store how to unpack the struct).

It may also be useful for other things, for example sorting could re-use this pattern (at least partially). Not quite a "traversal" I guess, but needs much the same.

The incref/decref function shouldn't be able to fail (especially the decref). But right now they can, this will be fixed when we redo clearing (see numpygh-22924)

seberg · 2023-02-09T13:36:48Z

Gentle ping on this clearing of array data PR.

mattip

This could use some documentation about how users are meant to design "clear" functions, how "clear" relates to "dealloc" and "decref". Or is there documentation in one of the dtype NEPS?

numpy/core/src/multiarray/refcount.c

seberg · 2023-02-09T14:29:52Z

This could use some documentation about how users are meant to design "clear" functions, how "clear" relates to "dealloc" and "decref". Or is there documentation in one of the dtype NEPS?

True, for users I didn't add any public API yet though, so I was thinking to add that together with that. OTOH, if we want internal docs than should definitely add it here!

mattip · 2023-02-15T11:41:35Z

if we want internal docs

I would at least like a type of glossary. How does "clearing" relate to "dealloc" or "decref"?

seberg · 2023-02-15T11:59:55Z

I added it to the get_clear_loop and mention decref+NULLing in a few more places. (except the last commit, this is just a rebase).

mattip · 2023-02-15T16:22:00Z

I added it to the get_clear_loop and mention decref+NULLing

Thanks, things are "clearer" now. There are a number of code coverage warnings. Most of them seem like code that should never be reached. Do we have a way to tell coverage "ignore this"?

seberg · 2023-02-15T18:05:23Z

Do we have a way to tell coverage "ignore this"?

I am not aware, although I suppose there probably is a way. But coverage is only reported as changed so it never bothered me?

mattip

Mainly concerned about the lack of coverage, it seems the copy functions are never used? Other than that this looks good.

numpy/core/src/multiarray/array_method.c

numpy/core/src/multiarray/arrayobject.c

mattip · 2023-02-19T07:50:05Z

numpy/core/src/multiarray/dtype_transfer.c

-        if (data->decref_src.func(&data->decref_src.context,
-                &orig_src, &N, &src_stride, data->decref_src.auxdata) < 0) {
+        if (data->decref_src.func(NULL, data->decref_src.descr,
+                orig_src, N, src_stride, data->decref_src.auxdata) < 0) {


Coverage says this whole section is not hit: is that true?

Hmmm, it might be possible to hit this for structured to object (I suspect not, but I will just try to add a test). And we only have object and structured, user dtypes could (in the future) hit it, though.

mattip · 2023-02-19T07:55:34Z

numpy/core/src/multiarray/dtype_traversal.c

+
+
+/* Same as in dtype_transfer.c */
+#define NPY_LOWLEVEL_BUFFER_BLOCKSIZE  128


If it is required these be equivalent, then that should be checked or the same constant used. Otherwise the comment should be changed to indicate the value is arbitrary: "Arbitrary value like the one in dtype_transfer.c"

It is the identical use case, but doesn't need to be the same value.

mattip · 2023-02-19T07:56:38Z

numpy/core/src/multiarray/dtype_traversal.c

+    get_traverse_loop_function *get_clear = NPY_DT_SLOTS(NPY_DTYPE(dtype))->get_clear_loop;
+    if (get_clear == NULL) {
+        PyErr_Format(PyExc_RuntimeError,
+                "Internal error, tried to fetch decref/clear function for the "


Suggested change

"Internal error, tried to fetch decref/clear function for the "

"Internal error, 'get_clear_loop' not set for "

mattip · 2023-02-19T08:04:01Z

numpy/core/src/multiarray/dtype_traversal.c

+
+/* transfer data copy function */
+static NpyAuxData *
+fields_clear_data_clone(NpyAuxData *data)


There is no test of this functionality, maybe it should be marked /* untested */. Or is it tested in the userdtype repo?

This is weird internal API that is hard to use. Let me try, it may actually be possible via nditer...

mattip · 2023-02-19T08:05:31Z

numpy/core/src/multiarray/dtype_traversal.c

+    npy_intp i, field_count = d->field_count;
+
+    /* Do the traversing a block at a time for better memory caching */
+    const npy_intp blocksize = NPY_LOWLEVEL_BUFFER_BLOCKSIZE;


Are you sure this is more performant than a simpler approach?

This is the approach we use everywhere. But yes, it should make sense for cache friendliness.

mattip · 2023-02-19T08:12:40Z

numpy/core/src/multiarray/dtype_traversal.c

+
+/* transfer data copy function */
+static NpyAuxData *
+subarray_clear_data_clone(NpyAuxData *data)


This is never used?

Yes, this one actually seems unreachable, added a ~~copy~~ comment. I think I covered most of the others (but its complicated, will have to see what is actually covered).

mattip · 2023-02-19T08:16:08Z

numpy/core/src/multiarray/dtypemeta.h

+     * If the DType class does not implement `get_clear_loop` setting
+     * NPY_ITEM_REFCOUNT on its dtype instances is invalid.  Note that it is
+     * acceptable for  NPY_ITEM_REFCOUNT to inidicate references that are not
+     * Python objects.


I think we should change this to make NPY_ITEM_HASOBJECT the preferred alias, and replace PyDataType_REFCHK with PyDataType_HASOBJECT, but that can be done in a future PR since this is all internal API.

charris · 2023-02-19T17:25:56Z

close/reopen

…gic once

This removes re-using `write` for the same purpose, which seems surprisingly clean, considering that the "clearing" path was already special in any case.

Also remove accidentally auto-inserted include...

Co-authored-by: Nathan Goldbaum <[email protected]>

We simply don't bother releasing the GIL right now. This could make sense in principle for some user dtypes, though. (NumPy only ever uses clearing with Python objects so the GIL is always needed). I see no plausible use of checking floating point errors, so do not bother to add such cruft, if anyone ever needs it, they should add it.

Co-authored-by: Matti Picus <[email protected]>

We only ever use the "clone" mechanism for `nditer.copy()` but even there, it is only used for buffer transfers (not final cleanups). It appears that structured dtypes are always a single cast step which doesn't require the clearing normally (so this is hard to hit).

mattip · 2023-02-20T13:17:04Z

Let's give this a try. It might take a few iterations to find all the corner cases, maybe the userdtype repo will find some of them.

mattip · 2023-02-20T13:17:15Z

Thanks @seberg

seberg · 2023-02-20T13:25:22Z

Thanks! @ngoldbaum do you want to look into making this public? (just fishing, but it should be very easy.)

ngoldbaum · 2023-02-20T18:28:24Z

Thanks! @ngoldbaum do you want to look into making this public? (just fishing, but it should be very easy.)

Sure, I can try that out. I'll ping you if I run into trouble.

mhvk · 2023-02-25T01:13:46Z

@seberg - This PR is causing failures failures over at astropy - see astropy/astropy#14460 (and maybe pandas-dev/pandas#51589 as well, given link above; cc @lithomas1)

lithomas1 · 2023-02-25T01:56:48Z

I couldn't reproduce the segfaults on my computer, but the backtrace from our tests is is

0x00007ffff588c71b in OBJECT_copyswapn () from /home/runner/micromamba/envs/test/lib/python3.10/site-packages/numpy/core/_multiarray_umath.cpython-310-x86_64-linux-gnu.so
#0  0x00007ffff588c71b in OBJECT_copyswapn () from /home/runner/micromamba/envs/test/lib/python3.10/site-packages/numpy/core/_multiarray_umath.cpython-310-x86_64-linux-gnu.so
#1  0x00007ffff5915c72 in _new_argsortlike () from /home/runner/micromamba/envs/test/lib/python3.10/site-packages/numpy/core/_multiarray_umath.cpython-310-x86_64-linux-gnu.so
#2  0x00007ffff591a16d in PyArray_ArgSort () from /home/runner/micromamba/envs/test/lib/python3.10/site-packages/numpy/core/_multiarray_umath.cpython-310-x86_64-linux-gnu.so
#3  0x00007ffff59690bf in array_argsort () from /home/runner/micromamba/envs/test/lib/python3.10/site-packages/numpy/core/_multiarray_umath.cpython-310-x86_64-linux-gnu.so
#4  0x000055555568bd[83](https://github.com/pandas-dev/pandas/actions/runs/4262938751/jobs/7419045241#step:8:85) in cfunction_vectorcall_FASTCALL_KEYWORDS (func=func@entry=<built-in method argsort of numpy.ndarray object at remote 0x7fffb456e550>, args=0x55555ff63278, nargsf=<optimized out>, kwnames=('kind',)) at /usr/local/src/conda/python-3.10.9/Objects/methodobject.c:446

Maybe this helps a little?

I only found this PR from git blame, where it seems like the only function to touch the functions in the backtrace recently.

mhvk · 2023-02-25T02:15:12Z

In our case, the error message "RuntimeError: Internal error, tried to fetch clear function for the user dtype '�' without fields or subarray (legacy support)" is triggered, which was introduced here... I'm trying to see if this dtype actually has objects inside or not (which is what this PR addresses), but am not very familiar with this part of astropy. Anyway, maybe @seberg has an idea just given what path seems to be hit...

mhvk · 2023-02-25T02:22:43Z

p.s. The test that triggers the problem may well have an empty structured array, since it explicitly reads in a table without columns. Given an installation of astropy, a minimal example to reproduce is at astropy/astropy#14460 (comment)

mhvk · 2023-02-25T02:47:00Z

OK, found a minimal pure numpy example - see #23277

seberg · 2023-02-25T07:36:53Z

@lithomas1 thanks. Seems like I forgot to add a NULLing of the buffer when trying to simplify that code. Odd that NumPy test suite doesn't notice that.

seberg · 2023-02-25T17:18:40Z

Triggered the wheel build, so this hopefully should clear up very soon. Thanks for the reports.

mhvk · 2023-02-25T18:11:52Z

Thanks for the quick fix!

github-actions bot added the 03 - Maintenance label Jan 3, 2023

seberg mentioned this pull request Jan 3, 2023

ENH: Refactor deallocation of dtypes with embedded references #22692

Closed

seberg force-pushed the refactor-decref-meth-only branch from ecd3611 to 59bbf61 Compare January 4, 2023 10:26

ngoldbaum reviewed Jan 4, 2023

View reviewed changes

seberg force-pushed the refactor-decref-meth-only branch from 396f931 to 34440bb Compare January 6, 2023 13:16

seberg mentioned this pull request Jan 20, 2023

BUG: Stack buffer overflow in nditer_api/dtype_transfer in 1.24.x branch #22790

Closed

ngoldbaum mentioned this pull request Jan 20, 2023

ENH: Move identity to the ArrayMethod to allow customization #20970

Merged

mattip reviewed Feb 9, 2023

View reviewed changes

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

seberg force-pushed the refactor-decref-meth-only branch from aca13d1 to 479170c Compare February 9, 2023 15:03

seberg force-pushed the refactor-decref-meth-only branch from d30f667 to 10e5c99 Compare February 15, 2023 11:59

mattip reviewed Feb 19, 2023

View reviewed changes

charris closed this Feb 19, 2023

charris reopened this Feb 19, 2023

seberg added 7 commits February 19, 2023 19:52

MAINT: Refactor clear function to live on the DType

7636765

MAINT: Reorganize array dealloc and nditer buffer cleanup

2f1351b

WIP: Move traversal and simplify signature

7b15e26

WIP: Further fixups and full implementation for structured dtypes

283f36b

Fixups, but npyiter is blocking me as it actually uses the "clear" lo…

78351e3

…gic once

MAINT: Make buffer clearing explicitly stored in the iterator

a5c2edd

This removes re-using `write` for the same purpose, which seems surprisingly clean, considering that the "clearing" path was already special in any case.

BUG: Readd accidentally remove function return type

4d7c29d

Also remove accidentally auto-inserted include...

seberg and others added 6 commits February 19, 2023 19:52

MAINT: Address review comments

65340d9

Co-authored-by: Nathan Goldbaum <[email protected]>

TST: Do not use dtypemeta.h in tests, they should use "external" API

59ed6d8

DOC: Expand comment for get_clear_loop and sprinkle dealloc+nulling

8134245

Apply suggestions from code review

7f4f1ec

Co-authored-by: Matti Picus <[email protected]>

TST: Add test and comment about path that seems uncoverable

998cbb5

seberg force-pushed the refactor-decref-meth-only branch from cd9be08 to 998cbb5 Compare February 19, 2023 20:00

seberg added 3 commits February 20, 2023 09:33

TST: Add some more test coverage (but probably not the big clone funcs)

bca3155

MAINT: "comment out" unreachable subarray clear data clone

f2be9f7

seberg force-pushed the refactor-decref-meth-only branch from 967b2c8 to f2be9f7 Compare February 20, 2023 11:46

mattip merged commit af9f656 into numpy:main Feb 20, 2023

seberg deleted the refactor-decref-meth-only branch February 20, 2023 13:19

ngoldbaum mentioned this pull request Feb 22, 2023

API: expose traversal typedefs and get_clear_loop slot #23262

Merged

lithomas1 mentioned this pull request Feb 24, 2023

CI: numpy dev build seems broken pandas-dev/pandas#51589

Closed

mhvk mentioned this pull request Feb 25, 2023

EXP: Debug devdeps failure with PytestUnraisableExceptionWarning astropy/astropy#14460

Closed

mhvk mentioned this pull request Feb 25, 2023

BUG: clearing of array data too aggressive after gh-22924 #23277

Closed



		/* Same as in dtype_transfer.c */
		#define NPY_LOWLEVEL_BUFFER_BLOCKSIZE 128

	"Internal error, tried to fetch decref/clear function for the "
	"Internal error, 'get_clear_loop' not set for "

Uh oh!

MAINT: Refactor clearing up of array data #22924

MAINT: Refactor clearing up of array data #22924

Uh oh!

Conversation

seberg commented Jan 3, 2023

Uh oh!

seberg commented Jan 3, 2023

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented Jan 5, 2023

Uh oh!

seberg commented Feb 9, 2023

Uh oh!

mattip left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seberg commented Feb 9, 2023

Uh oh!

mattip commented Feb 15, 2023

Uh oh!

seberg commented Feb 15, 2023

Uh oh!

mattip commented Feb 15, 2023

Uh oh!

seberg commented Feb 15, 2023

Uh oh!

mattip left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg Feb 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

seberg Feb 19, 2023 •

edited

Loading

lithomas1 commented Feb 25, 2023 •

edited

Loading