Pybytes specialization slices

[davidhewitt]

This is yet another attempt to optimise #4417. This time, I was able to achieve near-zero performance abstraction over the top of PyBytes::new.

The realisation was that the &[T] and Cow<[T]> implementations were dependent on having a new implementation impl IntoPyObject for &u8. If we remove that implementation, then we can instead have specialized implementations directly for &[u8] and Cow<[u8]>which don't conflict with the blanket implementation for &[T] or Cow<[T]>.

The downside of this approach is that we permanently lose the ability to implement IntoPyObject for &u8. But personally I don't see that as much of a problem; we didn't have that implementation before. This also prevents us from supporting sequences like Vec<&u8>, but again I sort of find that type weird and it doesn't bother me too much if we ask users to do something special with it themselves.

[davidhewitt]

Thinking about this a bit further; we already have impl ToPyObject for u8 which was approximately impl IntoPyObject for &u8, so maybe not having IntoPyObject for &u8 is more breaking than I might like.

[davidhewitt]

It's probably ok to go this route and not have impl IntoPyObject for &u8, but users might have to migrate code by adding a dereference to copy the &u8 into a u8.

That said, the compiler error currently doesn't suggest that. Maybe we can suggest rustc to improve the diagnostic here:

error[E0277]: `&u8` cannot be converted to a Python object
   --> test.rs:10:43
    |
 10 |             let obj = takes_into_pyobject(x);
    |                       ------------------- ^ the trait `conversion::IntoPyObject<'_>` is not implemented for `&u8`
    |                       |
    |                       required by a bound introduced by this call
    |
    = note: `IntoPyObject` is automatically implemented by the `#[pyclass]` macro
    = note: if you do not wish to have a corresponding Python type, implement it manually
    = note: if you do not own `&u8` you can perform a manual conversion to one of the types in `pyo3::types::*`
    = help: the trait `conversion::IntoPyObject<'_>` is implemented for `u8`
    = help: for that trait implementation, expected `u8`, found `&u8` 

[Icxolu]

Very interesting approach! I think not providing impl IntoPyObject for &u8 will have an considerable impact on the generic APIs that we can provide(, or rather how they work). For example I have started working on providing impls for references to collections like &HashSet<K> which will require &'a K: IntoPyObject<'py>, so a &HashSet<u8> will not be covered. This does not feel to uncommon to me (maybe we store a HashSet<u8> in a pyclass and access it via &self), so I'm a bit skeptical if it's a good idea to exclude &u8. I have the feeling that it will create many surprising corner cases...

Maybe we could combine this approach with #4424. That way we have the fast and efficient PyBytes::new implementation for the owning collection via AsRef<[Self]> from here and the borrowing collections will take a slight performance hit using the PyBytes::from_iterator approach from #4424 .

[davidhewitt]

Agreed, we need the &u8 implementation.

Maybe we could combine this approach with #4424. That way we have the fast and efficient PyBytes::new implementation for the owning collection via AsRef<[Self]> from here and the borrowing collections will take a slight performance hit using the PyBytes::from_iterator approach from #4424 .

This turned out to be a fantastic suggestion; and running with it managed to do one step further. I had to split the private method into two, owned_sequence_into_pyobject (used by Vec<u8>) and borrowed_sequence_into_pyobject (used by &[u8]), and needed a helper trait Reference to describe the slice in borrowed_sequence_into_pyobject as AsRef<[<Self as Reference>::BaseType]>.

That lets me restore impl IntoPyObject for &u8 while also managing to specialize slices on &[u8].

Now I think we have all the implementations we want, and everything safely uses PyBytes::new! 🎉🤯

This has consumed far too much of my spare brain cycles in the last week, but it's been fun 😂

If you're happy with how this looks, I think this might now be good to merge?

[Icxolu]

Woo Hoo 🥳 Very clever using another trait to specialize on references and referring to their owned type.

This has consumed far too much of my spare brain cycles in the last week, but it's been fun 😂

Thank you very much for putting in that time. I think now we landed on a pretty good solution that we can make good use of the future. I think borrowed_sequence_into_pyobject will come in handy for a few more impls on reference types.

If you're happy with how this looks, I think this might now be good to merge?

I think so too 🎉