BUG: fix choose refcount leak by tylerjereddy · Pull Request #24188 · numpy/numpy · GitHub

Fixes BUG: Choose (and probably more item selection) leaks references with out= #22683
use copyswap to avoid the reference count leaking reported above when np.choose is used with out
my impression from the ticket is that Sebastian doesn't think copyswap is a perfect solution, but may suffice short-term?

could probably go less than 10 here, though I think each incantation can at least do +1

In case you iterate: Randomly realized that this actually stopped doing anything useful on Python 3.12, because IIRC the literal 1 will be an immortal object (and thus it's reference count doesn't change reliably).

Thanks, we are trying a bit to avoid copyswap; unfortunately, I admit, it would be nice if the replacement was slightly shorter, e.g. see: gh-24176

Happy to do this first, though! Is this much slower for e.g. float64? (My guess is no, because memmov is also just a function call, this loop should be specialized for the typical small itemsizes without references (i.e. basic numeric types).

I'd prefer not adding new copyswap usages if we can avoid it since it will cause seg faults with new dtypes. Happy to push a fixup here if you don't have time to look at the other PRs replacing copyswap with a single-element cast.

This PR has been revised based on reviewer feedback to replace copyswap with the more verbose approach that tolerates new dtype stuff. It is substantially harder to read, but the CI, including the new regression test, seem to be passing.

Agreed this is a lot of boilerplate. It probably makes sense to have a dtype API slot that implements a single-element copy, then you wouldn’t need to deal with the flags or the strides (i think?). You’d still need two stages to get the copy function from the dtype and possibly set up the auxdata, but we could certainly simplify this sort of thing a bunch.

Anyway, thanks for rewriting this in a more forward-compatible way while we wait on polishing out the rough edges in the new dtype API.

@seberg Sounds like this can be improved, but is it good enough for now?

Just noticed it's also missing the xfree. I am happy with just moving it out of the inner-loop, but I do think that needs to be done. I can do that though if @tylerjereddy prefers.

I did try hoisting it up last week but got segfaults, will check again today.

Latest set of changes:

* hoist the special `out` handling code out of the inner
loop (to the degree the testsuite allowed me to)

* add a missing `NPY_cast_info_xfree`

* adjust the regression test such that it fails before/passes
after on both Python 3.11 and 3.12 beta 4, to deal with
PEP 683

Thanks I added three commits to:

Deal with errors during the copy (which are not possible for NumPy dtypes copies, but in principle are possible, e.g. for Nathan's strings).
Use this always if there are references:
- Avoids making out= special, which IMO shouldn't be different really
- New dtypes don't support PyArray_INCREF() maybe they never will since the API really isn't particularly clean (and may be outright impossible for some dtypes or object arrays in alternative Python implementations).
Remove stray iterator creation (and fix to use obj rather than out for alignment, which went from irrelevant bug to a crash due to my changes).
Use memcpy rather than memmov. We check for no memory overlap, there really isn't a reason to not use it, so I think it is clearer for future iterations also (and probably faster).

Also thought it would be clearer to use dtype since it is defined anyway.

EDIT: Also rebased, due to the noblas enforcement changes hitting CI

* Fixes numpy#22683 * use `copyswap` to avoid the reference count leaking reported above when `np.choose` is used with `out` * my impression from the ticket is that Sebastian doesn't think `copyswap` is a perfect solution, but may suffice short-term?

* remove copyswap, carefully controlling the reference counting to pass the testsuite

* hoist the special `out` handling code out of the inner loop (to the degree the testsuite allowed me to) * add a missing `NPY_cast_info_xfree` * adjust the regression test such that it fails before/passes after on both Python 3.11 and 3.12 beta 4, to deal with PEP 683

Also hoist the `dtype` definition up and use it.

Not sure this makes a difference, but we check for memory overlap so `memmov` isn't necessary and if the compiler keeps the order intact, we want the `memcpy` path to be the hot one.

Looks good to me, going to merge. Thanks for pushing this through @tylerjereddy and getting it across the line @seberg!

tylerjereddy added 00 - Bug Priority: low Valid, but not for immediate attention labels Jul 15, 2023

tylerjereddy commented Jul 15, 2023

View reviewed changes

tylerjereddy force-pushed the treddy_issue_22683 branch from 0f91f7d to a1e5395 Compare July 15, 2023 16:23

ngoldbaum reviewed Jul 17, 2023

View reviewed changes

tylerjereddy force-pushed the treddy_issue_22683 branch from a1e5395 to 7ba92a4 Compare July 18, 2023 22:34

ngoldbaum approved these changes Jul 19, 2023

View reviewed changes

seberg reviewed Jul 19, 2023

View reviewed changes

Comment thread numpy/core/src/multiarray/item_selection.c

seberg requested a review from ngoldbaum July 31, 2023 10:22

seberg reviewed Jul 31, 2023

View reviewed changes

Comment thread numpy/core/src/multiarray/item_selection.c Outdated

tylerjereddy and others added 6 commits July 31, 2023 17:59

BUG: fix choose refcount leak

2f92f29

* Fixes numpy#22683 * use `copyswap` to avoid the reference count leaking reported above when `np.choose` is used with `out` * my impression from the ticket is that Sebastian doesn't think `copyswap` is a perfect solution, but may suffice short-term?

BUG: PR 24188 revisions

c4261af

* remove copyswap, carefully controlling the reference counting to pass the testsuite

MAINT: Use explicit copy path in choose based on refcheck

fd96b34

BUG: Remove unnecessary (and now also segfaulting) iterator creation

74979f3

Also hoist the `dtype` definition up and use it.

ENH: Use memcpy and swap order

c176234

Not sure this makes a difference, but we check for memory overlap so `memmov` isn't necessary and if the compiler keeps the order intact, we want the `memcpy` path to be the hot one.

seberg force-pushed the treddy_issue_22683 branch from cf32ef1 to c176234 Compare July 31, 2023 15:59

ngoldbaum mentioned this pull request Jul 31, 2023

API: deprecate undocumented functions #24154

Merged

ngoldbaum merged commit e1ad0c1 into numpy:main Jul 31, 2023

tylerjereddy deleted the treddy_issue_22683 branch July 31, 2023 19:18

ngoldbaum added the 09 - Backport-Candidate PRs tagged should be backported label Jul 31, 2023

charris mentioned this pull request Aug 3, 2023

BUG: fix choose refcount leak #24328

Merged

charris removed 09 - Backport-Candidate PRs tagged should be backported Priority: low Valid, but not for immediate attention labels Aug 3, 2023

tylerjereddy mentioned this pull request Aug 19, 2023

BUG: Segfault in trimesh's test suite with numpy 1.26.0b1 #24459

Closed

ngoldbaum mentioned this pull request Aug 21, 2023

BUG: fix NPY_cast_info error handling in choose #24484

Merged

charris mentioned this pull request Aug 24, 2023

BUG: fix NPY_cast_info error handling in choose #24524

Merged

+                  np.choose(np.zeros(10000, dtype=int), [a], out=a)
+                  np.choose(np.zeros(10000, dtype=int), [a], out=a)
+                  refc_end = sys.getrefcount(1)
+                  assert refc_end - refc_start < 10

                           }
                       }
