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

collect arrays of objects prior to filling tuple in fixed-size conversions #3321

Merged
merged 2 commits into from
Jul 16, 2023

Conversation

davidhewitt
Copy link
Member

Replacement to #3296.

As discussed there, we believe it's safe to use PyTuple_New when there is confidence of no Python code running while filling the uninitialized tuple.

Therefore in this PR I adjust the fixed-size conversions to collect into an array and then fill the tuple. I also happened to manage to optimize this by use of a const-generic helper which uses PyTuple_SET_ITEM (on the stable abi).

Benchmark shows this is ~20% faster than main, and should be safer too:

main
tuple_into_py           time:   [59.337 ns 59.947 ns 60.588 ns]

safer-tuple-into-py
tuple_into_py           time:   [46.143 ns 46.567 ns 47.005 ns]

I tried the same thing for PyTuple::new (the variable sized constructor) with a Vec, but that worked out around 20% slower, so I haven't included that here for now.

src/types/tuple.rs Outdated Show resolved Hide resolved
@adamreichold adamreichold added the CI-skip-changelog Skip checking changelog entry label Jul 16, 2023
Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

I tried the same thing for PyTuple::new (the variable sized constructor) with a Vec, but that worked out around 20% slower, so I haven't included that here for now.

While it is nice that this is an improvement, I think we should compare the cost of the above to the safe version using PyTuple_Pack. We can either use the Vec and PyTuple_New or PyTuple_Pack for the dynamically sized case.

@davidhewitt
Copy link
Member Author

I tried the same thing for PyTuple::new (the variable sized constructor) with a Vec, but that worked out around 20% slower, so I haven't included that here for now.

While it is nice that this is an improvement, I think we should compare the cost of the above to the safe version using PyTuple_Pack. We can either use the Vec and PyTuple_New or PyTuple_Pack for the dynamically sized case.

I haven't worked out how to use PyTuple_Pack for the dynamically sized case; if there's something I'm missing, would be happy for someone to show me the trick :)

@adamreichold
Copy link
Member

I haven't worked out how to use PyTuple_Pack for the dynamically sized case; if there's something I'm missing, would be happy for someone to show me the trick :)

Ah, they chose a variadic interface which is not nice for composeability, c.f. https://c-faq.com/varargs/invvarargs.html

I guess libffi could do it, e.g. https://docs.rs/libffi/latest/libffi/low/fn.prep_cif_var.html, but I am pretty sure we do not actually want that.

So I guess optimizing the common case using something like SmallVec<[PyObject; 12]> might be helpful? Otherwise, we just leave that one open until a better replace for PyTuple_New comes along?

@davidhewitt davidhewitt added this pull request to the merge queue Jul 16, 2023
Merged via the queue into PyO3:main with commit 94dd6dc Jul 16, 2023
31 checks passed
@davidhewitt davidhewitt deleted the safer-tuple-into-py branch July 16, 2023 20:53
@davidhewitt
Copy link
Member Author

So I guess optimizing the common case using something like SmallVec<[PyObject; 12]> might be helpful? Otherwise, we just leave that one open until a better replace for PyTuple_New comes along?

Yes, if someone is interested in trying this out it might be interesting. It sounds like there's some upstream debate on what the best API is so I'm inclined to leave as-is for now and let the dust settle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants