skip type check of method receivers where unnecessary #4026
Conversation
|
According to codspeed, this is a 5% improvement in the method call benchmarks: Is it worth the complexity / lack of check? Maybe. |
| U: TryFrom<BoundRef<'a, 'py, T>>, | ||
| U::Error: Into<PyErr>, | ||
| { | ||
| U::try_from(BoundRef::ref_from_ptr(py, obj).downcast::<T>()?).map_err(Into::into) |
There was a problem hiding this comment.
The extra let obj binding would be worth the readability improvement here as well IMHO.
| pub unsafe fn receive_pyclass<'a, 'py: 'a, T: PyClass>( | ||
| py: Python<'py>, | ||
| obj: &'a *mut ffi::PyObject, | ||
| holder: &'a mut Option<PyRef<'py, T>>, |
There was a problem hiding this comment.
Doesn't this already imply 'py: 'a or is what requires you to add the bound?
There was a problem hiding this comment.
I quite like the controlled approach using AssumeCorrectReceiverType which also carries documentation nicely targeted and think this would be worth it. I would be grateful for a test case or two involving the binary-operator-based type confusion though.
We should be prepared for bug reports though as I expect us to get it wrong at least in some place or there being version- or implementation-dependent behaviour for which we have yet to account.
Agreed, I wondered whether to even make it
I think so too; as well as more thorough testing I'd like to dig deeper into the binary operators further. Also given the risk to introduce bugs I'm feeling like I'll leave this for 0.22 where this sort of adjustment seems more reasonable than a 0.21.1 patch release. I'll try to get back to this in the coming days, will leave in draft for now... |
Attempt to fix #3843
The idea is that the Python interpreter already type-checks the
selfpassed for many methods and slots, so we don't need to do this for receivers where we know the interpreter does the check.There is precedent in CPython for this assumption to be used, for example
tuple.__hash__()assumes it gets a tuple object: https://github.com/python/cpython/blob/bfc57d43d8766120ba0c8f3f6d7b2ac681a81d8a/Objects/tupleobject.c#L321Similarly for tuple methods such as
tuple.index(): https://github.com/python/cpython/blob/bfc57d43d8766120ba0c8f3f6d7b2ac681a81d8a/Objects/clinic/tupleobject.c.h#L23The only case where this seemed to be problematic was in the binary operators, where they can be called in reverse and that breaks the assumption that the type is correct. This might be our fault inside the implementation, so possibly something to check.
I thought that first I'd see how this implementation affects benchmarks...