-                      memmove(ret_data, PyArray_MultiIter_DATA(multi, mi), elsize);
+                      copyswap(ret_data, PyArray_MultiIter_DATA(multi, mi), swap, NULL);

BUG: fix choose refcount leak by tylerjereddy · Pull Request #24188 · numpy/numpy · GitHub

Uh oh!

BUG: fix choose refcount leak - #24188

BUG: fix choose refcount leak#24188ngoldbaum merged 6 commits intonumpy:mainnumpy/numpy:mainfrom tylerjereddy:treddy_issue_22683tylerjereddy/numpy:treddy_issue_22683Copy head branch name to clipboard

Conversation

tylerjereddy commented Jul 15, 2023

Uh oh!

tylerjereddy Jul 15, 2023

Choose a reason for hiding this comment

Uh oh!

seberg Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

seberg commented Jul 17, 2023

Uh oh!

ngoldbaum Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

tylerjereddy commented Jul 18, 2023

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charris commented Jul 24, 2023

Uh oh!

seberg commented Jul 25, 2023

Uh oh!

tylerjereddy commented Jul 25, 2023

Uh oh!

tylerjereddy commented Jul 25, 2023

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

ngoldbaum commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BUG: fix choose refcount leak#24188
ngoldbaum merged 6 commits into
numpy:mainfrom
tylerjereddy:treddy_issue_22683

seberg commented Jul 31, 2023 •
edited
Loading