Skip to content
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

replace PyTryFrom by splitting PyTypeInfo #3600

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

davidhewitt
Copy link
Member

This is a proposed resolution to #3516 and overall I think a positive incremental cleanup to our traits setup.

This PR splits PyTypeInfo into three traits:

  • HasPyGilRef, which can be removed in the near future when we get rid of the GIL refs, but will be useful in the short term. It defines the AsRefTarget associated type.
  • PyTypeCheck, which is used for types like PyIterator, PySequence, and PyMapping, as these can't implement PyTypeInfo as they don't have concrete type objects.
  • PyTypeInfo, which retains its existing API.

This then removes PyO3's internal dependency on PyTryFrom:

  • PyAny::downcast() and PyAnyMethods::downcast use PyTypeCheck.
  • PyAny::downcast_exact() and PyAnyMethods::downcast_exact to use PyTypeInfo.
  • PyAny::downcast_unchecked() uses HasPyGilRef. (PyAnyMethods::downcast_unchecked needs no helper trait.)

From my experience playing around with #3382, e.g. in #3531, having this adjustment makes it much easier to work with PyIterator, PySequence, and PyMapping in code which is migrating between the two APIs.

@@ -97,22 +101,6 @@ impl<'v> PyTryFrom<'v> for PyIterator {
}
}

impl Py<PyIterator> {
Copy link
Member Author

Choose a reason for hiding this comment

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

These special cases for PyIterator etc are no longer needed, because as_ref() and into_ref() are made available by the HasPyGilRef implementation for all T: HasPyGilRef.

// Safety: into_ptr produces a valid pointer to PyIterator object
unsafe { obj.py().from_owned_ptr(py2.into_ptr()) }
})
Self::from_object2(Py2::borrowed_from_gil_ref(&obj)).map(Py2::into_gil_ref)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an example of where this adjustment simplifies interchange between the gil-ref and Py2 APIs; there are more cases in #3572 which can be simplified also.

Copy link

codspeed-hq bot commented Nov 27, 2023

CodSpeed Performance Report

Merging #3600 will improve performances by ×2.2

Comparing davidhewitt:pytypecheck (ed87637) with main (93370d9)

Summary

⚡ 1 improvements
✅ 77 untouched benchmarks

Benchmarks breakdown

Benchmark main davidhewitt:pytypecheck Change
list_via_extract 273.9 ns 126.1 ns ×2.2

src/err/mod.rs Outdated
@@ -53,6 +53,7 @@ pub struct PyDowncastError<'a> {
impl<'a> PyDowncastError<'a> {
/// Create a new `PyDowncastError` representing a failure to convert the object
/// `from` into the type named in `to`.
#[cold]
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated am I am not sure it is a universally good idea. For example, what about downcast chains to identify the correct type? We steer users towards downcasting instead of extracting primarily since constructing PyDowncastError is cheaper than PyErr and this could affect this.

I am not saying this should never happen, just that I would prefer this a separate change with some measurements and discussion on its impact on reasonable code patterns.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed this was probably unnecessary, I'll revert and confirm that codspeed still thinks the diff is fine. I think the inline hints had a much greater effect than this.

I noticed recently that #[cold] doesn't prevent inlining and PGO is likely to do better than manual branch hints, so I suspect we should rarely if ever use #[cold] to be honest 👍

@@ -57,13 +75,36 @@ pub unsafe trait PyTypeInfo: Sized {
unsafe { ffi::PyObject_TypeCheck(object.as_ptr(), Self::type_object_raw(object.py())) != 0 }
}

/// Checks if `object` is an instance of this type.
/// Checks if `object` is an instance of this type or a subclass of this type.
Copy link
Member

Choose a reason for hiding this comment

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

Colour me confused. What is the difference between is_type_of and is_exact_type_of then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh sorry this is an error the new documentation is incorrect. Will revert.

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Some remarks but the change appears fundamentally right to me.

I do wonder whether this needs a migration guide entry but implementors of PyTypeInfo are probably not that common in user code?

@davidhewitt
Copy link
Member Author

Thanks for the review; I will fixup all points hopefully tomorrow. I think also there is no harm in a small migration note, we can state in it that most users will not care about the difference, though it may affect imports occasionally I guess.

@davidhewitt
Copy link
Member Author

Codspeed seems fine after removing #[cold], and the only other changes since review are docs changes so I'll proceed to merge this and rebase other stuff.

@davidhewitt davidhewitt added this pull request to the merge queue Dec 5, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 5, 2023
@davidhewitt davidhewitt added this pull request to the merge queue Dec 5, 2023
Merged via the queue into PyO3:main with commit 856c573 Dec 5, 2023
35 checks passed
@davidhewitt davidhewitt deleted the pytypecheck branch December 5, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants