Optimize bytes-like (l|r)strip - #4500
Conversation
Sorry, something went wrong.
| fn lstrip( | ||
| zelf: PyRef<Self>, | ||
| chars: OptionalOption<PyBytesInner>, | ||
| vm: &VirtualMachine, | ||
| ) -> PyRef<Self> { |
There was a problem hiding this comment.
I found bytes.lstrip has different issue to str.lstrip. zelf.inner.lstrip already returns Vec<u8>, which means unnecessary copy happened.
I think this duplication check need to be done inside zelf.inner.lstip. otherwise zelf.inner.lstip also could return &[u8] instead of Vec<u8>, but it may not be easy due to lifetime.
Sorry, something went wrong.
There was a problem hiding this comment.
I see. If the zelf.inner.lstrip returns &[u8] is it possible to turn &[u8] to Vec<u8> without copying the values or is there a implementation for turning &[u8] directly to PyBytesInner? Because otherwise zelf.inner.elements is unnecessarily copied in the case of duplication.
Sorry, something went wrong.
There was a problem hiding this comment.
Could wrapping the return value of zelf.inner.lstrip in Option be an option? In that case it would return None on duplication and Some(stripped) would be returned in other cases.
Sorry, something went wrong.
There was a problem hiding this comment.
PyBytesInner is a wrapper around Vec<u8>, so allocation is inevitable. Fortunately, converting a Vec<u8> into a PyBytesInner only takes ownership of that Vec<u8>.
Sorry, something went wrong.
fanninpm
left a comment
There was a problem hiding this comment.
Does this work? I don't think we need the explicit conversion in these two places, as PartialEq<&[U]> is implemented for Vec<T, A>.
Sorry, something went wrong.
| vm: &VirtualMachine, | ||
| ) -> PyRef<Self> { | ||
| let stripped = zelf.inner.lstrip(chars); | ||
| if stripped == zelf.as_bytes().to_vec() { |
There was a problem hiding this comment.
Does this work?
| if stripped == zelf.as_bytes().to_vec() { | |
| if stripped == zelf.as_bytes() { |
Sorry, something went wrong.
There was a problem hiding this comment.
I think &stripped == zelf.as_bytes() works, but still stripped is Vec<u8>.
Sorry, something went wrong.
| vm: &VirtualMachine, | ||
| ) -> PyRef<Self> { | ||
| let stripped = zelf.inner.rstrip(chars); | ||
| if stripped == zelf.as_bytes().to_vec() { |
There was a problem hiding this comment.
| if stripped == zelf.as_bytes().to_vec() { | |
| if stripped == zelf.as_bytes() { |
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
Great, Thank you!
Sorry, something went wrong.
Hi!
Here is a solution proposal to #4497. I was not quite sure what the optimization part of #4496 was (I assumed it was the if-else part).