use PyTuple_Pack in fixed-size tuple conversions

[davidhewitt]

Ref capi-workgroup/problems#56

PyTuple_Pack is preferred over PyTuple_New to avoid partially uninitialised tuples.

I was curious about the performance impact so benchmarked it; this PR using PyTuple_Pack is about 10% slower. I think we just have to eat that slowdown for the correctness.

main
tuple_into_py           time:   [54.670 ns 54.775 ns 54.906 ns]

this PR
tuple_into_py           time:   [59.857 ns 59.868 ns 59.883 ns]
                        change: [+9.2827% +9.4570% +9.6517%] (p = 0.00 < 0.05)

[davidhewitt]

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.)

[adamreichold]

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.

[davidhewitt]

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. :)

[adamreichold]

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.

[davidhewitt]

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.

[adamreichold]

I think we just have to eat that slowdown for the correctness.

But do we really? If I understand this correctly, then while having PyTuple_AllocNoTrack would be nice, we could also soundly use PyTuple_New as long as we make sure to not yield into the Python interpreter until the tuple is fully initialized? In this case, wouldn't this just mean to make sure we run all the .into_py(py) conversions first and only then when we basically have a tuple of pointers start constructing our Python tuple? I think doing so with a comment on the pitfalls would be preferable to just dropping 10% performance onto the floor for such an important conversion supporting e.g. multiple return values.

[davidhewitt]

I think we just have to eat that slowdown for the correctness.

But do we really? If I understand this correctly, then while having PyTuple_AllocNoTrack would be nice, we could also soundly use PyTuple_New as long as we make sure to not yield into the Python interpreter until the tuple is fully initialized? In this case, wouldn't this just mean to make sure we run all the .into_py(py) conversions first and only then when we basically have a tuple of pointers start constructing our Python tuple? I think doing so with a comment on the pitfalls would be preferable to just dropping 10% performance onto the floor for such an important conversion supporting e.g. multiple return values.

Good point, I think you're right that careful use of PyTuple_New can be perfectly sound. Sadly in practice it's probably in new_from_iter where we have the greater risk of arbitrary code allowing observation of the partially initialized tuple. 🙈

I'll see what the response is to capi-workgroup/problems#56 (comment) and potentially revert to use PyTuple_New according to the outcome there.

[adamreichold]

Sadly in practice it's probably in new_from_iter where we have the greater risk of arbitrary code allowing observation of the partially initialized tuple. 🙈

Even there, I think we could fallibly collect the iterator into a Rust tuple or array and only when that is fully filled, start building our Python tuple. This should give the Rust compiler full visibility into the data movement while avoiding to fully initialize the Python tuple via the much narrower FFI.

[davidhewitt]

Closed in favour of #3321