-
Notifications
You must be signed in to change notification settings - Fork 760
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
Add benchmark highlighting the costs of failed calls to FromPyObject::extract. #2279
Conversation
6311923
to
b14015c
Compare
I pushed a commit adding
So IMHO, this still seems to be a performance foot gun, especially in derived code. |
How about changing the pub trait FromPyObject<'source>: Sized {
type Error;
/// Extracts `Self` from the source `PyObject`.
fn extract(ob: &'source PyAny) -> Result<Self, Self::Error>;
} Where the Error type would be (note: if we start changing conversion traits we should probably consider all of them) |
I think we should consider this, but this could be in a maintenance release helping existing deployments without obstructing such a change to Regarding adding an associated #[derive(FromPyObject)]
struct Foo {
bar: Bar,
qux: Qux,
} must derive an |
There's a very old thread at #426 suggesting a rework. That thread is probably very out of date; it might be worth killing and starting a new discussion. For what it's worth, the idea in #1308 would also change
This kind of proposal gets a 👍 from me. Having error handling be as efficient as possible is a worthwhile goal. |
Just wanted to say that my intention for this PR was limited to provide the benchmark and a performance bugfix for 0.16.x. I don't consider closing the mentioned issue of missing "performance docs" and I think that if we want to revamp the conversion traits, we should probably reboot #426 or #1308.
I think it is of specific importance for the conversion traits as making Python functions "polymorphic" by downcast cascades which by necessity produce a lot of failed conversions is a common pattern. |
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.
Yep makes sense. I think would be helpful to put out a 0.16.4 for the coverage fix as well as #2289, so let's merge this as-is and then consider further refactoring for 0.17 later.
Please add a Changed
CHANGELOG entry documenting the performance tweak and then feel free to merge 👍
9d89056
to
10c285b
Compare
Done. Will give it some time if anyone fancies a look and merge in the afternoon. |
This was brought up in #2278 and probably affects quite a bit of code that does a case-by-case analysis of argument types:
The overhead itself mostly comes from this
conversion.
I find it especially troubling that the rather nice way of using an enum is also affected.