BUG: __array_ufunc__ should always be looked up on the type, never the instance #9087

eric-wieser · 2017-05-10T12:31:14Z

This also corrects a broken short-circuit when looking up __array_ufunc__.

Doesn't build on my machine, but msvc is not giving me useful errors.

eric-wieser · 2017-05-10T13:40:05Z

numpy/core/src/private/ufunc_override.c

-    int disables;
-
-    array_ufunc = PyObject_GetAttrString(obj, "__array_ufunc__");
-    disables = (array_ufunc == Py_None);


Seemed dumb to look up the attribute again, when we already did given that "The __array_func__ attribute must already be known to exist."

eric-wieser · 2017-05-10T13:40:40Z

numpy/core/src/umath/override.c

@@ -548,15 +554,13 @@ PyUFunc_CheckOverride(PyUFuncObject *ufunc, char *method,
            goto fail;
        }

-        /* Access the override */
-        array_ufunc = PyObject_GetAttrString(override_obj,
-                                             "__array_ufunc__");


Again, why look it up a third time? Especially since here we're using a different lookup rule to the previous time

eric-wieser · 2017-05-10T14:02:13Z

For some reason the last commit is not showing up in this PR - you'll see it if you click the branch name at the bottom of the page

juliantaylor · 2017-05-10T14:08:07Z

numpy/core/src/umath/override.c

-        }
+        /* Set the self argument, since we have an unbound method */
+        Py_INCREF(override_obj);
+        PyTuple_SetItem(override_args, 0, override_obj);


why this change?

Because PyArray_LookupSpecial returns an unbound method. Arguably it should return a bound one, but that's wasteful to construct, and I wasn't able to make it work

PyObject_GetAttrString should have done that too, was it buggy before?

Yes, it was buggy because it did PyObject_GetAttrString(obj, ...) not PyObject_GetAttrString(Py_TYPE(obj), ...)

juliantaylor · 2017-05-10T14:08:49Z

numpy/core/src/private/get_attr_string.h

