Skip to content

ENH: Use PyObject_GetOptionalAttr#27119

Merged
seberg merged 4 commits intonumpy:mainfrom
seberg:use-optional-attr
Aug 8, 2024
Merged

ENH: Use PyObject_GetOptionalAttr#27119
seberg merged 4 commits intonumpy:mainfrom
seberg:use-optional-attr

Conversation

@seberg
Copy link
Copy Markdown
Member

@seberg seberg commented Aug 6, 2024 •
edited
Loading

Now that PyObject_GetOptionalAttr is public in 3.13 and semi-public via the capi-backports... Let's use it!

This is a massive speed up for code paths that run into this. For full throttle gains, you need 3.12, because only in 3.12 were type lookups improved (I didn't test it with it).

For example:

o = object()  # some object that doesn't have __array__, or the other dunders

%timeit np.asarray(o)

will go from 800+ns to <200ns on my setup. Things like np.result_type(dtype, dtype) (dtypes hit slow lookups for __array_function__) should be at 2-3 times faster on Python 3.12 (untested as I didn't feel like bumping my Python version).

Just to give another example, a Python object that implements a no-op __array__ (i.e. returning an already wrapped array) goes from 575ns to 175ns.

Draft, because I had to base it on gh-27117 to avoid the conflict.

@seberg
Copy link
Copy Markdown
Member Author

seberg commented Aug 6, 2024

We should be using GetOptionalAttr in all places where an AttributeError is ignored. But... we also should really almost never use GetAttrStr in code branches that may be relevant for performance, so I think doing just the protocols is a nice step.

@seberg seberg marked this pull request as draft August 6, 2024 14:41
@seberg seberg force-pushed the use-optional-attr branch 4 times, most recently from 0be8647 to 1b99112 Compare August 6, 2024 19:25
@charris charris changed the title ENH: Use PyObject_GetOptionalAttr ENH: Use PyObject_GetOptionalAttr Aug 6, 2024
seberg added 3 commits August 7, 2024 15:09
@seberg
MAINT: Bump pythoncapi-compat version
e7d3776
@seberg
ENH: Use PyObject_GetOptionalAttr
cc61e95 
This uses `PyObject_GetOptionalAttr` which is backported in capi
compat.  This function significantly speeds up many attribute
lookups and thus dispatching protocols.
@seberg
DOC: Add release note snippet for attribute lookup improvement
fbd9a3b
@seberg seberg force-pushed the use-optional-attr branch from 1b99112 to fbd9a3b Compare August 7, 2024 13:09
@seberg seberg marked this pull request as ready for review August 7, 2024 13:09
@seberg
Copy link
Copy Markdown
Member Author

seberg commented Aug 7, 2024

Doesn't affect many inputs (but not few either, since a lot of objects have an __array__ attribute), but just to show that it does show up e.g. in the array-coercion benchmarks:

| Change   | Before [d6021964] <main>   | After [fbd9a3b9] <use-optional-attr>   |   Ratio | Benchmark (Parameter)                                                            |
|----------|----------------------------|----------------------------------------|---------|----------------------------------------------------------------------------------|
| -        | 1.18±0.01μs                | 508±10ns                               |    0.43 | bench_array_coercion.ArrayCoercionSmall.time_array_all_kwargs(range(0, 3))       |
| -        | 1.08±0.01μs                | 426±3ns                                |    0.4  | bench_array_coercion.ArrayCoercionSmall.time_asanyarray_dtype(range(0, 3))       |
| -        | 1.07±0.01μs                | 429±3ns                                |    0.4  | bench_array_coercion.ArrayCoercionSmall.time_asarray_dtype(range(0, 3))          |
| -        | 1.08±0.01μs                | 421±20ns                               |    0.39 | bench_array_coercion.ArrayCoercionSmall.time_array_subok(range(0, 3))            |
| -        | 1.09±0.01μs                | 417±4ns                                |    0.38 | bench_array_coercion.ArrayCoercionSmall.time_array_dtype_not_kwargs(range(0, 3)) |
| -        | 1.08±0μs                   | 412±3ns                                |    0.38 | bench_array_coercion.ArrayCoercionSmall.time_array_no_copy(range(0, 3))          |
| -        | 1.06±0μs                   | 396±5ns                                |    0.37 | bench_array_coercion.ArrayCoercionSmall.time_array(range(0, 3))                  |
| -        | 1.03±0μs                   | 386±0.2ns                              |    0.37 | bench_array_coercion.ArrayCoercionSmall.time_asanyarray(range(0, 3))             |
| -        | 1.03±0.01μs                | 383±3ns                                |    0.37 | bench_array_coercion.ArrayCoercionSmall.time_ascontiguousarray(range(0, 3))      |
| -        | 1.05±0.02μs                | 381±3ns                                |    0.36 | bench_array_coercion.ArrayCoercionSmall.time_asarray(range(0, 3))                |

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

ngoldbaum
ngoldbaum approved these changes Aug 8, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

Just one minor comment, otherwise this looks like a really nice cleanup. Didn't spot any bugs in the C refactorings.

numpy/_core/src/common/get_attr_string.h Outdated
@@ -46,22 +47,19 @@ _is_basic_python_type(PyTypeObject *tp)
*
* In future, could be made more like _Py_LookupSpecial
Copy link
Copy Markdown
Member

@ngoldbaum ngoldbaum Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

seems like you're moving the API away from _PyObject_LookupSpecial, which returns a PyObject *. Maybe delete this comment if it no longer applies?

Copy link
Copy Markdown
Member Author

@seberg seberg Aug 8, 2024

Choose a reason for hiding this comment

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

Good point, actually _PyObject_LookupSpecial is the old name for PyObject_GetOptionalAttr. But I didn't time if it makes sense to just remove the exact type-check fast path or not. So going to keep this (with the adaptation for now).

Copy link
Copy Markdown
Member Author

@seberg seberg Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(More importantly: You need Python 3.12 to get the super-fast behavior for type(...), so we actually can't fully replace it yet even if it might be fast enough for most cases at least.)

@seberg seberg merged commit 11eb606 into numpy:main Aug 8, 2024
@seberg seberg deleted the use-optional-attr branch August 8, 2024 19:24
@charris
Copy link
Copy Markdown
Member

charris commented Aug 10, 2024

I'd like to backport the pythoncapi-compat submodule to keep the 2.1 branch in sync. Any problem with that?

@ngoldbaum
Copy link
Copy Markdown
Member

ngoldbaum commented Aug 10, 2024

Nope that should be fine

@eendebakpt eendebakpt mentioned this pull request Aug 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ngoldbaum ngoldbaum ngoldbaum approved these changes

Assignees

No one assigned

Labels

01 - Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@seberg @charris @ngoldbaum