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

NonNull pointer for Py, PyObject #260

Merged
merged 1 commit into from
Nov 11, 2018
Merged

NonNull pointer for Py, PyObject #260

merged 1 commit into from
Nov 11, 2018

Conversation

ijl
Copy link
Contributor

@ijl ijl commented Nov 7, 2018

This feels marginally safer and the combination of NonNull and inlines yield a small performance improvement in a library I have.

}

/// Creates a `PyObject` instance for the given FFI pointer.
/// Panics if the pointer is `null`.
/// Undefined behavior if the pointer is invalid.
#[inline]
pub unsafe fn from_owned_ptr_or_panic(py: Python, ptr: *mut ffi::PyObject) -> PyObject {
pub unsafe fn from_owned_ptr_or_panic(_py: Python, ptr: *mut ffi::PyObject) -> PyObject {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the unused _py arg leads to pyfunction macro breaking in a way I'm not familiar enough to address now.

Copy link
Member

Choose a reason for hiding this comment

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

It's because you need to have the GIL acquired or the python c api calls done by panic_after_error and the debug version of from_owned_ptr are undefined behavior.

src/instance.rs Show resolved Hide resolved
src/instance.rs Outdated
@@ -212,12 +213,14 @@ where

/// Specialization workaround
trait AsPyRefDispatch<T: PyTypeInfo>: ToPyPointer {
#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

Is it really necessary?

@kngwyu
Copy link
Member

kngwyu commented Nov 7, 2018

Great, thanks!
But, personally I feel this PR includes some unnecessary changes.

@konstin
Copy link
Member

konstin commented Nov 8, 2018

In general I like the idea of using NonNull. It should however use NonNull::new(ptr) with some kind of match instead of if ptr.is_null() { /* */ } else { NonNull::new_unchecked(ptr)) }.

Do have benchmarks for the inline annotations? I'm reluctant to sprinkling #[inline} all over the code, especially since llvm also does some inlining when optimizing.

@ijl
Copy link
Contributor Author

ijl commented Nov 8, 2018

I've replaced the ptr.is_null() checks with matches on the output of NonNull::new.

I've removed the inlines rather than worry about it, except on as_ref, where it's consistent with existing usages on PyObject and Py, and is needed for cross-crate inlining.

@konstin
Copy link
Member

konstin commented Nov 11, 2018

Thank you, look good now!

@konstin konstin merged commit ef68b22 into PyO3:master Nov 11, 2018
@ijl ijl deleted the nonnull branch November 11, 2018 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants