add parsing variant that doesn't allocate #83
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
force-pushed
the
non-allocating-parsing
branch
from
e81561c to
0d76ca8
Compare
ParkMyCar
approved these changes
Apr 29, 2025
| // * 3 bytes for the largest possible exponent (308) | ||
| // An example of such maximal float value is f64::from_bits(0x8008000000000000) whose | ||
| // decimal representation is '-1.1125369292536007e-308' | ||
| const MAX_LEN: usize = 24; |
There was a problem hiding this comment.
This makes sense to me! Just double checked and ryu also sizes their buffer with 24 bytes so these seems quite safe
Contributor
Author
There was a problem hiding this comment.
Nice to see it validated elsewhere!
|
|
||
| let mut buf = [0u8; MAX_LEN + 1]; | ||
| let mut unwritten = &mut buf[..MAX_LEN]; | ||
| write!(unwritten, "{:e}", n).unwrap(); |
There was a problem hiding this comment.
IIRC the write! macro has a decent amount of machinery internally that it might be nice to bypass, not a huge priority though.
Contributor
Author
There was a problem hiding this comment.
Talked on slack, doesn't seem to be much we do here so leaving it as-is
| // decimal representation is '-1.1125369292536007e-308' | ||
| const MAX_LEN: usize = 24; | ||
|
|
||
| let mut buf = [0u8; MAX_LEN + 1]; |
There was a problem hiding this comment.
It seems like this +1 is to ensure the last byte is a null character for the C String? mind documenting that?
petrosagg
force-pushed
the
non-allocating-parsing
branch
from
0d76ca8 to
132d7ae
Compare
antiguru
requested changes
Apr 29, 2025
petrosagg
force-pushed
the
non-allocating-parsing
branch
from
132d7ae to
b906875
Compare
Contributor
Author
|
@antiguru good catch! Pushed a change to fix this. I added a generic method for both float sizes that calls into the appropriate stringify method |
antiguru
approved these changes
Apr 29, 2025
Member
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
Thanks for addressing the f32 issue!
dec/src/decimal.rs
Outdated
| self.from_float(n) | ||
| } | ||
|
|
||
| /// Converts an `f64` to a `Decimal<N>`. |
Member
There was a problem hiding this comment.
The description is now incorrect. Also, we should ensure that we only pass f32|f64 here, and not accidentally f128 in the future.
Contributor
Author
There was a problem hiding this comment.
Indeed, I'll fix the description. I'm not super concerned about ensuring statically that T is either f32 or f64 since you can't cause UB by doing that but I will document it for the future engineer
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
force-pushed
the
non-allocating-parsing
branch
from
b906875 to
2af10a8
Compare
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
force-pushed
the
non-allocating-parsing
branch
from
4d012b1 to
c7db9da
Compare
petrosagg
added a commit
that referenced
this pull request
May 7, 2025
This PR fixes a regression introduced in #83 which started serializing floats using their scientific notation, which is much shorter than the decimal notation and therefore faster. It turns out that conversion from string to decimal is syntactic instead of semantic. This means that the strings "12" and "1.2E2", which are semantically the same number, parse into a different Decimal instance. This PR fixes the regression by serializing to the decimal representation. The overall improvement from 0.4.9 is much more mild as a result. ``` parse_decimal time: [2.4036 µs 2.4180 µs 2.4389 µs] change: [-22.670% -21.979% -21.336%] (p = 0.00 < 0.05) Performance has improved. ``` Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
added a commit
that referenced
this pull request
May 7, 2025
This PR fixes a regression introduced in #83 which started serializing floats using their scientific notation, which is much shorter than the decimal notation and therefore faster. It turns out that conversion from string to decimal is syntactic instead of semantic. This means that the strings "12" and "1.2E2", which are semantically the same number, parse into a different Decimal instance. This PR fixes the regression by serializing to the decimal representation. The overall improvement from 0.4.9 is much more mild as a result. ``` parse_decimal time: [2.4036 µs 2.4180 µs 2.4389 µs] change: [-22.670% -21.979% -21.336%] (p = 0.00 < 0.05) Performance has improved. ``` Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
added a commit
that referenced
this pull request
May 7, 2025
This PR fixes a regression introduced in #83 which started serializing floats using their scientific notation, which is much shorter than the decimal notation and therefore faster. It turns out that conversion from string to decimal is syntactic instead of semantic. This means that the strings "12" and "1.2E2", which are semantically the same number, parse into a different Decimal instance. This PR fixes the regression by serializing to the decimal representation. The overall improvement from 0.4.9 is much more mild as a result. ``` parse_decimal time: [2.4036 µs 2.4180 µs 2.4389 µs] change: [-22.670% -21.979% -21.336%] (p = 0.00 < 0.05) Performance has improved. ``` Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves the routines that convert
f64values intoDecimal<N>values by avoiding allocating large strings. The performance comes from:I have included a benchmark for the
from_f64method which shows a 4x improvement: