-
Notifications
You must be signed in to change notification settings - Fork 763
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.
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
Rework PyAny::is_instance_of
for performance
#2881
Conversation
Given the proximity to releasing 0.18 and that there's a couple of design choices / refinements to add here, I'm going to park this until after 0.18 is out. |
Idea looks great, thanks so much. But I agree, no massive hurry. |
8ffeecc
to
eab3aba
Compare
eab3aba
to
248230b
Compare
Rebased, changed The benchmark added in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benchmark added in bench_any is about 30% faster for these changes compared to main. It's nice, though I'm not entirely sure it's worth the churn.
Considering the source of the churn is an improvement IMHO (dropping the Result
layer) and that idiomatic Python API will often check types before committing to an implementation due the high cost of error/exception handling, I would say this is worth it.
Hot paths would probably still benefit from look-up tables as discussed in #3104 but this could give a good boost across the ecosystem where the LUT approach would be inappropriately heavy-weight.
Ok, that's a good argument in favour. Let's ship it! bors r=adamreichold |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
I was talking to @samuelcolvin about the fastest way to identify object types (relevant e.g. for
pythonize
and alsopydantic-core
) and noticed thatPyAny::is_instance_of
is quite unoptimised because it expands to an ffi call toPyObject_IsInstance
.This PR proposes
PyAny::is_instance_of::<T>(obj)
is changed to be equivalent toT::is_type_of(obj)
, plus add a sprinkling of inlining. We often have implementations such asPyDict_Check
which can pretty much be optimised away to just checking a bit on the type object.The accompanying benchmark to run through a bunch of object types is approx 40% faster after making this change.
For completeness, I've also added
PyAny::is_exact_instance
andPyAny::is_exact_instance_of
, to pair withT::is_exact_type_of
. (This could be split into a separate PR if preferred.)