From ff6fb5dcc2b09150d3f00239b26a34aeb3c5c20f Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 22 Dec 2021 00:27:13 +0000 Subject: [PATCH 1/6] benches: add bench_frompyobject --- Cargo.toml | 4 ++++ benches/bench_frompyobject.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 benches/bench_frompyobject.rs diff --git a/Cargo.toml b/Cargo.toml index b81f1ae6..b68afedb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -91,6 +91,10 @@ harness = false name = "bench_dict" harness = false +[[bench]] +name = "bench_frompyobject" +harness = false + [[bench]] name = "bench_gil" harness = false diff --git a/benches/bench_frompyobject.rs b/benches/bench_frompyobject.rs new file mode 100644 index 00000000..6a90900d --- /dev/null +++ b/benches/bench_frompyobject.rs @@ -0,0 +1,26 @@ +use criterion::{criterion_group, criterion_main, Bencher, Criterion}; + +use pyo3::{prelude::*, types::PyString}; + +#[derive(FromPyObject)] +enum ManyTypes { + Int(i32), + Bytes(Vec), + String(String) +} + +fn enum_from_pyobject(b: &mut Bencher) { + Python::with_gil(|py| { + let obj = PyString::new(py, "hello world"); + b.iter(|| { + let _: ManyTypes = obj.extract().unwrap(); + }); + }) +} + +fn criterion_benchmark(c: &mut Criterion) { + c.bench_function("enum_from_pyobject", enum_from_pyobject); +} + +criterion_group!(benches, criterion_benchmark); +criterion_main!(benches); From 492b7e4c0f5c86a106a79244ef696285824e787a Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 22 Dec 2021 00:29:22 +0000 Subject: [PATCH 2/6] macros: optimize generated code for #[derive(FromPyObject)] --- CHANGELOG.md | 3 +- Cargo.toml | 2 + benches/bench_frompyobject.rs | 2 +- benches/bench_pyclass.rs | 85 ++++++++----------- .../src/{from_pyobject.rs => frompyobject.rs} | 50 ++++++----- pyo3-macros-backend/src/lib.rs | 4 +- src/impl_.rs | 2 + src/impl_/frompyobject.rs | 26 ++++++ tests/test_frompyobject.rs | 54 ++++++++++-- 9 files changed, 139 insertions(+), 89 deletions(-) rename pyo3-macros-backend/src/{from_pyobject.rs => frompyobject.rs} (94%) create mode 100644 src/impl_/frompyobject.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index c39cdb45..ae6edaf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `PyErr::new_type` now takes an optional docstring and now returns `PyResult>` rather than a `ffi::PyTypeObject` pointer. - The `create_exception!` macro can now take an optional docstring. This docstring, if supplied, is visible to users (with `.__doc__` and `help()`) and accompanies your error type in your crate's documentation. - +- Improve performance and error messages for `#[derive(FromPyObject)]` for enums. [#2068](https://github.com/PyO3/pyo3/pull/2068) + ### Removed - Remove all functionality deprecated in PyO3 0.14. [#2007](https://github.com/PyO3/pyo3/pull/2007) diff --git a/Cargo.toml b/Cargo.toml index b68afedb..943b8362 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -94,6 +94,7 @@ harness = false [[bench]] name = "bench_frompyobject" harness = false +required-features = ["macros"] [[bench]] name = "bench_gil" @@ -106,6 +107,7 @@ harness = false [[bench]] name = "bench_pyclass" harness = false +required-features = ["macros"] [[bench]] name = "bench_pyobject" diff --git a/benches/bench_frompyobject.rs b/benches/bench_frompyobject.rs index 6a90900d..7f1274a8 100644 --- a/benches/bench_frompyobject.rs +++ b/benches/bench_frompyobject.rs @@ -6,7 +6,7 @@ use pyo3::{prelude::*, types::PyString}; enum ManyTypes { Int(i32), Bytes(Vec), - String(String) + String(String), } fn enum_from_pyobject(b: &mut Bencher) { diff --git a/benches/bench_pyclass.rs b/benches/bench_pyclass.rs index 03a8c07a..04676b1a 100644 --- a/benches/bench_pyclass.rs +++ b/benches/bench_pyclass.rs @@ -1,64 +1,49 @@ -#[cfg(feature = "macros")] use criterion::{criterion_group, criterion_main, Criterion}; +use pyo3::{class::PyObjectProtocol, prelude::*, type_object::LazyStaticType}; -#[cfg(feature = "macros")] -mod m { - use pyo3::{class::PyObjectProtocol, prelude::*, type_object::LazyStaticType}; +/// This is a feature-rich class instance used to benchmark various parts of the pyclass lifecycle. +#[pyclass] +struct MyClass { + #[pyo3(get, set)] + elements: Vec, +} - /// This is a feature-rich class instance used to benchmark various parts of the pyclass lifecycle. - #[pyclass] - struct MyClass { - #[pyo3(get, set)] - elements: Vec, +#[pymethods] +impl MyClass { + #[new] + fn new(elements: Vec) -> Self { + Self { elements } } - #[pymethods] - impl MyClass { - #[new] - fn new(elements: Vec) -> Self { - Self { elements } - } - - fn __call__(&mut self, new_element: i32) -> usize { - self.elements.push(new_element); - self.elements.len() - } - } - - #[pyproto] - impl PyObjectProtocol for MyClass { - /// A basic __str__ implementation. - fn __str__(&self) -> &'static str { - "MyClass" - } - } - - pub fn first_time_init(b: &mut criterion::Bencher) { - let gil = Python::acquire_gil(); - let py = gil.python(); - b.iter(|| { - // This is using an undocumented internal PyO3 API to measure pyclass performance; please - // don't use this in your own code! - let ty = LazyStaticType::new(); - ty.get_or_init::(py); - }); + fn __call__(&mut self, new_element: i32) -> usize { + self.elements.push(new_element); + self.elements.len() } } -#[cfg(feature = "macros")] +#[pyproto] +impl PyObjectProtocol for MyClass { + /// A basic __str__ implementation. + fn __str__(&self) -> &'static str { + "MyClass" + } +} + +pub fn first_time_init(b: &mut criterion::Bencher) { + let gil = Python::acquire_gil(); + let py = gil.python(); + b.iter(|| { + // This is using an undocumented internal PyO3 API to measure pyclass performance; please + // don't use this in your own code! + let ty = LazyStaticType::new(); + ty.get_or_init::(py); + }); +} + fn criterion_benchmark(c: &mut Criterion) { - c.bench_function("first_time_init", m::first_time_init); + c.bench_function("first_time_init", first_time_init); } -#[cfg(feature = "macros")] criterion_group!(benches, criterion_benchmark); -#[cfg(feature = "macros")] criterion_main!(benches); - -#[cfg(not(feature = "macros"))] -fn main() { - unimplemented!( - "benchmarking `bench_pyclass` is only available with the `macros` feature enabled" - ); -} diff --git a/pyo3-macros-backend/src/from_pyobject.rs b/pyo3-macros-backend/src/frompyobject.rs similarity index 94% rename from pyo3-macros-backend/src/from_pyobject.rs rename to pyo3-macros-backend/src/frompyobject.rs index c0560fb8..ef72c069 100644 --- a/pyo3-macros-backend/src/from_pyobject.rs +++ b/pyo3-macros-backend/src/frompyobject.rs @@ -54,38 +54,39 @@ impl<'a> Enum<'a> { /// Build derivation body for enums. fn build(&self) -> TokenStream { let mut var_extracts = Vec::new(); - let mut error_names = String::new(); - for (i, var) in self.variants.iter().enumerate() { + let mut variant_names = Vec::new(); + let mut error_names = Vec::new(); + for var in &self.variants { let struct_derive = var.build(); - let ext = quote!( + let ext = quote!({ let maybe_ret = || -> _pyo3::PyResult { #struct_derive }(); match maybe_ret { ok @ ::std::result::Result::Ok(_) => return ok, - ::std::result::Result::Err(err) => { - let py = _pyo3::PyNativeType::py(obj); - err_reasons.push_str(&::std::format!("{}\n", err.value(py).str()?)); - } + ::std::result::Result::Err(err) => err } - ); + }); var_extracts.push(ext); - if i > 0 { - error_names.push_str(" | "); - } - error_names.push_str(&var.err_name); + variant_names.push(var.path.segments.last().unwrap().ident.to_string()); + error_names.push(&var.err_name); } let ty_name = self.enum_ident.to_string(); quote!( - let mut err_reasons = ::std::string::String::new(); - #(#var_extracts)* - let err_msg = ::std::format!("failed to extract enum {} ('{}')\n{}", - #ty_name, - #error_names, - &err_reasons); - ::std::result::Result::Err(_pyo3::exceptions::PyTypeError::new_err(err_msg)) + let errors = [ + #(#var_extracts),* + ]; + ::std::result::Result::Err( + _pyo3::impl_::frompyobject::failed_to_extract_enum( + obj.py(), + #ty_name, + &[#(#variant_names),*], + &[#(#error_names),*], + &errors + ) + ) ) } } @@ -216,13 +217,8 @@ impl<'a> Container<'a> { new_err })?}) ) - } else { - let error_msg = if self.is_enum_variant { - let variant_name = &self.path.segments.last().unwrap(); - format!("- variant {} ({})", quote!(#variant_name), &self.err_name) - } else { - format!("failed to extract inner field of {}", quote!(#self_ty)) - }; + } else if !self.is_enum_variant { + let error_msg = format!("failed to extract inner field of {}", quote!(#self_ty)); quote!( ::std::result::Result::Ok(#self_ty(obj.extract().map_err(|err| { let py = _pyo3::PyNativeType::py(obj); @@ -232,6 +228,8 @@ impl<'a> Container<'a> { _pyo3::exceptions::PyTypeError::new_err(err_msg) })?)) ) + } else { + quote!(obj.extract().map(#self_ty)) } } diff --git a/pyo3-macros-backend/src/lib.rs b/pyo3-macros-backend/src/lib.rs index 69fc24d2..deb9f3fa 100644 --- a/pyo3-macros-backend/src/lib.rs +++ b/pyo3-macros-backend/src/lib.rs @@ -11,7 +11,7 @@ mod utils; mod attributes; mod defs; mod deprecations; -mod from_pyobject; +mod frompyobject; mod konst; mod method; mod module; @@ -23,7 +23,7 @@ mod pyimpl; mod pymethod; mod pyproto; -pub use from_pyobject::build_derive_from_pyobject; +pub use frompyobject::build_derive_from_pyobject; pub use module::{process_functions_in_module, py_init, PyModuleOptions}; pub use pyclass::{build_py_class, build_py_enum, PyClassArgs}; pub use pyfunction::{build_py_function, PyFunctionOptions}; diff --git a/src/impl_.rs b/src/impl_.rs index 46f5d771..31826afb 100644 --- a/src/impl_.rs +++ b/src/impl_.rs @@ -6,3 +6,5 @@ pub mod deprecations; pub mod freelist; +#[doc(hidden)] +pub mod frompyobject; diff --git a/src/impl_/frompyobject.rs b/src/impl_/frompyobject.rs new file mode 100644 index 00000000..9d859de9 --- /dev/null +++ b/src/impl_/frompyobject.rs @@ -0,0 +1,26 @@ +use crate::{exceptions::PyTypeError, PyErr, Python}; + +#[cold] +pub fn failed_to_extract_enum( + py: Python, + type_name: &str, + variant_names: &[&str], + error_names: &[&str], + errors: &[PyErr], +) -> PyErr { + let mut err_msg = format!( + "failed to extract enum {} ('{}')", + type_name, + error_names.join(" | ") + ); + for ((variant_name, error_name), error) in variant_names.iter().zip(error_names).zip(errors) { + err_msg.push('\n'); + err_msg.push_str(&format!( + "- variant {variant_name} ({error_name}): {error_msg}", + variant_name = variant_name, + error_name = error_name, + error_msg = error.value(py).str().unwrap().to_str().unwrap(), + )); + } + PyTypeError::new_err(err_msg) +} diff --git a/tests/test_frompyobject.rs b/tests/test_frompyobject.rs index 18c73eba..0a213acb 100644 --- a/tests/test_frompyobject.rs +++ b/tests/test_frompyobject.rs @@ -312,8 +312,6 @@ pub enum Foo<'a> { #[pyo3(item("foo"))] a: String, }, - #[pyo3(transparent)] - CatchAll(&'a PyAny), } #[pyclass] @@ -381,15 +379,52 @@ fn test_enum() { Foo::StructWithGetItemArg { a } => assert_eq!(a, "test"), _ => panic!("Expected extracting Foo::StructWithGetItemArg, got {:?}", f), } + }); +} +#[test] +fn test_enum_error() { + Python::with_gil(|py| { let dict = PyDict::new(py); - let f = Foo::extract(dict.as_ref()).expect("Failed to extract Foo from dict"); + let err = Foo::extract(dict.as_ref()).unwrap_err(); + assert_eq!( + err.to_string(), + "\ +TypeError: failed to extract enum Foo ('TupleVar | StructVar | TransparentTuple | TransparentStructVar | StructVarGetAttrArg | StructWithGetItem | StructWithGetItemArg') +- variant TupleVar (TupleVar): 'dict' object cannot be converted to 'PyTuple' +- variant StructVar (StructVar): 'dict' object has no attribute 'test' +- variant TransparentTuple (TransparentTuple): 'dict' object cannot be interpreted as an integer +- variant TransparentStructVar (TransparentStructVar): failed to extract field Foo :: TransparentStructVar.a +- variant StructVarGetAttrArg (StructVarGetAttrArg): 'dict' object has no attribute 'bla' +- variant StructWithGetItem (StructWithGetItem): 'a' +- variant StructWithGetItemArg (StructWithGetItemArg): 'foo'" + ); + }); +} + +#[derive(Debug, FromPyObject)] +enum EnumWithCatchAll<'a> { + #[pyo3(transparent)] + Foo(Foo<'a>), + #[pyo3(transparent)] + CatchAll(&'a PyAny), +} + +#[test] +fn test_enum_catch_all() { + Python::with_gil(|py| { + let dict = PyDict::new(py); + let f = EnumWithCatchAll::extract(dict.as_ref()) + .expect("Failed to extract EnumWithCatchAll from dict"); match f { - Foo::CatchAll(any) => { + EnumWithCatchAll::CatchAll(any) => { let d = <&PyDict>::extract(any).expect("Expected pydict"); assert!(d.is_empty()); } - _ => panic!("Expected extracting Foo::CatchAll, got {:?}", f), + _ => panic!( + "Expected extracting EnumWithCatchAll::CatchAll, got {:?}", + f + ), } }); } @@ -412,10 +447,11 @@ fn test_err_rename() { assert!(f.is_err()); assert_eq!( f.unwrap_err().to_string(), - "TypeError: failed to extract enum Bar (\'str | uint | int\')\n- variant A (str): \ - \'dict\' object cannot be converted to \'PyString\'\n- variant B (uint): \'dict\' object \ - cannot be interpreted as an integer\n- variant C (int): \'dict\' object cannot be \ - interpreted as an integer\n" + "\ +TypeError: failed to extract enum Bar (\'str | uint | int\') +- variant A (str): \'dict\' object cannot be converted to \'PyString\' +- variant B (uint): \'dict\' object cannot be interpreted as an integer +- variant C (int): \'dict\' object cannot be interpreted as an integer" ); }); } From a5511b8f8ae6b7b74d3ed7bb907e39d72a451a11 Mon Sep 17 00:00:00 2001 From: James Hilton-Balfe <50501825+Gobot1234@users.noreply.github.com> Date: Wed, 22 Dec 2021 16:51:03 +0000 Subject: [PATCH 3/6] Fix markdown link (#2071) --- guide/src/features.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guide/src/features.md b/guide/src/features.md index 5af4f3ec..1a29885c 100644 --- a/guide/src/features.md +++ b/guide/src/features.md @@ -83,7 +83,7 @@ These features enable conversions between Python types and types from other Rust ### `anyhow` -Adds a dependency on [anyhow](https://docs.rs/anyhow). Enables a conversion from [anyhow](https://docs.rs/anyhow)’s [`Error`]https://docs.rs/anyhow/latest/anyhow/struct.Error.html) type to [`PyErr`](https://docs.rs/pyo3/latest/pyo3/struct.PyErr.html), for easy error handling. +Adds a dependency on [anyhow](https://docs.rs/anyhow). Enables a conversion from [anyhow](https://docs.rs/anyhow)’s [`Error`](https://docs.rs/anyhow/latest/anyhow/struct.Error.html) type to [`PyErr`](https://docs.rs/pyo3/latest/pyo3/struct.PyErr.html), for easy error handling. ### `eyre` From 5be5d775899122fbbeb4037631b2dbf0ab88136f Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Thu, 23 Dec 2021 21:17:33 +0000 Subject: [PATCH 4/6] opt: improve handle_panic generated code --- CHANGELOG.md | 1 + src/callback.rs | 11 +++-------- src/gil.rs | 10 +++++++--- src/impl_.rs | 1 + src/impl_/not_send.rs | 9 +++++++++ src/internal_tricks.rs | 6 ------ src/panic.rs | 1 + src/python.rs | 3 ++- tests/test_compile_error.rs | 1 + tests/ui/not_send.rs | 11 +++++++++++ tests/ui/not_send.stderr | 14 ++++++++++++++ 11 files changed, 50 insertions(+), 18 deletions(-) create mode 100644 src/impl_/not_send.rs create mode 100644 tests/ui/not_send.rs create mode 100644 tests/ui/not_send.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index ae6edaf7..feb477f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The `create_exception!` macro can now take an optional docstring. This docstring, if supplied, is visible to users (with `.__doc__` and `help()`) and accompanies your error type in your crate's documentation. - Improve performance and error messages for `#[derive(FromPyObject)]` for enums. [#2068](https://github.com/PyO3/pyo3/pull/2068) +- Tweak LLVM code for compile times for internal `handle_panic` helper. [#2073](https://github.com/PyO3/pyo3/pull/2073) ### Removed diff --git a/src/callback.rs b/src/callback.rs index e29f07e0..4a8598f3 100644 --- a/src/callback.rs +++ b/src/callback.rs @@ -10,7 +10,7 @@ use crate::{GILPool, IntoPyPointer}; use crate::{IntoPy, PyObject, Python}; use std::any::Any; use std::os::raw::c_int; -use std::panic::{AssertUnwindSafe, UnwindSafe}; +use std::panic::UnwindSafe; use std::{isize, panic}; /// A type which can be the return type of a python C-API callback @@ -241,13 +241,8 @@ where R: PyCallbackOutput, { let pool = GILPool::new(); - let unwind_safe_py = AssertUnwindSafe(pool.python()); - let panic_result = panic::catch_unwind(move || -> PyResult<_> { - let py = *unwind_safe_py; - body(py) - }); - - panic_result_into_callback_output(pool.python(), panic_result) + let py = pool.python(); + panic_result_into_callback_output(py, panic::catch_unwind(move || -> PyResult<_> { body(py) })) } fn panic_result_into_callback_output( diff --git a/src/gil.rs b/src/gil.rs index 3de82040..1c0fa03c 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -2,7 +2,8 @@ //! Interaction with Python's global interpreter lock -use crate::{ffi, internal_tricks::Unsendable, Python}; +use crate::impl_::not_send::{NotSend, NOT_SEND}; +use crate::{ffi, Python}; use parking_lot::{const_mutex, Mutex, Once}; use std::cell::{Cell, RefCell}; use std::{ @@ -157,6 +158,7 @@ where pub struct GILGuard { gstate: ffi::PyGILState_STATE, pool: ManuallyDrop>, + _not_send: NotSend, } impl GILGuard { @@ -230,6 +232,7 @@ impl GILGuard { GILGuard { gstate, pool: ManuallyDrop::new(pool), + _not_send: NOT_SEND, } } } @@ -332,7 +335,7 @@ pub struct GILPool { /// Initial length of owned objects and anys. /// `Option` is used since TSL can be broken when `new` is called from `atexit`. start: Option, - no_send: Unsendable, + _not_send: NotSend, } impl GILPool { @@ -351,11 +354,12 @@ impl GILPool { POOL.update_counts(Python::assume_gil_acquired()); GILPool { start: OWNED_OBJECTS.try_with(|o| o.borrow().len()).ok(), - no_send: Unsendable::default(), + _not_send: NOT_SEND, } } /// Gets the Python token associated with this [`GILPool`]. + #[inline] pub fn python(&self) -> Python { unsafe { Python::assume_gil_acquired() } } diff --git a/src/impl_.rs b/src/impl_.rs index 31826afb..586ce1a5 100644 --- a/src/impl_.rs +++ b/src/impl_.rs @@ -8,3 +8,4 @@ pub mod deprecations; pub mod freelist; #[doc(hidden)] pub mod frompyobject; +pub(crate) mod not_send; diff --git a/src/impl_/not_send.rs b/src/impl_/not_send.rs new file mode 100644 index 00000000..fca8a96c --- /dev/null +++ b/src/impl_/not_send.rs @@ -0,0 +1,9 @@ +use std::marker::PhantomData; + +use crate::Python; + +/// A marker type that makes the type !Send. +/// Workaround for lack of !Send on stable (https://github.com/rust-lang/rust/issues/68318). +pub(crate) struct NotSend(PhantomData<*mut Python<'static>>); + +pub(crate) const NOT_SEND: NotSend = NotSend(PhantomData); diff --git a/src/internal_tricks.rs b/src/internal_tricks.rs index 307f1bfc..06422871 100644 --- a/src/internal_tricks.rs +++ b/src/internal_tricks.rs @@ -1,11 +1,5 @@ use crate::ffi::{Py_ssize_t, PY_SSIZE_T_MAX}; use std::ffi::{CStr, CString}; -use std::marker::PhantomData; -use std::rc::Rc; - -/// A marker type that makes the type !Send. -/// Temporal hack until https://github.com/rust-lang/rust/issues/13231 is resolved. -pub(crate) type Unsendable = PhantomData>; pub struct PrivateMarker; diff --git a/src/panic.rs b/src/panic.rs index 0fe6c0bf..4fe6100d 100644 --- a/src/panic.rs +++ b/src/panic.rs @@ -17,6 +17,7 @@ Python interpreter to exit. impl PanicException { // Try to format the error in the same way panic does + #[cold] pub(crate) fn from_panic_payload(payload: Box) -> PyErr { if let Some(string) = payload.downcast_ref::() { Self::new_err((string.clone(),)) diff --git a/src/python.rs b/src/python.rs index 014e866e..c410d0d1 100644 --- a/src/python.rs +++ b/src/python.rs @@ -4,6 +4,7 @@ use crate::err::{self, PyDowncastError, PyErr, PyResult}; use crate::gil::{self, GILGuard, GILPool}; +use crate::impl_::not_send::NotSend; use crate::type_object::{PyTypeInfo, PyTypeObject}; use crate::types::{PyAny, PyDict, PyModule, PyType}; use crate::{ffi, AsPyPointer, FromPyPointer, IntoPyPointer, PyNativeType, PyObject, PyTryFrom}; @@ -184,7 +185,7 @@ impl PartialOrd<(u8, u8, u8)> for PythonVersionInfo<'_> { /// [`Py::clone_ref`]: crate::Py::clone_ref /// [Memory Management]: https://pyo3.rs/main/memory.html#gil-bound-memory #[derive(Copy, Clone)] -pub struct Python<'py>(PhantomData<&'py GILGuard>); +pub struct Python<'py>(PhantomData<(&'py GILGuard, NotSend)>); impl Python<'_> { /// Acquires the global interpreter lock, allowing access to the Python interpreter. The diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index ce9288fa..daade52d 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -29,6 +29,7 @@ fn _test_compile_errors() { t.compile_fail("tests/ui/invalid_pymodule_args.rs"); t.compile_fail("tests/ui/missing_clone.rs"); t.compile_fail("tests/ui/reject_generics.rs"); + t.compile_fail("tests/ui/not_send.rs"); tests_rust_1_49(&t); tests_rust_1_55(&t); diff --git a/tests/ui/not_send.rs b/tests/ui/not_send.rs new file mode 100644 index 00000000..d7c47dec --- /dev/null +++ b/tests/ui/not_send.rs @@ -0,0 +1,11 @@ +use pyo3::prelude::*; + +fn test_not_send_allow_threads(py: Python) { + py.allow_threads(|| { drop(py); }); +} + +fn main() { + Python::with_gil(|py| { + test_not_send_allow_threads(py); + }) +} diff --git a/tests/ui/not_send.stderr b/tests/ui/not_send.stderr new file mode 100644 index 00000000..31340cd7 --- /dev/null +++ b/tests/ui/not_send.stderr @@ -0,0 +1,14 @@ +error[E0277]: `*mut pyo3::Python<'static>` cannot be shared between threads safely + --> tests/ui/not_send.rs:4:8 + | +4 | py.allow_threads(|| { drop(py); }); + | ^^^^^^^^^^^^^ `*mut pyo3::Python<'static>` cannot be shared between threads safely + | + = help: within `pyo3::Python<'_>`, the trait `Sync` is not implemented for `*mut pyo3::Python<'static>` + = note: required because it appears within the type `PhantomData<*mut pyo3::Python<'static>>` + = note: required because it appears within the type `pyo3::impl_::not_send::NotSend` + = note: required because it appears within the type `(&GILGuard, pyo3::impl_::not_send::NotSend)` + = note: required because it appears within the type `PhantomData<(&GILGuard, pyo3::impl_::not_send::NotSend)>` + = note: required because it appears within the type `pyo3::Python<'_>` + = note: required because of the requirements on the impl of `Send` for `&pyo3::Python<'_>` + = note: required because it appears within the type `[closure@$DIR/tests/ui/not_send.rs:4:22: 4:38]` From 33a618914f35bff18d9e70f500784330fa0bd5ec Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Fri, 24 Dec 2021 01:04:23 +0000 Subject: [PATCH 5/6] opt: reduce class creation generated code --- CHANGELOG.md | 4 +- src/pyclass.rs | 108 ++++++++++++++++++++++++++++--------------------- 2 files changed, 65 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index feb477f2..a4929599 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The `create_exception!` macro can now take an optional docstring. This docstring, if supplied, is visible to users (with `.__doc__` and `help()`) and accompanies your error type in your crate's documentation. - Improve performance and error messages for `#[derive(FromPyObject)]` for enums. [#2068](https://github.com/PyO3/pyo3/pull/2068) -- Tweak LLVM code for compile times for internal `handle_panic` helper. [#2073](https://github.com/PyO3/pyo3/pull/2073) +- Reduce generated LLVM code size (to improve compile times) for: + - internal `handle_panic` helper [#2073](https://github.com/PyO3/pyo3/pull/2073) + - `#[pyclass]` type object creation [#2075](https://github.com/PyO3/pyo3/pull/2075) ### Removed diff --git a/src/pyclass.rs b/src/pyclass.rs index 132fe599..5d800e29 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -7,7 +7,7 @@ use crate::{ }; use std::{ convert::TryInto, - ffi::CString, + ffi::{CStr, CString}, os::raw::{c_char, c_int, c_uint, c_void}, ptr, }; @@ -29,32 +29,6 @@ pub trait PyClass: type BaseNativeType: PyTypeInfo + PyNativeType; } -/// For collecting slot items. -#[derive(Default)] -struct TypeSlots(Vec); - -impl TypeSlots { - fn push(&mut self, slot: c_int, pfunc: *mut c_void) { - self.0.push(ffi::PyType_Slot { slot, pfunc }); - } -} - -fn tp_doc() -> PyResult> { - Ok(match T::DOC { - "\0" => None, - s if s.as_bytes().ends_with(b"\0") => Some(s.as_ptr() as _), - // If the description is not null-terminated, create CString and leak it - s => Some(CString::new(s)?.into_raw() as _), - }) -} - -fn get_type_name(module_name: Option<&str>) -> PyResult<*mut c_char> { - Ok(match module_name { - Some(module_name) => CString::new(format!("{}.{}", module_name, T::NAME))?.into_raw(), - None => CString::new(format!("builtins.{}", T::NAME))?.into_raw(), - }) -} - fn into_raw(vec: Vec) -> *mut c_void { Box::into_raw(vec.into_boxed_slice()) as _ } @@ -66,41 +40,53 @@ pub(crate) fn create_type_object( where T: PyClass, { - let mut slots = TypeSlots::default(); + let mut slots = Vec::new(); - slots.push(ffi::Py_tp_base, T::BaseType::type_object_raw(py) as _); - if let Some(doc) = tp_doc::()? { - slots.push(ffi::Py_tp_doc, doc); + fn push_slot(slots: &mut Vec, slot: c_int, pfunc: *mut c_void) { + slots.push(ffi::PyType_Slot { slot, pfunc }); } - slots.push(ffi::Py_tp_new, T::get_new().unwrap_or(fallback_new) as _); - slots.push(ffi::Py_tp_dealloc, tp_dealloc:: as _); + push_slot( + &mut slots, + ffi::Py_tp_base, + T::BaseType::type_object_raw(py) as _, + ); + if let Some(doc) = py_class_doc(T::DOC) { + push_slot(&mut slots, ffi::Py_tp_doc, doc as _); + } + + push_slot( + &mut slots, + ffi::Py_tp_new, + T::get_new().unwrap_or(fallback_new) as _, + ); + push_slot(&mut slots, ffi::Py_tp_dealloc, tp_dealloc:: as _); if let Some(alloc) = T::get_alloc() { - slots.push(ffi::Py_tp_alloc, alloc as _); + push_slot(&mut slots, ffi::Py_tp_alloc, alloc as _); } if let Some(free) = T::get_free() { - slots.push(ffi::Py_tp_free, free as _); + push_slot(&mut slots, ffi::Py_tp_free, free as _); } #[cfg(Py_3_9)] { - let members = py_class_members::(); + let members = py_class_members(PyCell::::dict_offset(), PyCell::::weakref_offset()); if !members.is_empty() { - slots.push(ffi::Py_tp_members, into_raw(members)) + push_slot(&mut slots, ffi::Py_tp_members, into_raw(members)) } } // normal methods let methods = py_class_method_defs(&T::for_each_method_def); if !methods.is_empty() { - slots.push(ffi::Py_tp_methods, into_raw(methods)); + push_slot(&mut slots, ffi::Py_tp_methods, into_raw(methods)); } // properties let props = py_class_properties(T::Dict::IS_DUMMY, &T::for_each_method_def); if !props.is_empty() { - slots.push(ffi::Py_tp_getset, into_raw(props)); + push_slot(&mut slots, ffi::Py_tp_getset, into_raw(props)); } // protocol methods @@ -109,16 +95,16 @@ where has_gc_methods |= proto_slots .iter() .any(|slot| slot.slot == ffi::Py_tp_clear || slot.slot == ffi::Py_tp_traverse); - slots.0.extend_from_slice(proto_slots); + slots.extend_from_slice(proto_slots); }); - slots.push(0, ptr::null_mut()); + push_slot(&mut slots, 0, ptr::null_mut()); let mut spec = ffi::PyType_Spec { - name: get_type_name::(module_name)?, + name: py_class_qualified_name(module_name, T::NAME)?, basicsize: std::mem::size_of::() as c_int, itemsize: 0, flags: py_class_flags(has_gc_methods, T::IS_GC, T::IS_BASETYPE), - slots: slots.0.as_mut_ptr(), + slots: slots.as_mut_ptr(), }; let type_object = unsafe { ffi::PyType_FromSpec(&mut spec) }; @@ -188,6 +174,33 @@ fn tp_init_additional(type_object: *mut ffi::PyTypeObject) { #[cfg(any(Py_LIMITED_API, Py_3_10))] fn tp_init_additional(_type_object: *mut ffi::PyTypeObject) {} +fn py_class_doc(class_doc: &str) -> Option<*mut c_char> { + match class_doc { + "\0" => None, + s => { + // To pass *mut pointer to python safely, leak a CString in whichever case + let cstring = if s.as_bytes().last() == Some(&0) { + CStr::from_bytes_with_nul(s.as_bytes()) + .unwrap_or_else(|e| panic!("doc contains interior nul byte: {:?} in {}", e, s)) + .to_owned() + } else { + CString::new(s) + .unwrap_or_else(|e| panic!("doc contains interior nul byte: {:?} in {}", e, s)) + }; + Some(cstring.into_raw()) + } + } +} + +fn py_class_qualified_name(module_name: Option<&str>, class_name: &str) -> PyResult<*mut c_char> { + Ok(CString::new(format!( + "{}.{}", + module_name.unwrap_or("builtins"), + class_name + ))? + .into_raw()) +} + fn py_class_flags(has_gc_methods: bool, is_gc: bool, is_basetype: bool) -> c_uint { let mut flags = if has_gc_methods || is_gc { ffi::Py_TPFLAGS_DEFAULT | ffi::Py_TPFLAGS_HAVE_GC @@ -230,7 +243,10 @@ fn py_class_method_defs( /// /// Only works on Python 3.9 and up. #[cfg(Py_3_9)] -fn py_class_members() -> Vec { +fn py_class_members( + dict_offset: Option, + weakref_offset: Option, +) -> Vec { #[inline(always)] fn offset_def(name: &'static str, offset: ffi::Py_ssize_t) -> ffi::structmember::PyMemberDef { ffi::structmember::PyMemberDef { @@ -245,12 +261,12 @@ fn py_class_members() -> Vec { let mut members = Vec::new(); // __dict__ support - if let Some(dict_offset) = PyCell::::dict_offset() { + if let Some(dict_offset) = dict_offset { members.push(offset_def("__dictoffset__\0", dict_offset)); } // weakref support - if let Some(weakref_offset) = PyCell::::weakref_offset() { + if let Some(weakref_offset) = weakref_offset { members.push(offset_def("__weaklistoffset__\0", weakref_offset)); } From 90479ddae4453e56736c476677f61a072bd62956 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Thu, 23 Dec 2021 23:33:51 +0000 Subject: [PATCH 6/6] opt: make argument extraction code smaller --- CHANGELOG.md | 5 +- pyo3-macros-backend/src/params.rs | 12 +-- pytests/pyo3-benchmarks/tox.ini | 3 + src/derive_utils.rs | 141 ++++++++++++++++-------------- src/impl_.rs | 1 + src/impl_/extract_argument.rs | 30 +++++++ 6 files changed, 120 insertions(+), 72 deletions(-) create mode 100644 src/impl_/extract_argument.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index a4929599..3e58036e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,8 +41,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 accompanies your error type in your crate's documentation. - Improve performance and error messages for `#[derive(FromPyObject)]` for enums. [#2068](https://github.com/PyO3/pyo3/pull/2068) - Reduce generated LLVM code size (to improve compile times) for: - - internal `handle_panic` helper [#2073](https://github.com/PyO3/pyo3/pull/2073) - - `#[pyclass]` type object creation [#2075](https://github.com/PyO3/pyo3/pull/2075) + - internal `handle_panic` helper [#2074](https://github.com/PyO3/pyo3/pull/2074) + - `#[pyfunction]` and `#[pymethods]` argument extraction [#2075](https://github.com/PyO3/pyo3/pull/2075) + - `#[pyclass]` type object creation [#2076](https://github.com/PyO3/pyo3/pull/2076) ### Removed diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index c15b3e2e..5e73a7bf 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -213,8 +213,9 @@ fn impl_arg_param( let ty = arg.ty; let name = arg.name; + let name_str = name.to_string(); let transform_error = quote! { - |e| _pyo3::derive_utils::argument_extraction_error(#py, stringify!(#name), e) + |e| _pyo3::impl_::extract_argument::argument_extraction_error(#py, #name_str, e) }; if is_args(&spec.attrs, name) { @@ -223,7 +224,7 @@ fn impl_arg_param( arg.name.span() => "args cannot be optional" ); return Ok(quote_arg_span! { - let #arg_name = _args.unwrap().extract().map_err(#transform_error)?; + let #arg_name = _pyo3::impl_::extract_argument::extract_argument(_args.unwrap(), #name_str)?; }); } else if is_kwargs(&spec.attrs, name) { ensure_spanned!( @@ -231,9 +232,8 @@ fn impl_arg_param( arg.name.span() => "kwargs must be Option<_>" ); return Ok(quote_arg_span! { - let #arg_name = _kwargs.map(|kwargs| kwargs.extract()) - .transpose() - .map_err(#transform_error)?; + let #arg_name = _kwargs.map(|kwargs| _pyo3::impl_::extract_argument::extract_argument(kwargs, #name_str)) + .transpose()?; }); } @@ -243,7 +243,7 @@ fn impl_arg_param( let extract = if let Some(FromPyWithAttribute(expr_path)) = &arg.attrs.from_py_with { quote_arg_span! { #expr_path(_obj).map_err(#transform_error) } } else { - quote_arg_span! { _obj.extract().map_err(#transform_error) } + quote_arg_span! { _pyo3::impl_::extract_argument::extract_argument(_obj, #name_str) } }; let arg_value_or_default = match (spec.default_value(name), arg.optional.is_some()) { diff --git a/pytests/pyo3-benchmarks/tox.ini b/pytests/pyo3-benchmarks/tox.ini index 1713e7eb..635f66aa 100644 --- a/pytests/pyo3-benchmarks/tox.ini +++ b/pytests/pyo3-benchmarks/tox.ini @@ -3,3 +3,6 @@ usedevelop = True description = Run the unit tests under {basepython} deps = -rrequirements-dev.txt commands = pytest --benchmark-sort=name {posargs} +# Use recreate so that tox always rebuilds, otherwise changes to Rust are not +# picked up. +recreate = True diff --git a/src/derive_utils.rs b/src/derive_utils.rs index 0fc18ae2..ab4e6a0c 100644 --- a/src/derive_utils.rs +++ b/src/derive_utils.rs @@ -102,9 +102,12 @@ impl FunctionDescription { varkeywords } (Some(kwargs), false) => { - self.extract_keyword_arguments(kwargs, output, |name, _| { - Err(self.unexpected_keyword_argument(name)) - })?; + self.extract_keyword_arguments( + kwargs, + output, + #[cold] + |name, _| Err(self.unexpected_keyword_argument(name)), + )?; None } (None, _) => None, @@ -112,58 +115,39 @@ impl FunctionDescription { // Check that there's sufficient positional arguments once keyword arguments are specified if args_provided < self.required_positional_parameters { - let missing_positional_arguments: Vec<_> = self - .positional_parameter_names - .iter() - .take(self.required_positional_parameters) - .zip(output.iter()) - .filter_map(|(param, out)| if out.is_none() { Some(*param) } else { None }) - .collect(); - if !missing_positional_arguments.is_empty() { - return Err( - self.missing_required_arguments("positional", &missing_positional_arguments) - ); + for out in &output[..self.required_positional_parameters] { + if out.is_none() { + return Err(self.missing_required_positional_arguments(output)); + } } } // Check no missing required keyword arguments - let missing_keyword_only_arguments: Vec<_> = self - .keyword_only_parameters - .iter() - .zip(&output[num_positional_parameters..]) - .filter_map(|(keyword_desc, out)| { - if keyword_desc.required && out.is_none() { - Some(keyword_desc.name) - } else { - None - } - }) - .collect(); - - if !missing_keyword_only_arguments.is_empty() { - return Err(self.missing_required_arguments("keyword", &missing_keyword_only_arguments)); + let keyword_output = &output[num_positional_parameters..]; + for (param, out) in self.keyword_only_parameters.iter().zip(keyword_output) { + if param.required && out.is_none() { + return Err(self.missing_required_keyword_arguments(keyword_output)); + } } Ok((varargs, varkeywords)) } - #[inline] fn extract_keyword_arguments<'p>( &self, kwargs: impl Iterator, output: &mut [Option<&'p PyAny>], mut unexpected_keyword_handler: impl FnMut(&'p PyAny, &'p PyAny) -> PyResult<()>, ) -> PyResult<()> { - let (args_output, kwargs_output) = - output.split_at_mut(self.positional_parameter_names.len()); + let positional_args_count = self.positional_parameter_names.len(); let mut positional_only_keyword_arguments = Vec::new(); - for (kwarg_name, value) in kwargs { - let utf8_string = match kwarg_name.downcast::()?.to_str() { - Ok(utf8_string) => utf8_string, + 'for_each_kwarg: for (kwarg_name_py, value) in kwargs { + let kwarg_name = match kwarg_name_py.downcast::()?.to_str() { + Ok(kwarg_name) => kwarg_name, // This keyword is not a UTF8 string: all PyO3 argument names are guaranteed to be // UTF8 by construction. Err(_) => { - unexpected_keyword_handler(kwarg_name, value)?; + unexpected_keyword_handler(kwarg_name_py, value)?; continue; } }; @@ -171,31 +155,24 @@ impl FunctionDescription { // Compare the keyword name against each parameter in turn. This is exactly the same method // which CPython uses to map keyword names. Although it's O(num_parameters), the number of // parameters is expected to be small so it's not worth constructing a mapping. - if let Some(i) = self - .keyword_only_parameters - .iter() - .position(|param| utf8_string == param.name) - { - kwargs_output[i] = Some(value); - continue; + for (i, param) in self.keyword_only_parameters.iter().enumerate() { + if param.name == kwarg_name { + output[positional_args_count + i] = Some(value); + continue 'for_each_kwarg; + } } // Repeat for positional parameters - if let Some((i, param)) = self - .positional_parameter_names - .iter() - .enumerate() - .find(|&(_, param)| utf8_string == *param) - { + if let Some(i) = self.find_keyword_parameter_in_positionals(kwarg_name) { if i < self.positional_only_parameters { - positional_only_keyword_arguments.push(*param); - } else if args_output[i].replace(value).is_some() { - return Err(self.multiple_values_for_argument(param)); + positional_only_keyword_arguments.push(kwarg_name); + } else if output[i].replace(value).is_some() { + return Err(self.multiple_values_for_argument(kwarg_name)); } continue; } - unexpected_keyword_handler(kwarg_name, value)?; + unexpected_keyword_handler(kwarg_name_py, value)?; } if positional_only_keyword_arguments.is_empty() { @@ -205,6 +182,16 @@ impl FunctionDescription { } } + fn find_keyword_parameter_in_positionals(&self, kwarg_name: &str) -> Option { + for (i, param_name) in self.positional_parameter_names.iter().enumerate() { + if *param_name == kwarg_name { + return Some(i); + } + } + None + } + + #[cold] fn too_many_positional_arguments(&self, args_provided: usize) -> PyErr { let was = if args_provided == 1 { "was" } else { "were" }; let msg = if self.required_positional_parameters != self.positional_parameter_names.len() { @@ -228,6 +215,7 @@ impl FunctionDescription { PyTypeError::new_err(msg) } + #[cold] fn multiple_values_for_argument(&self, argument: &str) -> PyErr { PyTypeError::new_err(format!( "{} got multiple values for argument '{}'", @@ -236,6 +224,7 @@ impl FunctionDescription { )) } + #[cold] fn unexpected_keyword_argument(&self, argument: &PyAny) -> PyErr { PyTypeError::new_err(format!( "{} got an unexpected keyword argument '{}'", @@ -244,6 +233,7 @@ impl FunctionDescription { )) } + #[cold] fn positional_only_keyword_arguments(&self, parameter_names: &[&str]) -> PyErr { let mut msg = format!( "{} got some positional-only arguments passed as keyword arguments: ", @@ -253,6 +243,7 @@ impl FunctionDescription { PyTypeError::new_err(msg) } + #[cold] fn missing_required_arguments(&self, argument_type: &str, parameter_names: &[&str]) -> PyErr { let arguments = if parameter_names.len() == 1 { "argument" @@ -269,18 +260,40 @@ impl FunctionDescription { push_parameter_list(&mut msg, parameter_names); PyTypeError::new_err(msg) } -} -/// Add the argument name to the error message of an error which occurred during argument extraction -pub fn argument_extraction_error(py: Python, arg_name: &str, error: PyErr) -> PyErr { - if error.is_instance_of::(py) { - let reason = error - .value(py) - .str() - .unwrap_or_else(|_| PyString::new(py, "")); - PyTypeError::new_err(format!("argument '{}': {}", arg_name, reason)) - } else { - error + #[cold] + fn missing_required_keyword_arguments(&self, keyword_outputs: &[Option<&PyAny>]) -> PyErr { + debug_assert_eq!(self.keyword_only_parameters.len(), keyword_outputs.len()); + + let missing_keyword_only_arguments: Vec<_> = self + .keyword_only_parameters + .iter() + .zip(keyword_outputs) + .filter_map(|(keyword_desc, out)| { + if keyword_desc.required && out.is_none() { + Some(keyword_desc.name) + } else { + None + } + }) + .collect(); + + debug_assert!(!missing_keyword_only_arguments.is_empty()); + self.missing_required_arguments("keyword", &missing_keyword_only_arguments) + } + + #[cold] + fn missing_required_positional_arguments(&self, output: &[Option<&PyAny>]) -> PyErr { + let missing_positional_arguments: Vec<_> = self + .positional_parameter_names + .iter() + .take(self.required_positional_parameters) + .zip(output) + .filter_map(|(param, out)| if out.is_none() { Some(*param) } else { None }) + .collect(); + + debug_assert!(!missing_positional_arguments.is_empty()); + self.missing_required_arguments("positional", &missing_positional_arguments) } } diff --git a/src/impl_.rs b/src/impl_.rs index 586ce1a5..0f61b91e 100644 --- a/src/impl_.rs +++ b/src/impl_.rs @@ -5,6 +5,7 @@ //! breaking semver guarantees. pub mod deprecations; +pub mod extract_argument; pub mod freelist; #[doc(hidden)] pub mod frompyobject; diff --git a/src/impl_/extract_argument.rs b/src/impl_/extract_argument.rs new file mode 100644 index 00000000..610e76cd --- /dev/null +++ b/src/impl_/extract_argument.rs @@ -0,0 +1,30 @@ +use crate::{ + exceptions::PyTypeError, type_object::PyTypeObject, FromPyObject, PyAny, PyErr, PyResult, + Python, +}; + +#[doc(hidden)] +#[inline] +pub fn extract_argument<'py, T>(obj: &'py PyAny, arg_name: &str) -> PyResult +where + T: FromPyObject<'py>, +{ + match obj.extract() { + Ok(e) => Ok(e), + Err(e) => Err(argument_extraction_error(obj.py(), arg_name, e)), + } +} + +/// Adds the argument name to the error message of an error which occurred during argument extraction. +/// +/// Only modifies TypeError. (Cannot guarantee all exceptions have constructors from +/// single string.) +#[doc(hidden)] +#[cold] +pub fn argument_extraction_error(py: Python, arg_name: &str, error: PyErr) -> PyErr { + if error.get_type(py) == PyTypeError::type_object(py) { + PyTypeError::new_err(format!("argument '{}': {}", arg_name, error.value(py))) + } else { + error + } +}