Optimize bytes-like (l|r)strip #4500
Conversation
| 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.
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.
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.
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>.
vm/src/builtins/bytes.rs
Outdated
| 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() { |
There was a problem hiding this comment.
I think &stripped == zelf.as_bytes() works, but still stripped is Vec<u8>.
| 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() { |
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).