dec: use decimal representation when converting to Decimal #86
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
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>
ParkMyCar
approved these changes
May 7, 2025
| // NOTE: The code is generic over any T: LowerExp but passing something like f128 wouldn't | ||
| // always work since there are f128 values that don't fit in 24 bytes. | ||
| pub fn from_float<T: LowerExp>(&mut self, n: T) -> Decimal<N> { | ||
| pub fn from_float<T: LowerExp + Display>(&mut self, n: T) -> Decimal<N> { |
There was a problem hiding this comment.
We should probably mark this #[inline(never)] like std does, wdyt?
Contributor
Author
There was a problem hiding this comment.
std marks it as such because the entry point of the user facing interface (i.e the Display impl on f64) goes through this function that only conditionally calls float_to_decimal_common_exact. So #[inline(never)] serves as a means to not use the large stack space if fmt.options.precision is None.
In our case we call from_float uncoditionally so the stackspace will be used no matter what, which makes inlining not problematic.
There was a problem hiding this comment.
Makes sense! Thanks for double checking!
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 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.