-    if (obj == Py_None ||
-            PyBool_Check(obj) ||
-            /* Basic number types */
+    PyTypeObject const * const known_types[] = {


this is somewhat performance relevant for small arrays, a large if will give better code than creating this array each time.

Can't the compiler optimize the array since it's const, and full of addresses to static variables?

I could add a static if it helps

no those variables may come from a dynamic loading so they are not const as far as the compiler can tell.
adding static won't work for the same reason.
You could add a static init variable, but imo just using an if statement is nicer than modifiable global state.

Ugh, that's a bit of a pain. I guess the if is not too verbose.

an if is less lines of code and a decent text editor with block and multicursor editing should handle it just as well :)

I'll wait for the tests to pass before pushing that, to see if I missed anything that my local build didn't pick up. Any other comments?

This also corrects a broken short-circuit when looking up __array_ufunc__

Previously, we would check if the attribute existed on the class, yet use it from the instance. This also cuts 3 lookups of `__array_ufunc__` down to one.

eric-wieser · 2017-05-10T14:45:32Z

Ugh, warning hell - this file is shared between multiple libraries, but each library only uses one function from it, so warnings are produced about the unused ones

+ tee warnings
+ grep -E 'warning\>'
    numpy/core/src/private/get_attr_string.h:129:1: warning: 'PyArray_LookupSpecial_OnInstance' defined but not used [-Wunused-function]
    numpy/core/src/private/get_attr_string.h:108:1: warning: 'PyArray_LookupSpecial' defined but not used [-Wunused-function]
    numpy/core/src/private/get_attr_string.h:129:1: warning: 'PyArray_LookupSpecial_OnInstance' defined but not used [-Wunused-function]
    numpy/core/src/private/get_attr_string.h:108:1: warning: 'PyArray_LookupSpecial' defined but not used [-Wunused-function]
    numpy/core/src/private/get_attr_string.h:108:1: warning: 'PyArray_LookupSpecial' defined but not used [-Wunused-function]
    numpy/core/src/private/get_attr_string.h:129:1: warning: 'PyArray_LookupSpecial_OnInstance' defined but not used [-Wunused-function]
    numpy/core/src/private/get_attr_string.h:129:1: warning: 'PyArray_LookupSpecial_OnInstance' defined but not used [-Wunused-function]
    numpy/core/src/private/get_attr_string.h:129:1: warning: 'PyArray_LookupSpecial_OnInstance' defined but not used [-Wunused-function]
    numpy/core/src/private/get_attr_string.h:129:1: warning: 'PyArray_LookupSpecial_OnInstance' defined

juliantaylor · 2017-05-10T14:54:55Z

add NPY_INLINE to the static header functions, that removes the unused warnings as no code is emitted when they are not used (c99 inline semantics we rely on in many places already)

juliantaylor · 2017-05-10T14:56:14Z

numpy/core/src/private/get_attr_string.h


-    return 0;
+        /* sentinel to swallow trailing || */
+        NPY_FALSE


just remove the trailing ||?

Could do, but it makes it much easier to reorder the above conditions, and add new ones, leaving it this way.

In particular, there's probably some small performance gain to be had by putting the most common types first.

indeed, Py_None should be at the top as in the original as it is one of the common arguments (out, axis, ...)

Good catch, I'll move that back

though that is very very minor as GCC just does all comparisons at once anyway and follows it up with some form of jump table.

In the interest of not incurring another rebase/wait-for-tests cycle, shall I leave it as is?

I'd leave it, it is not that important that it needs so much microoptimization.

juliantaylor · 2017-05-10T15:05:41Z

numpy/core/src/umath/override.c

-    int nargs = PyTuple_GET_SIZE(args);
+    npy_intp nin = ufunc->nin;
+    npy_intp nout = ufunc->nout;
+    npy_intp nargs = PyTuple_GET_SIZE(args);


changing this will emit warnings in the following format functions, they use %d
for intp you have to use NPY_INTP_FMT or cast them to int again

Maybe I should just add casts to PyTuple_GET_SIZE instead?

just leaving it int is probably fine, I'm not too worried about stuff crashing when you pass more than 32 bit arguments.

Also, it seems like either travis does not catch this type of warning, or int == intp on travis

the warning check is only done on 32 bit where int == intp, I wanted to fix that at some point ...

Can we leave fixing these remaining warnings until that time then? With this change, I get fewer warnings on MSVC, which insists on only showing warnings when an error is elsewhere

I'd rather have them fixed. I can push a commit doing so when you are otherwise done.

Ok, I think I'm otherwise done here. I've given up on making PyArray_LookupSpecial behave more like _PyObject_LookupSpecial, because it messes with our if (override == ndarray.__array_ufunc__) check in ways that I can't debug (probably comparing bound/unbound methods, or a copy being made of the function object at some level)

eric-wieser · 2017-05-10T15:25:29Z

Is there any way to make the warning travis build be first? It's maddening having to come back to this every half hour, only to find that most of the way through the test, the warning one (which isn't easy to test locally) has failed

juliantaylor · 2017-05-10T15:30:24Z

the warning test is after the regular matrix job in the.travis.yaml, I never tried switching the ordering of there. It probably works.

pv · 2017-05-10T15:31:43Z

You can reorder the runs in the matrixl: section of .travis.yml

juliantaylor · 2017-05-10T15:32:55Z

I know, these are ordered from slowest to fastest for scheduling already, but I wonder if the python: and matrix: job can be reordered.
Or we just add a warning check to the python: checks too

eric-wieser · 2017-05-10T15:54:13Z

numpy/core/src/private/ufunc_override.c

    }
    /* does the class define __array_ufunc__? */
-    cls_array_ufunc = PyArray_GetAttrString_SuppressException(
-                          (PyObject *)Py_TYPE(obj), "__array_ufunc__");


It's worth noting that before and after this patch, __array_ufunc__ would fail on old-style classes, where type(obj) is instancetype

The error is

TypeError: unsupported operand type(s) for +: 'instance' and 'instance'

