PERF: Exact NDArray reduction fast path - #32041
Conversation
|
The test failure seems unrelated |
|
@ikrommyd I'd appreciate it if you could give this a review pass. Since you've been looking at the reduction code paths and getting acquainted with ufunc internals recently. |
There was a problem hiding this comment.
I'm overall happy. Left some minor styling comments only. All can be ignored and functionality will not change.
I'd personally add a performance release note too.
I don't know if any testing is required here. I'd assume the existing tests cover all these code paths.
|
Addressed all of the comments and also added a performance release note as suggested. |
ngoldbaum
left a comment
There was a problem hiding this comment.
Also just a general comment that I'd like to see more tests targeting error paths in C code. The biggest bug in this (line 740) is in an error path and C error paths tend to be buggy and poorly tested.
Don't worry about OOM paths but paths from passing the wrong kind of argument might actually be hit by users in the real world.
You also have some custom logic to construct nice error messages - those code paths also commonly are buggy and poorly tested. Try to add a test that triggers these errors and probes that the error messages are correct.
Note that most of these comments are nitpicks, feel free to disagree if you think anything I'm asking is out-of-scope.
| default: | ||
| return 0; | ||
| } | ||
| if (parsed < 0 && PyErr_ExceptionMatches(PyExc_TypeError)) { PyErr_Clear(); return 0; } |
There was a problem hiding this comment.
Rather than inferring "didn't bind" from the exception class, as a followup we could consider teaching npy_argparse a probe mode: a raise_on_mismatch flag (or npy_parse_arguments_probe macro) where the four structural failure sites return a distinct NPY_ARGPARSE_MISMATCH code instead of raising, while converter/internal errors still raise. Then this becomes if (parsed == NPY_ARGPARSE_MISMATCH) { return 0; } if (parsed < 0) { return -1; } with no PyErr_Clear() at all and no reliance on exception-class matching (which also catches TypeError subclasses from unrelated code paths). If that's too much for this PR, at minimum add a comment here stating the invariant this depends on (no explicit converters in your npy_parse_arguments uses).
There was a problem hiding this comment.
Adding a comment for now with a TODO and a link to this comment; Will follow up on this after this one is merged.
| # Keep in sync with the enum in arrayfunction_override.c | ||
| SUM_PROD = 1 | ||
| MIN_MAX = 2 | ||
| ANY_ALL = 3 |
There was a problem hiding this comment.
You should add a test that fails when/if someone ever adds a new thing to the C enum, otherwise this definitely won't get updated. I think if that happens it's possible e.g. tagging any as SUM_PROD would make np.any(a, dtype=..., initial=...) quietly start working.
There was a problem hiding this comment.
Will add a test to enforce the enum synchronization.
Tagging a function with a wrong reduction kind is possible even now. For that to happen while changing the C enum it would require reshuffling the values instead of extending it. Could have a static mapping check between ufunc and specific reduction kind but seems unnecessary.
ikrommyd
left a comment
There was a problem hiding this comment.
I don't have anything more to add, should be good from my side once Nathan's comments are addressed.
seberg
left a comment
There was a problem hiding this comment.
Thanks, yeah, LGTM to me as well. I wish there was something more straight-forward than that enum, but it's not bad at all considering how much it safes on the overhead here.
| where == npy_static_pydata._NoValue ? Py_True : where, | ||
| }; | ||
| *result = PyObject_Vectorcall(self->reduction, call_args, 7, NULL); | ||
| return 1; |
There was a problem hiding this comment.
A bit late now, but
| return 1; | |
| return *result ? 1: -1; |
Since presumably the function call can fail.
There was a problem hiding this comment.
I guess it doesn't matter given how it's used, but it feels slightly easy to mess up later like this
|
@seberg @ngoldbaum @ikrommyd This work overlaps with #31943. The approach here only handles 8 reductions (the linked PR over a 100 reductions), but this PR does optimize a bit more. Since this targets the important cases ( Do you already have an idea whether we should extend the approach here (add more reductions to the enum), go with #31943 or leave performance as is? |
|
@eendebakpt yeah, there is some overlap, so sorry about the conflict. We should try to push it through, it's something I always thought would be worthwhile (considering that it, like here, affects practically every call in NumPy a bit -- this only affects the silly reductions that need to check for a downstream object attribute). I think the fact that this side-steps the Python call fully is very worthwhile here, but maybe not something needed to extend in gh-31943 as it is specific to these reductions. So, maybe for now both are just independent things? If gh-31943 can help us figure out a way to move forward that isn't an enum for side-stepping the Python call that would be awesome, but I am not sure about how right now. |
PR summary
Adds an exact-
ndarrayfast path fornp.sum,np.prod,np.min/np.max,np.amin/np.amax, andnp.any/np.all. The__array_function__dispatcher now parses reduction arguments in C and directly invokes the correspondingufunc.reduce, avoiding Python dispatch overhead for common calls. Reduction metadata is configured through the existingarray_function_dispatchdecorator for the three supported signature families. Custom array types and arguments still use the original dispatch logic, so their override behavior is preserved.spin bench -c HEAD^ -t bench_reduce.SmallReductionshows speed-up over the previous version:This could potentially close #31098.
First time committer introduction
The patch was developed during the EuroPython sprint : )
AI Disclosure
Used
opencodefor code development. Every line is human-reviewed.