Skip to content

[BUG]: string_caster should not use temporary on Python >= 3.3 #3252

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

Closed
3 tasks done
jbms opened this issue Sep 9, 2021 · 2 comments · Fixed by #3257
Closed
3 tasks done

[BUG]: string_caster should not use temporary on Python >= 3.3 #3252

jbms opened this issue Sep 9, 2021 · 2 comments · Fixed by #3257
Labels

Comments

@jbms
Copy link
Contributor

jbms commented Sep 9, 2021

Required prerequisites

Problem description

Currently, string_caster always creates a temporary PyBytes object by calling PyUnicode_AsEncodedString. However, in the common case of UTF_N == 8, on Python >= 3.3 we can instead use PyUnicode_AsUTF8AndSize, which manages and caches the UTF-8 encoding internally within the PyUnicode object. If the UTF-8 representation is cached, then the encoding does not have to be done at all, avoiding an extra copy of the string.

This is particularly advantageous in the IsView == true case: users likely expect casting from PyUnicode to std::string_view to be low cost, but currently it always involves a copy of the string. Additionally, it the IsView == true case has the additional cost of relying on loader_life_support::add_patient, which introduces additional cost and additional memory allocations (e.g. with the change in #3237, an allocation of the unordered_set bucket array on first use of loader_life_support, and an additional allocation of the node).

Reproducible example code

No response

@jbms jbms added the triage New bug, unverified label Sep 9, 2021
@Skylion007
Copy link
Collaborator

@jbms PR on this front is welcome, would you mind submitting one?

@jbms
Copy link
Contributor Author

jbms commented Sep 9, 2021

@Skylion007 Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants