ENH: Optimize and cleanup ufunc calls and ufunc CheckOverrides #15271

seberg · 2020-01-06T23:03:12Z

This is a rather large commit which clean up the ufunc override
code for argument parsing. (I may attempt to split it up)
There are two main things, argument parsing, especially for
reductions is now much faster since METH_FASTCALL is used
(when keyword arguments are being used).
By moving the argument parsing and using more generic code this
also simplifies the ufunc override checking especially when
keyword arguments are present.

Both of this decreases the argument parsing overhead quite a lot
for ufunc calls, especially for small reductions. (Without double
checking, I believe the speedup was up to around 30% for small
reductions.)

The downside is some added/annoyance due to the use of marcos to
support both FASTCALL and no FASTCALL semantics.
As a side note: vectorcall is likely not a huge factor for ufuncs
since it is very common not to use any keyword arguments. OTOH,
code that uses out=... a lot on small arrays should see a nice
difference.

Only the last commit is new, the others are changes from gh-15269 and gh-15270
@mhvk the last commit here is about what you had once looked at for the override cleanup stuff.

mhvk · 2020-01-07T16:20:01Z

Great! Would think it is useful to split up. In particular, the benchmarks should go in first, to establish a baseline.

seberg · 2021-03-18T17:07:52Z

Just to note, this one should be almost ready, but I should prod it a little bit, and test against astropy (and maybe dask) to make sure they are fine with the __array_ufunc__ changes (the changes are within the specifications, but if someone breaks those, we may have to help them out a bit).

mhvk

I had a cursory look just to see what was up, and overall it seems a really nice change. I mostly looked at the ufunc code, which is considerably simplified. Great! If it is ready for more detailed review, I can try to make some time (though perhaps just focussing on the ufunc stuff; it is a lot, though I can see it is not so easy to separate).

mhvk · 2021-03-18T18:30:48Z

numpy/core/include/numpy/ufuncobject.h

@@ -41,7 +41,7 @@ struct _tagPyUFuncObject;
 *
 * For backwards compatibility, the regular type resolution function does not
 * support auxiliary data with object semantics. The type resolution call
- * which returns a masked generic function returns a standard NpyAuxData
+ * which returns a masked generic function returns 5015a standard NpyAuxData


numpy/core/src/umath/override.c

mhvk · 2021-03-18T18:43:09Z

