CodSpeed Performance Report

Merging #4205 will not alter performance

_{Comparing JRRudy1:pybackedstr-intopy (43fb3f7) with main (cb34737)}

Summary

✅ 67 untouched benchmarks

I actually just noticed that the conversion is not as trivial when cfg(not(any(Py_3_10, not(Py_LIMITED_API)))) since the backing PyString gets converted to a PyBytes when the PyBackedStr is created; this isn't necessarily a problem, but means the object passed back to Python would be bytes instead of str as expected. I just added attributes to limit these impls to the trivial case, but maybe this is not the best solution. I see three options:

1. limit the impls as I just did
2. allow the impls unconditionally, accepting that the Python object will be bytes in some cases
3. flesh out the impls to handle the limited-API case by converting back to PyString; I would need some advice on the correct way to do this.

Regarding the CI, do you have any tips on why it would be failing and what I can do to fix that? The change seems too small to actually effect anything

https://github.com/PyO3/pyo3/actions/runs/9238604511/job/25416822459?pr=4205#step:7:23

It looks like cargo fmt will rearrange the imports.

3. flesh out the impls to handle the limited-API case by converting back to PyString; I would need some advice on the correct way to do this.

Something like PyString::new(py, self) probably works, because self as a PyBackedStr will deref to a str.

Something like PyString::new(py, self) probably works, because self as a PyBackedStr will deref to a str.

I guess I was overthinking it because I was expecting to need some ffi wizardry like PyUnicode_FromString or something haha. But is my understanding correct that the limited-API case will involve copying the whole string, while the other case simply copies a reference to the original string?

I updated the impls and will look into adding some tests.

I went ahead and added ToPyObject and IntoPy<PyObject> impls for PyBackedBytes as well, but it is a bit unclear whether the PyBackedBytesStorage::Rust variant should covert to PyBytes or PyByteArray. Mirroring the FromPyObject impl suggests PyByteArray so this is what I went with, but now I'm questioning whether it would be more intuitive to consistently return PyBytes in both cases. The data is getting copied either way so I don't see a technical reason to prefer one over the other.

What do you think @davidhewitt?

I also added a couple tests, but let me know if you have any suggestions to improve them.

I changed my mind and updated the PyBackedBytes conversion impls to return a Python bytes object regardless of the backing storage type. My logic is that the storage type is an implementation detail, and the user shouldn't have to know what it is in order to predict the resulting Python type. The Rust type is called "PyBackedBytes", so intuitively the Python type should be bytes as well. The only weird aspect is that the round-trip conversion would be PyByteArray -> PyBackedBytes -> PyBytes instead of returning to PyByteArray where it started, but I think that's a worthy tradeoff.

I think one of the runners crashed and the checks need to be restarted