RUST-1798 use simd to optimize utf8 validation #440
Conversation
isabelatkinson
left a comment
isabelatkinson
left a comment
There was a problem hiding this comment.
A few suggestions, it looks like there are also some formatting errors. Can you please run the following on the files you've changed?
rustfmt +nightly --unstable-features
| /// A [`std::string::FromUtf8Error`](https://doc.rust-lang.org/std/string/struct.FromUtf8Error.html) encountered | ||
| /// while decoding a UTF-8 String from the input data. | ||
| InvalidUtf8String(string::FromUtf8Error), | ||
| InvalidUtf8String(Utf8Error), |
There was a problem hiding this comment.
Changing the type contained in this error variant is a breaking change and would require a major version release, which we are not planning to do in the near term. Can a simdutf8::simple::Utf8Error be mapped to a string::FromUtf8Error to avoid this?
There was a problem hiding this comment.
The std's String::FromUtf8Error does not expose any constructor. So may be this can not be avoided ...
There was a problem hiding this comment.
Got it. Unfortunately we'll need to hold off on this until we can change our error types. I've added the 3.0 version to RUST-1798 to pick up this work again when we do our next major version release.
| fn to_string(v: Vec<u8>) -> Result<String> { | ||
| let _ = simdutf8::basic::from_utf8(&v)?; | ||
| // Safety: `v` is a valid UTF-8 string. | ||
| unsafe { Ok(String::from_utf8_unchecked(v)) } |
There was a problem hiding this comment.
Any reason the result of the first line of this function cannot be converted to a String and returned directly?
fn to_string(v: Vec<u8>) -> Result<String> {
let str = simdutf8::basic::from_utf8(&v)?;
Ok(str.to_string())
}
I'd like to avoid unsafe code if we do not need it here.
There was a problem hiding this comment.
str.to_string() will make a copy of original bytes. It may be even slower then the std's String::from_utf8.
Also, you can see the issue here.
isabelatkinson
changed the title
|
I know it's been a long time but I just merged in #548 based on this PR, to go live when we release 3.0 of the bson crate :) |
4 participants
#437