8000 ENH: Remove NpyIter_Close by mattip · Pull Request #11376 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: Remove NpyIter_Close #11376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 20, 2018
Merged

ENH: Remove NpyIter_Close #11376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82bc85d
ENH: Remove NpyIter_Close
mattip Jun 18, 2018
7349468
fixes from review
mattip Jun 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions doc/release/1.15.0-notes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ Deprecations
* Users of ``nditer`` should use the nditer object as a context manager
anytime one of the iterator operands is writeable, so that numpy can
manage writeback semantics, or should call ``it.close()``. A
`RuntimeWarning` will be emitted otherwise in these cases. Users of the C-API
should call ``NpyIter_Close`` before ``NpyIter_Deallocate``.
`RuntimeWarning` may be emitted otherwise in these cases.

* The ``normed`` argument of ``np.histogram``, deprecated long ago in 1.6.0,
now emits a ``DeprecationWarning``.
Expand Down Expand Up @@ -170,9 +169,6 @@ in the user guide.
C API changes
=============

* ``NpyIter_Close`` has been added and should be called before
``NpyIter_Deallocate`` to resolve possible writeback-enabled arrays.

* Functions ``npy_get_floatstatus_barrier`` and ``npy_clear_floatstatus_barrier``
have been added and should be used in place of the ``npy_get_floatstatus``and
``npy_clear_status`` functions. Optimizing compilers like GCC 8.1 and Clang
Expand Down
32 changes: 7 additions & 25 deletions doc/source/reference/c-api.iterator.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ number of non-zero elements in an array.
/* Increment the iterator to the next inner loop */
} while(iternext(iter));

NpyIter_Close(iter) /* best practice, not strictly required in this case */
NpyIter_Deallocate(iter);

return nonzero_count;
Expand Down Expand Up @@ -195,7 +194,6 @@ is used to control the memory layout of the allocated result, typically
ret = NpyIter_GetOperandArray(iter)[1];
Py_INCREF(ret);

