Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue in float serialization #1188

Merged
merged 11 commits into from
Jul 20, 2024
Merged

Fix issue in float serialization #1188

merged 11 commits into from
Jul 20, 2024

Conversation

stephenberry
Copy link
Owner

@stephenberry stephenberry commented Jul 20, 2024

This merge uses dragonbox underneath to ensure proper serialization of floats.

@stephenberry stephenberry linked an issue Jul 20, 2024 that may be closed by this pull request
@stephenberry stephenberry changed the title dtoa cleanup Fix issue in float serialization Jul 20, 2024
@stephenberry stephenberry merged commit 9954027 into main Jul 20, 2024
8 checks passed
@stephenberry stephenberry deleted the dtoa_cleanup branch July 20, 2024 19:31
@pauldreik
Copy link
Contributor

pauldreik commented Jul 20, 2024

This does not look right - 1.40129846e-45 gets serialized to {"value":1.E-45} which is ok (it is denormalized) but it cant be deserialized by glaze.

following the railroad diagram at https://www.json.org/json-en.html, it seems like the dot must be followed by a digit.

@stephenberry
Copy link
Owner Author

Thanks for pointing this out. Fixed in #1190

@jk-jeon
Copy link

jk-jeon commented Sep 1, 2024

By the way I noticed that you are calling dragonbox::to_decimal. There is an alternative API dragonbox::to_decimal_ex which takes the decomposed sign-significand bits and exponent bits. Since you are doing this decomposition by yourself anyway, calling dragonbox::to_decimal instead of dragonbox::to_decimal_ex will pointlessly duplicate this job.

I'm expecting that maybe the performance gain you can obtain by calling dragonbox::to_decimal_ex directly rather than indirectly via dragonbox::to_decimal is barely noticeable, since JSON serialization is much much much more than floating-point number formatting, obviously. However, I generally aim for the API that allows the absolute zero-cost integration into the actual formatting code, and currently I'm not completely sure what exactly should be the right parameters that dragonbox::to_decimal_ex is supposed to take to eliminate any possible API friction, which is why I consider it as experimental at this moment. This is subject to change until the next release. And this is why inputs from real users like Glaze would be extremely valuable to me.

I would appreciate it if you can investigate the possibility of replacing your call to dragonbox::to_decimal by dragonbox::to_decimal_ex, and give me any feedback. To be more precise, it would be a real issue for me if fitting into the API causes any more assemblies to be generated than necessary, while just some small inconvenience of needing to cast things around feels not really that important.

If you're interested, these two are the points relevant to usage of dragonbox::to_decimal_ex:

https://github.com/jk-jeon/dragonbox/blob/master/include/dragonbox/dragonbox_to_chars.h#L220
https://github.com/jk-jeon/dragonbox/blob/master/include/dragonbox/dragonbox_to_chars.h#L264

@stephenberry
Copy link
Owner Author

stephenberry commented Sep 19, 2024

@jk-jeon, Thanks so much for pointing this out. I just implemented this change here: #1316. But, I'm doing something wrong.

One of the complications of this interface is the compile time policies. It is critical for these policies to be compile time (for performance), but you have a lot of code that complicates your library for handling these policies and it makes your code harder to read and work with. Your approach is also more compile time intensive than it needs to be.

Since C++ now supports non-type template parameters, a much cleaner and faster to compile solution is to pass a struct of policies (e.g. options).

Glaze uses this approach with great success.

Here's a simple example:

struct policies
{
   bool option_one{};
   bool option_two{};
};

template <policies Policies = policies{}>
void func()
{

}

// Now a user can call this function via:
func<policies{.option_two = true}>();

// or, they can even make a struct of policies to pass around:
static constexpr policies policy{.option_two = true};
func<policies>();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

converting float -8536070.f to string gives "-08536070"
3 participants