numpy/core/src/umath/override.c

 {
    /*
-     * ufunc.accumulate(a[, axis, dtype, out])
+     * If the keywords include sign rename to signature. An error


SHould be sig, not sign.

mhvk · 2021-03-18T19:01:22Z

numpy/core/src/umath/ufunc_object.c

+
+/*
+ * Make use of fastcall. If Python has tp_vectorcall we can use this
+ * directly, otherwise we have to unpack it.


The unpacking happens elsewhere, correct?

mhvk · 2021-03-18T19:25:26Z

numpy/core/src/common/npy_argparse.h

+ *
+ *     PyObject *argument1, *argument3;
+ *     int argument2 = -1;
+ *     if (npy_parse_arguments("method", args, len_args, kwnames),


Parenthesis after kwnames should not be there.

mhvk · 2021-03-18T20:00:58Z

numpy/core/src/common/npy_argparse.h

+ *     PyObject *argument1, *argument3;
+ *     int argument2 = -1;
+ *     if (npy_parse_arguments("method", args, len_args, kwnames),
+ *                "argument1", NULL, &argument1,


I wondered quite a bit about whether this implementation perhaps should be shared a bit with the PREPARE macro, but in the end it does seem that this is quite simple and logical, keeping keyword names, their actions, and their destinations together, etc. So, I guess I ended with a 👍

Oh, this should be in now, does it still show up in the diff? Let me rebase quickly to make it hopefully go away. (will make a note of the above comments if they disappear though.)

I tried for a while to do that, but I don't think it is possible. IIRC there was a gcc specific solution (not C99) to be able to give a return value and declare a variable.

seberg · 2021-03-18T20:30:48Z

@mhvk yeah, it would be great to move this along. Because I need to attack the ufunc code soon, and separating argument parsing from the rest of the code is pretty much a prerequisite to refactor more.

It seems I messed up with the other PRs (picking a wrong commit during rebase), the whole asarray stuff should be part of a separate PR (technically, I should not need to include that here, so let me try to rebase it away so this is less noisy for you).

seberg · 2021-03-18T20:40:59Z

Rebased with only the ufunc related changes. I will put further fixups into their own commits for clarity (I don't really expect much, but maybe an occasional refcount problem in override-paths or so).

mhvk

@seberg - this is very nice indeed!!!! I think the code is much cleaner now. My comments are basically all nitpicks, with perhaps the only one to be thought through being the one about the type of error to be raised when a tuple of outputs has the wrong size.

mhvk · 2021-03-20T15:11:25Z

numpy/core/src/umath/ufunc_object.c

+ * only in this step. This function takes the reference objects and
+ * parses them into the desired values.
+ * This function cleans up after itself and NULLs references on error,
+ * the caller has to ensure that out_op[0:nargs] is NULLed.


Add "on input" at the end (at least, that is how I understand it!)

mhvk · 2021-03-20T15:16:20Z

numpy/core/src/umath/ufunc_object.c

-    if (out_axis != NULL) {
-        *out_axis = NULL;
-    }
+
    if (out_wheremask != NULL) {


It would seem we might as well leave this to the caller as well, since we ask for it to initialize the output objects to NULL too.

mhvk · 2021-03-20T15:30:27Z

numpy/core/src/umath/ufunc_object.c

+        return -1;
+    }
+
+    PyObject *res = ufunc_generic_call_mps(ufunc, args, kwds, op);


Name *_mps is not very instructive... Maybe "*_with_operand_pointers"? (not very important, obviously!)

Renamed it to _with_operands, but you are right and I will make a PR after this to just remove the whole thing. Since nobody complained yet, I assume we can go ahead with disabling PyUFunc_GenericFunction for good.

mhvk · 2021-03-20T15:32:47Z

numpy/core/src/umath/ufunc_object.c

+{
+    if (PyTuple_CheckExact(out_obj)) {
+        if (PyTuple_GET_SIZE(out_obj) != nout) {
+            // TODO: Was ValueError (and error order changed)


Here perhaps ValueError is actually more logical? After all, the type is correct at this point (it being a tuple). Not sure. cc @eric-wieser who introduced this routine.

Trying to turn it back into a ValueError. I don't remember if I had more reason to change it (will have to see if the tests need adapting later).

mhvk · 2021-03-20T15:37:34Z

numpy/core/src/umath/ufunc_object.c

+        if (npy_parse_arguments("reduceat", args, len_args, kwnames,
+                "array", NULL, &op,
+                "indices", NULL, &indices_obj,
+                "|axis", NULL, &axes_in,


I realize this was here before, but shall we call the variable axis_obj in analogy with all the others? (Feel free to leave it as is, though!)

mhvk · 2021-03-20T17:14:08Z

numpy/core/src/umath/override.c

 {
    /*
-     * ufunc.accumulate(a[, axis, dtype, out])
+     * If the keywords include `sig` rename to `signature`. An error


Delete "An error ..." as this is done in ufuncobject.c

numpy/core/src/umath/override.c

mhvk · 2021-03-20T17:22:55Z

numpy/core/src/umath/override.c

@@ -495,104 +240,58 @@ PyUFunc_CheckOverride(PyUFuncObject *ufunc, char *method,
    }

    /*
-     * Normalize ufunc arguments.
+     * Normalize ufunc arguments, note that args does not hold any positional


This note was very confusing to me: maybe "note that any input and output arrays in args have already been stored in in_args and out_args"

The comment about len_args seems false (at least for reductions).

Hmmmpf, this probably made a bit more sense in the old (more complicated) iteration. Adopting your comment.

mhvk · 2021-03-20T17:25:30Z

numpy/core/src/umath/override.c

+    /*
+     * Reduce-like methods can pass keyword arguments also by position,
+     * in which case the additional positional arguments have to be copied
+     * into the keyword argument dictionary. The __call__ method has to


__call__ and outer methods.

mhvk · 2021-03-20T17:31:11Z

numpy/core/src/umath/ufunc_object.c

@@ -2637,6 +2273,8 @@ PyUFunc_GeneralizedFunction(PyUFuncObject *ufunc,

    /* Possibly remap axes. */
    if (axes != NULL || axis != NULL) {
+        assert((axes == NULL) || (axis == NULL));


Stupid and small, but perhaps assert(!(axes != NULL && axis != NULL))

seberg · 2021-03-23T17:09:06Z

OK, leak checker flushed out two small remaining issues (there are more issues unrelated, I had not run pytest-leaks in too long...). (Note: I rebased on main the first commits are not actually modified.)

From my side, I consider this finished now. I don't think the stricter argument parsing should be problematic.

seberg · 2021-03-23T19:46:02Z

Hmmmpf, there is a small "conflict" with gh-18670, in the outer logic. It doesn't matter much which one goes in first, but either will need rebasing to ensure a small leak doesn't reappear.

mhvk

Only some nitpicks left, really... Might be good to get another pair of eyes, just in case.

mhvk · 2021-03-23T19:19:10Z

doc/release/upcoming_changes/15271.compatibility.rst

+``__array_ufunc__``.  Previously, it was possible to pass
+on invalid arguments (such as a non-existing keyword
+argument) when dispatch was known to occur.
+This was always the intended behaviour, if existing


"This" is confusing - does it refer to what was the case previously, or what is the case now. Overall, my suggestion would be to just delete the sentence, as I think it is very unlikely someone relied on this behaviour.

mhvk · 2021-03-23T19:20:38Z

doc/release/upcoming_changes/15271.compatibility.rst

+``__array_ufunc__`` and additional positional arguments
+-------------------------------------------------------
+Previously, all positionally passed arguments were used
+where used for ``__array_ufunc__``.  In the case of reduce,


Sentence not grammatical! I think you want "where used for" -> "to check for"

mhvk · 2021-03-23T19:21:18Z

doc/release/upcoming_changes/15271.compatibility.rst

+Since this depended on the way the arguments were passed
+(by position or by keyword), NumPy will now only dispatch
+on the input and output array.
+For example, it will never dispatch on the ``where`` array.


Add "array in a reduction such as np.add.reduce."

doc/release/upcoming_changes/15271.performance.rst

mhvk · 2021-03-23T19:23:40Z

numpy/core/src/umath/override.c

+ * ufunc() and ufunc.outer() accept 'sig' or 'signature'.  We guarantee
+ * that it is passed as 'signature' by renaming 'sig' if present.
+ * Note that we have already validated that only one of them was passed
+ * before checking for checking for overrides.


Nit: double "checking" should be "before checking for overrides"

numpy/core/src/umath/override.c

mhvk · 2021-03-23T19:29:28Z

numpy/core/src/umath/ufunc_object.c

+ * and an array of (pointers to) PyArrayObjects which are NULL.
+ *
+ * 'op' is an array of at least NPY_MAXARGS PyArrayObject *.
+ */
 NPY_NO_EXPORT int
 PyUFunc_GenericFunction(PyUFuncObject *ufunc,
        PyObject *args, PyObject *kwds, PyArrayObject **op)
 {
    /* NumPy 1.19, 2020-01-24 */


OK, sounds good!

numpy/core/src/umath/ufunc_object.c

This is a rather large commit which clean up the ufunc override code for argument parsing. (I may attempt to split it up) There are two main things, argument parsing, especially for reductions is now much faster since `METH_FASTCALL` is used (when keyword arguments are being used). By moving the argument parsing and using more generic code this also simplifies the ufunc override checking especially when keyword arguments are present. Both of this decreases the argument parsing overhead quite a lot for ufunc calls, especially for small reductions. (Without double checking, I believe the speedup was up to around 30% for small reductions.) The downside is some added/annoyance due to the use of marcos to support both FASTCALL and no FASTCALL semantics. As a side note: vectorcall is likely not a huge factor for ufuncs since it is very common not to use any keyword arguments. OTOH, code that uses ``out=...`` a lot on small arrays should see a nice difference.

The TypeError TODO is also reflected in the tests I think, the other TODO seems pretty unnecessary (most users will go through `tp_vectorcall` anyway, so micro-optimizing that is not all that relevant).

This fixes the review comments by Marten, mainly small cleanups. The largest change is that it is now back to a ValueError if the out tuple has the wrong length. Co-Authored-By: Marten van Kerkwijk <mhvk@astro.utoronto.ca>

These functions do not require `NPY_NO_EXPORT`. One of them was (incorrectly) flagged by code coverage. Maybe this helps...

This is based on Marten's review again.

charris · 2021-03-23T23:27:24Z

If there are two things going on in this PR it might be best to split them.

seberg · 2021-03-23T23:40:17Z

I had not written release notes for gh-15269 and gh-18642, since they should not have any compatibility issues. But as the PRs do similar things on different part of the code-base I thought it fits together in the release notes.

Other than the release notes thing, this PR is a bit annoyingly large, but that is mostly the nature of the beast, unfortunately. There are some side battles here, but those won't be easy to split out I think.

mhvk

Thanks. To me, this looks all OK!

mhvk · 2021-03-23T23:52:19Z

doc/release/upcoming_changes/15271.compatibility.rst

+
+``__array_ufunc__`` and additional positional arguments
+-------------------------------------------------------
+Previously, all positionally passed arguments were check for


Sorry, missed a small mistake: check -> checked

Thanks. Yeah I have to remember to add [skip CI] more often. Seems it didn't pan out, because I pushed before CI was finished I guess.

mhvk · 2021-03-24T00:03:09Z

p.s. Minus the last spelling mistake, that is. Note that skipping CI on actions is now possible, by adding [skip CI] to the commit message - see https://github.blog/changelog/2021-02-08-github-actions-skip-pull-request-and-push-workflows-with-skip-ci/

[skip CI]

charris · 2021-03-24T21:27:09Z

Thanks Sebastian.

seberg force-pushed the splitup-faster-argparsing branch 2 times, most recently from 383139c to 261f7bf Compare January 7, 2020 05:03

charris added 01 - Enhancement 03 - Maintenance 25 - WIP component: numpy._core labels Jan 8, 2020

charris changed the title ~~ENH: Optimize and cleanup ufunc calls and ufunc CheckOverrides~~ WIP, ENH: Optimize and cleanup ufunc calls and ufunc CheckOverrides Jan 8, 2020

seberg mentioned this pull request Jun 3, 2020

Numpy square does not acccept unsigned integer dtype #16488

Closed

seberg force-pushed the splitup-faster-argparsing branch from 261f7bf to cdb54ba Compare December 6, 2020 05:24

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

seberg force-pushed the splitup-faster-argparsing branch 4 times, most recently from ceb04e3 to f85ae19 Compare March 18, 2021 01:46

seberg force-pushed the splitup-faster-argparsing branch from f85ae19 to 3c73f86 Compare March 18, 2021 02:02

mhvk reviewed Mar 18, 2021

View reviewed changes

seberg force-pushed the splitup-faster-argparsing branch 2 times, most recently from 7ca285b to a701a62 Compare March 18, 2021 20:25

seberg force-pushed the splitup-faster-argparsing branch from a701a62 to 51a17ca Compare March 18, 2021 20:39

seberg marked this pull request as ready for review March 18, 2021 20:39

seberg added 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes and removed 25 - WIP labels Mar 18, 2021

mhvk reviewed Mar 20, 2021

View reviewed changes

github-actions bot added the 25 - WIP label Mar 22, 2021

seberg removed the 25 - WIP label Mar 23, 2021

seberg changed the title ~~WIP, ENH: Optimize and cleanup ufunc calls and ufunc CheckOverrides~~ ENH: Optimize and cleanup ufunc calls and ufunc CheckOverrides Mar 23, 2021

seberg removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Mar 23, 2021

mhvk reviewed Mar 23, 2021

View reviewed changes

seberg force-pushed the splitup-faster-argparsing branch from b334712 to d4ad09e Compare March 23, 2021 20:49

This was referenced Mar 23, 2021

TST: __array_ufunc__ cannot be overridden on axis, etc. #9107

Closed

MAINT: post __array_ufunc__ merger to-do list #8892

Closed

seberg force-pushed the splitup-faster-argparsing branch from d4ad09e to fe40b15 Compare March 23, 2021 21:14

seberg and others added 9 commits March 23, 2021 18:10

DOC: Fixup comments based on review and remove TODO

ac100c8

The TypeError TODO is also reflected in the tests I think, the other TODO seems pretty unnecessary (most users will go through `tp_vectorcall` anyway, so micro-optimizing that is not all that relevant).

DOC: Fixup comment style for now

3731478

MAINT: Address comments by Marten

b33cf66

This fixes the review comments by Marten, mainly small cleanups. The largest change is that it is now back to a ValueError if the out tuple has the wrong length. Co-Authored-By: Marten van Kerkwijk <mhvk@astro.utoronto.ca>

DOC: Add release notes for ufunc (and FASTCALL) refactor

ed0ebd7

BUG: Add missing decref to dtype parsing

ace44ff

BUG: Add missing static leading to a (harmless) ref-leak

d2a6082

MAINT: Declare functions static rather than NPY_NO_EXPORT

4b4b6ef

These functions do not require `NPY_NO_EXPORT`. One of them was (incorrectly) flagged by code coverage. Maybe this helps...

DOC: Fixup ufunc-argparse release notes and code comments

605be4f

This is based on Marten's review again.

seberg force-pushed the splitup-faster-argparsing branch from fe40b15 to a1fc8c4 Compare March 23, 2021 23:20

seberg added 2 commits March 23, 2021 18:34

BUG: Fix bug(s) introduced due to rebase/merge conflict

0b61aba

DOC: Link all three PRs and use the first PR as the "base" (filename)

10ee4e0

seberg force-pushed the splitup-faster-argparsing branch from a1fc8c4 to 10ee4e0 Compare March 23, 2021 23:34

mhvk approved these changes Mar 23, 2021

View reviewed changes

DOC: Fix spelling in release note

bebf5c8

[skip CI]

seberg mentioned this pull request Mar 24, 2021

MAINT: Implement new style promotion for np.result_type, etc. #18676

Merged

charris merged commit 74e5bff into numpy:main Mar 24, 2021

seberg deleted the splitup-faster-argparsing branch March 24, 2021 21:44

ENH: Optimize and cleanup ufunc calls and ufunc CheckOverrides #15271

ENH: Optimize and cleanup ufunc calls and ufunc CheckOverrides #15271

Conversation

seberg commented Jan 6, 2020

mhvk commented Jan 7, 2020

seberg commented Mar 18, 2021

mhvk 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

seberg commented Mar 18, 2021

seberg commented Mar 18, 2021

mhvk 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

seberg commented Mar 23, 2021

seberg commented Mar 23, 2021

mhvk 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

charris commented Mar 23, 2021

seberg commented Mar 23, 2021

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented Mar 24, 2021

charris commented Mar 24, 2021