NpyIter_Close(iter);
if (NpyIter_Deallocate(iter) != NPY_SUCCEED) {
Py_DECREF(ret);
return NULL;
Expand Down Expand Up @@ -495,7 +493,7 @@ Construction and Destruction
per operand. Using ``NPY_ITER_READWRITE`` or ``NPY_ITER_WRITEONLY``
for a user-provided operand may trigger `WRITEBACKIFCOPY``
semantics. The data will be written back to the original array
when ``NpyIter_Close`` is called.
when ``NpyIter_Deallocate`` is called.

.. c:var:: NPY_ITER_COPY

Expand All @@ -507,13 +505,13 @@ Construction and Destruction

Triggers :c:data:`NPY_ITER_COPY`, and when an array operand
is flagged for writing and is copied, causes the data
in a copy to be copied back to ``op[i]`` when ``NpyIter_Close`` is
called.
in a copy to be copied back to ``op[i]`` when
``NpyIter_Deallocate`` is called.

If the operand is flagged as write-only and a copy is needed,
an uninitialized temporary array will be created and then copied
to back to ``op[i]`` on calling ``NpyIter_Close``, instead of doing
the unnecessary copy operation.
to back to ``op[i]`` on calling ``NpyIter_Deallocate``, instead of
doing the unnecessary copy operation.

.. c:var:: NPY_ITER_NBO
.. c:var:: NPY_ITER_ALIGNED
Expand Down Expand Up @@ -709,9 +707,7 @@ Construction and Destruction
the functions will pass back errors through it instead of setting
a Python exception.

:c:func:`NpyIter_Deallocate` must be called for each copy. One call to
:c:func:`NpyIter_Close` is sufficient to trigger writeback resolution for
all copies since they share buffers.
:c:func:`NpyIter_Deallocate` must be called for each copy.

.. c:function:: int NpyIter_RemoveAxis(NpyIter* iter, int axis)``

Expand Down Expand Up @@ -763,23 +759,9 @@ Construction and Destruction

Returns ``NPY_SUCCEED`` or ``NPY_FAIL``.

.. c:function:: int NpyIter_Close(NpyIter* iter)

Resolves any needed writeback resolution. Should be called before
:c:func::`NpyIter_Deallocate`. After this call it is not safe to use the operands.
When using :c:func:`NpyIter_Copy`, only one call to :c:func:`NpyIter_Close`
is sufficient to resolve any writebacks, since the copies share buffers.

Returns ``0`` or ``-1`` if unsuccessful.

.. c:function:: int NpyIter_Deallocate(NpyIter* iter)

Deallocates the iterator object.

:c:func:`NpyIter_Close` should be called before this. If not, and if
writeback is needed, it will be performed at this point in order to maintain
backward-compatibility with older code, and a deprecation warning will be
emitted. Old code should be updated to call `NpyIter_Close` beforehand.
Deallocates the iterator object and resolves any needed writebacks.

Returns ``NPY_SUCCEED`` or ``NPY_FAIL``.

Expand Down
3 changes: 1 addition & 2 deletions numpy/core/code_generators/cversions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@
0x0000000b = edb1ba83730c650fd9bc5772a919cda7

# Version 12 (NumPy 1.14) Added PyArray_ResolveWritebackIfCopy,
# Version 12 (NumPy 1.15) No change.
# PyArray_SetWritebackIfCopyBase and deprecated PyArray_SetUpdateIfCopyBase.
0x0000000c = a1bc756c5782853ec2e3616cf66869d8

# Version 13 (NumPy 1.15) Added NpyIter_Close
0x0000000d = 4386e829d65aafce6bd09a85b142d585
2 changes: 0 additions & 2 deletions numpy/core/code_generators/numpy_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,6 @@
'PyArray_ResolveWritebackIfCopy': (302,),
'PyArray_SetWritebackIfCopyBa 6D40 se': (303,),
# End 1.14 API
'NpyIter_Close': (304,),
# End 1.15 API
}

ufunc_types_api = {
Expand Down
4 changes: 2 additions & 2 deletions numpy/core/setup_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@
# 0x0000000a - 1.12.x
# 0x0000000b - 1.13.x
# 0x0000000c - 1.14.x
# 0x0000000d - 1.15.x
C_API_VERSION = 0x0000000d
# 0x0000000c - 1.15.x
C_API_VERSION = 0x0000000c

class MismatchCAPIWarning(Warning):
pass
Expand Down
73 changes: 0 additions & 73 deletions numpy/core/src/multiarray/_multiarray_tests.c.src
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1041,76 +1041,6 @@ test_nditer_too_large(PyObject *NPY_UNUSED(self), PyObject *args) {
return NULL;
}

static PyObject *
test_nditer_writeback(PyObject *NPY_UNUSED(ignored), PyObject *args, PyObject *kwds)
{
/* like npyiter_init */
PyObject *op_in = NULL, *op_dtypes_in = NULL, *value = NULL;
PyArrayObject * opview;
int iop, nop = 0;
PyArrayObject *op[NPY_MAXARGS];
npy_uint32 flags = 0;
NPY_ORDER order = NPY_KEEPORDER;
NPY_CASTING casting = NPY_EQUIV_CASTING;
npy_uint32 op_flags[NPY_MAXARGS];
PyArray_Descr *op_request_dtypes[NPY_MAXARGS];
int retval;
unsigned char do_close;
int buffersize = 0;
NpyIter *iter = NULL;
static char *kwlist[] = {"value", "do_close", "input", "op_dtypes", NULL};

if (!PyArg_ParseTupleAndKeywords(args, kwds,
"ObO|O:test_nditer_writeback", kwlist,
&value,
&do_close,
&op_in,
&op_dtypes_in)) {
return NULL;
}
/* op and op_flags */
if (! PyArray_Check(op_in)) {
return NULL;
}
nop = 1;
op[0] = (PyArrayObject*)op_in;
op_flags[0] = NPY_ITER_READWRITE|NPY_ITER_UPDATEIFCOPY;

/* Set the dtypes */
for (iop=0; iop<nop; iop++) {
PyObject *dtype = PySequence_GetItem(op_dtypes_in, iop);
PyArray_DescrConverter2(dtype, &op_request_dtypes[iop]);
}

iter = NpyIter_AdvancedNew(nop, op, flags, order, casting, op_flags,
op_request_dtypes,
-1, NULL, NULL,
buffersize);
if (iter == NULL) {
goto fail;
}

opview = NpyIter_GetIterView(iter, 0);
retval = PyArray_FillWithScalar(opview, value);
Py_DECREF(opview);
if (retval < 0) {
NpyIter_Deallocate(iter);
return NULL;
}
if (do_close != 0) {
NpyIter_Close(iter);
}
NpyIter_Deallocate(iter);
Py_RETURN_NONE;

fail:
for (iop = 0; iop < nop; ++iop) {
Py_XDECREF(op[iop]);
Py_XDECREF(op_request_dtypes[iop]);
}
return NULL;
}

static PyObject *
array_solve_diophantine(PyObject *NPY_UNUSED(ignored), PyObject *args, PyObject *kwds)
{
Expand Down Expand Up @@ -1948,9 +1878,6 @@ static PyMethodDef Multiarray_TestsMethods[] = {
{"test_nditer_too_large",
test_nditer_too_large,
METH_VARARGS, NULL},
{"test_nditer_writeback",
(PyCFunction)test_nditer_writeback,
METH_VARARGS | METH_KEYWORDS, NULL},
{"solve_diophantine",
(PyCFunction)array_solve_diophantine,
METH_VARARGS | METH_KEYWORDS, NULL},
Expand Down
3 changes: 1 addition & 2 deletions numpy/core/src/multiarray/arrayobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,7 @@ array_dealloc(PyArrayObject *self)
{
char const * msg = "WRITEBACKIFCOPY detected in array_dealloc. "
" Required call to PyArray_ResolveWritebackIfCopy or "
"PyArray_DiscardWritebackIfCopy is missing. This could also "
"be caused by using a nditer without a context manager";
"PyArray_DiscardWritebackIfCopy is missing.";
Py_INCREF(self); /* hold on to self in next call since if
* refcount == 0 it will recurse back into
*array_dealloc
Expand Down
2 changes: 0 additions & 2 deletions numpy/core/src/multiarray/einsum.c.src
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2857,7 +2857,6 @@ PyArray_EinsteinSum(char *subscripts, npy_intp nop,

iternext = NpyIter_GetIterNext(iter, NULL);
if (iternext == NULL) {
NpyIter_Close(iter);
NpyIter_Deallocate(iter);
Py_DECREF(ret);
goto fail;
Expand All @@ -2881,7 +2880,6 @@ PyArray_EinsteinSum(char *subscripts, npy_intp nop,
}

finish:
NpyIter_Close(iter);
NpyIter_Deallocate(iter);
for (iop = 0; iop < nop; ++iop) {
Py_DECREF(op[iop]);
Expand Down
60 changes: 19 additions & 41 deletions numpy/core/src/multiarray/nditer_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,47 +1380,6 @@ NpyIter_GetInnerLoopSizePtr(NpyIter *iter)
}
}

/*NUMPY_API
* Resolves all writebackifcopy scratch buffers, not safe to use iterator
* operands after this call, in this iterator as well as any copies.
* Returns 0 on success, -1 on failure
*/
NPY_NO_EXPORT int
NpyIter_Close(NpyIter *iter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the code below need to move into NpyIter_Dealloc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it now lives inside NpyIter_Deallc. Comment expanded to emphasize that.

{
int ret=0, iop, nop;
PyArrayObject ** operands;
npyiter_opitflags *op_itflags;
if (iter == NULL) {
return 0;
}
nop = NIT_NOP(iter);
operands = NIT_OPERANDS(iter);
op_itflags = NIT_OPITFLAGS(iter);
/* If NPY_OP_ITFLAG_HAS_WRITEBACK flag set on operand, resolve it.
* If the resolution fails (should never happen), continue from the
* next operand and discard the writeback scratch buffers, and return
* failure status
*/
for (iop=0; iop<nop; iop++) {
if (op_itflags[iop] & NPY_OP_ITFLAG_HAS_WRITEBACK) {
op_itflags[iop] &= ~NPY_OP_ITFLAG_HAS_WRITEBACK;
if (PyArray_ResolveWritebackIfCopy(operands[iop]) < 0) {
ret = -1;
iop++;
break;
}
}
}
for (; iop<nop; iop++) {
if (op_itflags[iop] & NPY_OP_ITFLAG_HAS_WRITEBACK) {
op_itflags[iop] &= ~NPY_OP_ITFLAG_HAS_WRITEBACK;
PyArray_DiscardWritebackIfCopy(operands[iop]);
}
}
return ret;
}

/*NUMPY_API
* For debugging
*/
Expand Down Expand Up @@ -2830,4 +2789,23 @@ npyiter_checkreducesize(NpyIter *iter, npy_intp count,
}
return count * (*reduce_innersize);
}

NPY_NO_EXPORT npy_bool
npyiter_has_writeback(NpyIter *iter)
Copy link
Member
@eric-wieser eric-wieser Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this function, which traverses the flags a second time - can you just add an extra return value from NpyIter_Dealloc?

Perhaps most simple would be to rename it to

// internal helper
NpyIter_Dealloc_int(NpyIter *iter, npy_bool *did_writeback) {
    // as before, but set did_writeback
}
/* NUMPY_API */
NpyIter_Dealloc(NpyIter *iter) {
    npy_bool did_writeback;
    return NpyIter_Dealloc_int(iter, &did_writeback);
}

Or perhaps even NpyIter_Dealloc_int(iter, warn_if_writeback_needed=False), and have it raise the warning itself

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had considered that, but this API seems cleaner to me. Only the rarely used python nditer needs the warning, where the C NpyIter is used very frequently. The more commonly used path should be as clean as possible. A modified NpyIter_Dealloc would go through a second function call and would contain extra logic even for the fast path, where for nditer the extra call and flag traversal should be a small part of the GC cleanup when tp_dealloc is called.

We could revisit this idea in the future when the dust has settled around nditer use, it is an internal refactoring of the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fear with the current approach is that it separates the check of whether cleanup is needed from the actual code doing the cleanup.

a second function call

It wouldn't surprise me if the compiler just inlined the outer function - assuming that we're using the internal function definition, rather than the external function pointer.

We could revisit this idea in the future

True, this PR as is is already a nice improvement

{
int iop, nop;
npyiter_opitflags *op_itflags;
if (iter == NULL) {
return 0;
}
nop = NIT_NOP(iter);
op_itflags = NIT_OPITFLAGS(iter);

for (iop=0; iop<nop; iop++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit for future reference: should be spaces a round = and <. Not going to make you fixup this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be fixed up online before merge.

if (op_itflags[iop] & NPY_OP_ITFLAG_HAS_WRITEBACK) {
return NPY_TRUE;
}
}
return NPY_FALSE;
}
#undef NPY_ITERATOR_IMPLEMENTATION_CODE
23 changes: 18 additions & 5 deletions numpy/core/src/multiarray/nditer_constr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,6 @@ NpyIter_AdvancedNew(int nop, PyArrayObject **op_in, npy_uint32 flags,
*/
if (!npyiter_allocate_arrays(iter, flags, op_dtype, subtype, op_flags,
op_itflags, op_axes)) {
NpyIter_Close(iter);
NpyIter_Deallocate(iter);
return NULL;
}
Expand Down Expand Up @@ -465,14 +464,12 @@ NpyIter_AdvancedNew(int nop, PyArrayObject **op_in, npy_uint32 flags,
/* If buffering is set without delayed allocation */
if (itflags & NPY_ITFLAG_BUFFER) {
if (!npyiter_allocate_transfer_functions(iter)) {
NpyIter_Close(iter);
NpyIter_Deallocate(iter);
return NULL;
}
if (!(itflags & NPY_ITFLAG_DELAYBUF)) {
/* Allocate the buffers */
if (!npyiter_allocate_buffers(iter, NULL)) {
NpyIter_Close(iter);
NpyIter_Deallocate(iter);
return NULL;
}
Expand Down Expand Up @@ -654,6 +651,8 @@ NpyIter_Deallocate(NpyIter *iter)
int iop, nop;
PyArray_Descr **dtype;
PyArrayObject **object;
npyiter_opitflags *op_itflags;
npy_bool resolve = 1;

if (iter == NULL) {
return NPY_SUCCEED;
Expand All @@ -663,6 +662,7 @@ NpyIter_Deallocate(NpyIter *iter)
nop = NIT_NOP(iter);
dtype = NIT_DTYPES(iter);
object = NIT_OPERANDS(iter);
op_itflags = NIT_OPITFLAGS(iter);

/* Deallocate any buffers and buffering data */
if (itflags & NPY_ITFLAG_BUFFER) {
Expand Down Expand Up @@ -691,15 +691,28 @@ NpyIter_Deallocate(NpyIter *iter)
}
}

/* Deallocate all the dtypes and objects that were iterated */
/*
* Deallocate all the dtypes and objects that were iterated and resolve
* any writeback buffers created by the iterator
*/
for(iop = 0; iop < nop; ++iop, ++dtype, ++object) {
if (op_itflags[iop] & NPY_OP_ITFLAG_HAS_WRITEBACK) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with how dtype and object are used below, you could use *op_itflags and increment it along with the others.

Probably not worth it though - if anything I find your approach clearer.

if (resolve && PyArray_ResolveWritebackIfCopy(*object) < 0) {
resolve = 0;
}
else {
PyArray_DiscardWritebackIfCopy(*object);
}
}
Py_XDECREF(*dtype);
Py_XDECREF(*object);
}

/* Deallocate the iterator memory */
PyObject_Free(iter);

if (resolve == 0) {
return NPY_FAIL;
}
return NPY_SUCCEED;
}

Expand Down
Loading
0