Implement DecInt without itoa
#1245
Conversation
There was a problem hiding this comment.
This looks reasonable to me. @SUPERCILEX do you have any concerns about replacing the itoa crate?
If rustix will have its own integer formatting code, could you also add tests covering the min, max and zero cases, and some other assorted positive and negative values, of the supported types, and test that the NUL termination is correct?
|
I don't have strong objections, but I disagree with the premise. There's no memcpy in the assembly: https://rust.godbolt.org/z/qj3zoE7b6 (though there are mov instructions and I haven't looked at it deep enough to see if that's an inlined memcpy). Can we see a comparison of the assembly please? If it's smaller then code size is a good argument to merge this PR. Otherwise maintaining our own lesser version of itoa seems like a bad idea. |
In the code you linked in line 79 of the assembly code memcpy is called. Will address the other questions this afternoon/evening (CET). |
|
My bad, turns out if you Ctrl+f on godbolt (mobile website) it's not actually rendering all lines so it didn't find anything. Ok I take back my objection then, I think as long as the code size isn't bad this is an improvement. I'm definitely sad that we have to maintain our own itoa though. |
|
That's the generated code: https://godbolt.org/z/jdPKs4h7h. |
src/path/dec_int.rs
Outdated
| buf, | ||
| len: str_buf.len(), | ||
| // SAFETY: cannot be less than 2 (initialized with 1 + at least one loop) | ||
| len: unsafe { NonZeroU8::new_unchecked(len as u8) }, |
There was a problem hiding this comment.
No need for the unsafe, the compiler optimizes out len: NonZeroU8::new(len as u8).unwrap(),.
src/path/dec_int.rs
Outdated
| @@ -96,7 +218,12 @@ impl DecInt { | |||
| /// Return the raw byte buffer including the NUL byte. | |||
| #[inline] | |||
| pub fn as_bytes_with_nul(&self) -> &[u8] { | |||
| let init = &self.buf[..=self.len]; | |||
| let len = self.len.get() as usize; | |||
There was a problem hiding this comment.
Use usize::from(self.len.get()).
Using `itoa` is unlikely to have any actual benefits for any reasonable usecase. You are unlikely to format lots of integers in a tight loop without performing IO operations in between, so any speed-benefits of `itoa` are likely moot. The buffer `itoa` writes to is not re-used by `DecInt`, which makes the generated code use a `memcpy` call. `itoa`'s buffer is twice the size as we need, because it is implemented to be able to hold an `i128`/`u128`. The `i128`/`u128` implementation of `DecInt` would panic if the input was outside of `-10**19 + 1 ..= 10**20 - 1`. This PR implements the integer stringification naively using a div-mod-10 loop. Users will be able to use integers as path arguments without opting-in to any feature. This is a breaking change because the feature `itoa` was removed, and because the implementation for `i128`/`u128` was removed.
Using
itoais unlikely to have any actual benefits for any reasonable usecase. You are unlikely to format lots of integers in a tight loop without performing IO operations in between, so any speed-benefits ofitoaare likely moot. The bufferitoawrites to is not re-used byDecInt, which makes the generated code use amemcpycall.itoa's buffer is twice the size as we need, because it is implemented to be able to hold ani128/u128. Thei128/u128implementation ofDecIntwould panic if the input was outside of-10**19 + 1 ..= 10**20 - 1.This PR implements the integer stringification naively using a div-mod-10 loop. Users will be able to use integers as path arguments without opting-in to any feature.
This is a breaking change because the feature
itoawas removed, and because the implementation fori128/u128was removed.Resolves #1202.