Remove allocation in impl Display for path::Display #38879

[martinhath]

Fixes #38879

I wasn't too sure about testing this, but I threw in a test for checking that the non-unicode sequences were converted to '\u{fffd}'

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nagisa]

I’d much rather this do 1 allocation for a cold case (invalid unicode) rather than have write_char called for every single codepoint every single time.

[martinhath]

Do you mean performance wise? I think it would be interesting to see some benchmarks here.

[martinhath]

Ok, so I hacked together a quick bench of the two options. On my system, with a path length of 8, with illegal unicode bytes, the benchmarks reported about the same running time. With legal unicode paths, the to_lossy_str was always faster (path length of 8 was the shortest I tested). The difference was a between a factor of two and five, depending on the path length.

Now, allocation time is of course system dependent so the exact numbers here aren't written in stone, but seeing what kind of numbers we are talking about here, I'm not sure this is worth pursuing.

I guess the results are expected, so unless @jethrogb have some input here I'll close this PR.

[jethrogb]

You can test whether str::from_utf8 returns an error or not before moving into the slow path. This is what to_string_lossy does.

[martinhath]

That is certainly possible, but seeing how small the gains seems to be, and that checking for illegal utf8 before deciding on the path will slow down the function even further, I'm struggling to imagine a scenario where we'll end up on the plus side here.

[jethrogb]

If it's valid, you can just push the resulting &str through Formatter::write_str

[hanna-kruppe]

The write_char in the loop means that the bytes are not only checked for UTF-8 validity (unavoidable) but moreover the decoded code points are then re-encoded and pushed on the buffer piecemeal. It could be much more efficient to try and decode, but write the UTF-8 parts of the source string directly rather than encoding again and going char-by-char.

However, that is only economical if the nitty gritty part is already implemented, i.e., if there's existing code that validates the bytes and tells you where (if at all) the first non-UTF-8 byte is. I don't know if we have something like that.

[jethrogb]

@rkruppe that's pretty much the to_string_lossy internals...

[hanna-kruppe]

Great! Can the relevant parts be accessed from the outside, or can the code easily be refactored to allow that?

[clarfonthey]

IMHO this should be a double-check for invalid unicode: check if the slice is valid unicode and If so, write_str, otherwise, do the write_char loop.

This way, you get the proper optimisation on the average case (valid UTF-8), and the allocation in the uncommon case is replaced by two validity checks. Still better than before, but doesn't incur a performance penalty in the average case.

Perhaps in the long-term if we want to optimise this better on the fmt::Write side, we could offer a write_chars which takes in an iterator, and then specialise it depending on what's passed in.

Side note: It'd be nice if we had a no-allocate version of to_string_lossy so that people can use this algorithm without having to re-implement every time they need it.

[aturon]

I'm going to close out this PR; as it currently stands, it's a net loss to performance and introduces a bit more implementation complexity. We could consider exposing the to_string_lossy internals a bit more, but we'd need to see some strong performance numbers showing a benefit.

Thanks for the PR!