Which I think is probably good enough

eric-wieser · 2017-05-10T16:19:31Z

@juliantaylor: Thanks for that commit. Tests all pass.

juliantaylor · 2017-05-10T16:19:58Z

thanks

eric-wieser · 2017-05-10T16:20:48Z

Does this fix the performance issue you raised in #9085?

juliantaylor · 2017-05-10T16:25:33Z

yes as you moved the basic check to the type object it doesn't call GetAttr anymore so CheckOverride is down to 4% of the runtime from 50%

mhvk · 2017-05-10T17:14:13Z

numpy/core/src/umath/override.c

    /* PyTuple_SET_ITEM steals reference */
-    PyTuple_SET_ITEM(override_args, 0, (PyObject *)ufunc);
+    Py_INCREF(Py_None);


Overall, thanks for putting this together. Much better! But why set override_args 0 here? It is just overwritten below anyway (and if you keep this write, wouldn't you have to write to decref None below)?

I was worried about the behaviour of PyTuple_SetItem with a NULL in its target position.

I do not need to decref None, because unlike SETITEM, SetItem handles the decref

Why not just use SET_ITEM below too?

Because that doesn't free the reference of the object previously in the tuple. To be honest, the reference counting design here was based on the case that I was sure I had reasoned correctly about, rather than picking a strategy that would require me to read the tuple source code again.

Fair enough! Indeed, while correcting, I played with changing it to SET_ITEM, and realised it became a convoluted pain of now having to decref things oneself.

mhvk · 2017-05-10T17:15:36Z

numpy/core/src/umath/override.c

-        Py_DECREF(array_ufunc);
+        /* Call the method */
+        *result = PyObject_Call(
+            override_array_ufunc, override_args, normal_kwds);


I think you need to DECREF override_array_ufunc after this, no?

Nope, they all ~~get~~ should have been cleaned up at the bottom. Wanna patch that quick?

eric-wieser · 2017-05-10T17:16:22Z

numpy/core/src/umath/override.c

@@ -580,6 +584,9 @@ PyUFunc_CheckOverride(PyUFuncObject *ufunc, char *method,
    return 0;

 fail:
+    for (i = 0; i < num_override_args; i++) {
+        Py_XDECREF(array_ufunc_methods[i]);
+    }


Ah, mistake here - this path needs to be run under both failure and success

Yes, that is probably the easiest solution...

mhvk · 2017-05-10T17:18:15Z

@eric-wieser, @juliantaylor - this is much nicer than what I had -- I had thought of storing __array_ufunc__ but felt I needed the wrapped method, and it just seemed a bit too many changes from the original __numpy_ufunc__ code.

But also a worry about a memory leak: see inline comment.

charris · 2017-05-10T17:28:35Z

I assume a fixup for this is coming?

eric-wieser · 2017-05-10T17:29:22Z

I don't have time tonight for a fixup, I'm afraid. @mhvk, can you make the fix mentioned in the inline comment?

mhvk · 2017-05-10T18:26:01Z

OK, I'll make a fixup.

mhvk · 2017-05-10T19:09:43Z

See #9092

mattip · 2017-05-31T18:28:27Z

there is a subtle problem with this code. The override_args tuple is used in the function call, but if the call returns NotImplemented, override_args is rebuilt to reflect the next attempt to call, violating the assumption that a tuple will never be modified after use. CPython cannot enforce this, the best it can do is check that refcount == 1 in PyTuple_SetItem. But PyPy does enforces this, and the code fails on PyPy. I have submitted pull request #9195 to push building override_args into the loop.

eric-wieser requested a review from juliantaylor May 10, 2017 12:33

eric-wieser force-pushed the fix-ufunc-resolution branch 2 times, most recently from 1836f00 to 260a128 Compare May 10, 2017 12:51

eric-wieser added this to the 1.13.0 release milestone May 10, 2017

eric-wieser force-pushed the fix-ufunc-resolution branch from 260a128 to 671cfdd Compare May 10, 2017 13:38

eric-wieser commented May 10, 2017

View reviewed changes

eric-wieser changed the title ~~MAINT: Distinguish "correct" special method lookups from incorrect ones~~ BUG: __array_ufunc__ should always be looked up on the type, never the instance May 10, 2017

eric-wieser force-pushed the fix-ufunc-resolution branch from 671cfdd to c190798 Compare May 10, 2017 14:02

juliantaylor reviewed May 10, 2017

View reviewed changes

eric-wieser added 2 commits May 10, 2017 15:18

MAINT: Distinguish "correct" special method lookups from incorrect ones

69a423b

This also corrects a broken short-circuit when looking up __array_ufunc__

MAINT: Fix warnings about int vs intp

ccfa36c

eric-wieser force-pushed the fix-ufunc-resolution branch from c190798 to 99789a4 Compare May 10, 2017 14:20

BUG: Fix inconsistent lookup of __array_ufunc__.

5bf2a79

Previously, we would check if the attribute existed on the class, yet use it from the instance. This also cuts 3 lookups of `__array_ufunc__` down to one.

eric-wieser force-pushed the fix-ufunc-resolution branch from 99789a4 to 5bf2a79 Compare May 10, 2017 14:25

MAINT: use if instead of loop

08b987a

juliantaylor reviewed May 10, 2017

View reviewed changes

eric-wieser force-pushed the fix-ufunc-resolution branch from bc888bd to d37fc08 Compare May 10, 2017 15:00

juliantaylor reviewed May 10, 2017

View reviewed changes

MAINT: Remove avoidable warnings

171cdee

eric-wieser force-pushed the fix-ufunc-resolution branch from d37fc08 to 171cdee Compare May 10, 2017 15:23

eric-wieser mentioned this pull request May 10, 2017

ENH: fix small reduction performance after array_ufunc #9085

Closed

MAINT: fix intp formatting warnings

8f9eeef

eric-wieser commented May 10, 2017

View reviewed changes

juliantaylor merged commit 14ff219 into numpy:master May 10, 2017

mhvk reviewed May 10, 2017

View reviewed changes

eric-wieser commented May 10, 2017

View reviewed changes

mattip mentioned this pull request May 31, 2017

BUG: pull request 9087 modifies a tuple after use #9195

Merged

BUG: __array_ufunc__ should always be looked up on the type, never the instance #9087

BUG: __array_ufunc__ should always be looked up on the type, never the instance #9087

Conversation

eric-wieser commented May 10, 2017 • edited

eric-wieser May 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented May 10, 2017 • edited

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

eric-wieser May 10, 2017 • edited

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

eric-wieser May 10, 2017 • edited

Choose a reason for hiding this comment

eric-wieser commented May 10, 2017 • edited

juliantaylor commented May 10, 2017

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

eric-wieser May 10, 2017 • edited

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

eric-wieser May 10, 2017 • edited

Choose a reason for hiding this comment

eric-wieser commented May 10, 2017

juliantaylor commented May 10, 2017

pv commented May 10, 2017 via email

juliantaylor commented May 10, 2017

eric-wieser May 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented May 10, 2017

juliantaylor commented May 10, 2017

eric-wieser commented May 10, 2017 • edited

juliantaylor commented May 10, 2017

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

eric-wieser May 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented May 10, 2017

charris commented May 10, 2017

eric-wieser commented May 10, 2017

mhvk commented May 10, 2017

mhvk commented May 10, 2017

mattip commented May 31, 2017

eric-wieser commented May 10, 2017 •

edited

eric-wieser May 10, 2017 •

edited

eric-wieser commented May 10, 2017 •

edited

eric-wieser May 10, 2017 •

edited

eric-wieser May 10, 2017 •

edited

eric-wieser commented May 10, 2017 •

edited

eric-wieser May 10, 2017 •

edited

eric-wieser May 10, 2017 •

edited

eric-wieser May 10, 2017 •

edited

eric-wieser commented May 10, 2017 •

edited

eric-wieser May 10, 2017 •

edited