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.

Sign up for GitHub

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

use PyTuple_Pack in fixed-size tuple conversions #3296

Closed
wants to merge 2 commits into from
Closed

use PyTuple_Pack in fixed-size tuple conversions #3296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40437c5
add tuple_into_py benchmark
davidhewitt Jul 4, 2023
867a12b
use `PyTuple_Pack` in fixed-size tuple conversions
davidhewitt Jul 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions benches/bench_tuple.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ fn tuple_to_list(b: &mut Bencher<'_>) {
});
}

fn tuple_into_py(b: &mut Bencher<'_>) {
Python::with_gil(|py| {
b.iter(|| -> PyObject { (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12).into_py(py) });
});
}

fn criterion_benchmark(c: &mut Criterion) {
c.bench_function("iter_tuple", iter_tuple);
c.bench_function("tuple_new", tuple_new);
Expand All @@ -92,6 +98,7 @@ fn criterion_benchmark(c: &mut Criterion) {
c.bench_function("sequence_from_tuple", sequence_from_tuple);
c.bench_function("tuple_new_list", tuple_new_list);
c.bench_function("tuple_to_list", tuple_to_list);
c.bench_function("tuple_into_py", tuple_into_py);
}

criterion_group!(benches, criterion_benchmark);
Expand Down
33 changes: 18 additions & 15 deletions src/types/tuple.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,22 +284,24 @@ fn wrong_tuple_length(t: &PyTuple, expected_length: usize) -> PyErr {
macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+} => {
impl <$($T: ToPyObject),+> ToPyObject for ($($T,)+) {
fn to_object(&self, py: Python<'_>) -> PyObject {
unsafe {
let ptr = ffi::PyTuple_New($length);
let ret = PyObject::from_owned_ptr(py, ptr);
$(ffi::PyTuple_SetItem(ptr, $n, self.$n.to_object(py).into_ptr());)+
ret
#[inline]
fn inner(py: Python<'_>, $($refN: PyObject),+) -> PyObject {
unsafe {
PyObject::from_owned_ptr(py, ffi::PyTuple_Pack($length, $($refN.as_ptr()),+))
}
}
inner(py, $(self.$n.to_object(py)),+)
}
}
impl <$($T: IntoPy<PyObject>),+> IntoPy<PyObject> for ($($T,)+) {
fn into_py(self, py: Python<'_>) -> PyObject {
unsafe {
let ptr = ffi::PyTuple_New($length);
let ret = PyObject::from_owned_ptr(py, ptr);
$(ffi::PyTuple_SetItem(ptr, $n, self.$n.into_py(py).into_ptr());)+
ret
#[inline]
fn inner(py: Python<'_>, $($refN: PyObject),+) -> PyObject {
Comment on lines +298 to +299
Copy link
Member Author

Choose a reason for hiding this comment

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

For good measure I updated all of these to have concrete inner functions. Interestingly I found without #[inline] here the benchmarks were about 3% slower. This makes me very tempted to go apply #[inline] on all of the inner functions following merge of #3273. (Probably a separate PR and I'll report on the performance impact.)

Copy link
Member

Choose a reason for hiding this comment

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

I think adding #[inline] everywhere would sort of make the point moot because this reintroduces the kind of code bloat this is supposed to fight?

I would suggest to a) try this using thin LTO which should give the compiler more leeway in inlining this without generating these separately for each CGU and b) if we still want to use #[inline], then to just drop the inner layer completely because this is basically say the "code bloat" is worth it and not bloat at all.

Furthermore, in this particular case, the body of inner is particularly small which suggests to me that the balance is tilted towards b). Especially so since the outer body is potentially large due to calling ToPyObject for each tuple component and the argument passing overhead is potentially large for the same reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good points, here's some more thoughts:

  • I just tried this benchmark with lto = "thin" and also lto = "fat". Both provide a speedup over the default of no LTO. I did still observe that adding #[inline] here has the same perf bump with thin lto. With fat lto there is no difference which is unsuprising.
    • An open question whether we should be considering enabling LTO for our benchmarks more generally? Presumably those who are most concerned about performance will have fat LTO turned on (pydantic-core does), so if we measure with LTO we're understanding how the most optimized code may perform. My gut says turning LTO on is correct (i.e. measuring most optimized is most appropriate). I would also accept a counter-argument that it's better to have LTO off so that we measure the base case.
  • I think there are two main types of "code bloat" - LLVM IR and binary code. I argue that #[inline] fn inner is marginally better than not having the concrete inner, because the LLVM IR will be smaller (one copy with an inline hint, rather than the whole body for each monomorphization of the outer generic). We leave it to the optimizer to decide whether to follow the inline hint and bloat the binary code. So I'd wager #[inline] fn inner may be the best compromise of compile times and performance.
  • I agree with you that the body is very small compared to the call setup in this case. If we switch back to PyTuple_New and looping through arguments the body gets bigger. :)

Copy link
Member

Choose a reason for hiding this comment

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

An open question whether we should be considering enabling LTO for our benchmarks more generally? Presumably those who are most concerned about performance will have fat LTO turned on (pydantic-core does), so if we measure with LTO we're understanding how the most optimized code may perform. My gut says turning LTO on is correct (i.e. measuring most optimized is most appropriate). I would also accept a counter-argument that it's better to have LTO off so that we measure the base case.

I think they inform different decisions. The base case informs us about missing #[inline] attributes on trivial functions which really suffer from the call overhead, e.g. trait impls which should compile down to nothing.

Benchmarks with LTO would help us with code size trade-offs like the one discussed here but would completely mask the previous issue for users who want good performance, but do not want to pay the compile-time cost for fat LTO. (Personally, I tend to prefer thin LTO outside of scientific code as it appears to be the sweet spot between performance and productivity.)

Since masking missing #[inline] on trivial function bodies is the more "catastrophic" failure mode IMHO, I would prefer to keep LTO off.

I think there are two main types of "code bloat" - LLVM IR and binary code. I argue that #[inline] fn inner is marginally better than not having the concrete inner, because the LLVM IR will be smaller (one copy with an inline hint, rather than the whole body for each monomorphization of the outer generic). We leave it to the optimizer to decide whether to follow the inline hint and bloat the binary code. So I'd wager #[inline] fn inner may be the best compromise of compile times and performance.

I don't think this is how this currently works. Adding #[inline] means that in addition changing the inlining heuristics, a separate copy of the function is generated into each CGU in the same way generic functions are currently handled. Hence, I think using an #[inline] fn inner inside the generic outer function would increase LLVM IR because each CGU potentially contains the monomorphizations of the outer function and a copy of the inlined inner function. (This reasoning might change when -Zshare-generics stabilizes as generics might get more efficient because monomorphizations can be shared between CGU.)

I also see the completely independent argument of preferring simpler code (i.e. no inner function at all) when the performance is basically equivalent. Adding additional layers like non-generic inner function should always yield measurable benefits. (Of course, we might not actually measure this in all cases when there is no trade-off involved like here.) Hence, I think keeping the dichotomy of just a generic function for throughput or a non-inlined non-generic inner function for code size is the more helpful approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with what you say here though I still defend that per-CGU having an #[inline] inner function works out as less LLVM IR per CGU. Instead of N instantiations of a full generic function without any inner I understand there would be N instantiaitions of the much reduced outer generic plus 1 instantiation of the inline inner.

That said, I found that in #3321 adding #[inline] was actually slower than not. So clearly the moral of the story is we shouldn't try to beat the compiler, and I'll stay away from adding #[inline] for now.

unsafe {
Py::from_owned_ptr(py, ffi::PyTuple_Pack($length, $($refN.as_ptr()),+))
}
}
inner(py, $(self.$n.into_py(py)),+)
}

#[cfg(feature = "experimental-inspect")]
Expand All @@ -310,12 +312,13 @@ fn type_output() -> TypeInfo {

impl <$($T: IntoPy<PyObject>),+> IntoPy<Py<PyTuple>> for ($($T,)+) {
fn into_py(self, py: Python<'_>) -> Py<PyTuple> {
unsafe {
let ptr = ffi::PyTuple_New($length);
let ret = Py::from_owned_ptr(py, ptr);
$(ffi::PyTuple_SetItem(ptr, $n, self.$n.into_py(py).into_ptr());)+
ret
#[inline]
fn inner(py: Python<'_>, $($refN: PyObject),+) -> Py<PyTuple> {
unsafe {
Py::from_owned_ptr(py, ffi::PyTuple_Pack($length, $($refN.as_ptr()),+))
}
}
inner(py, $(self.$n.into_py(py)),+)
}

#[cfg(feature = "experimental-inspect")]
Expand Down