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.

Sign up for GitHub

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

Jump to bottom

[WIP] add bounded versions of f2s(), d2s() et al #184

Closed
wants to merge 3 commits into from
Closed

[WIP] add bounded versions of f2s(), d2s() et al #184

wants to merge 3 commits into from

Conversation

biojppm
Copy link

@biojppm biojppm commented Oct 12, 2020 •
edited
Loading

This PR sketches a possible implementation / fix #183 . Early feedback would be appreciated.

I'd like to make it clear that this is only a suggestion, and that it does come at a cost (20%):

3x ryu_benchmark[BEFORE]: // from master
    Average & Stddev Ryu  Average & Stddev Grisu3
32:  168.158   29.956      839.107  102.448
32:  180.789   47.192      898.842  201.735
32:  176.231   37.091      885.192  150.919
64:  321.055   70.136      993.035  218.611
64:  312.228   66.931      960.860  168.603
64:  317.734   74.092      986.843  197.059

4x ryu_benchmark[AFTER]: // from this branch
    Average & Stddev Ryu  Average & Stddev Grisu3
32:  192.162   17.935      831.047   72.141
32:  206.995   50.129      884.922  187.032
32:  211.375   43.112      895.407  171.450
32:  214.633   56.386      908.859  222.498
64:  346.018   21.572      920.093   45.274
64:  373.713   73.352      977.686  169.832
64:  392.833   75.857     1001.664  176.876
64:  368.120   74.838      965.297  176.205

Another possible approach is having a reasonable 100% safe size for every function. Can one be found? If it can, then maybe we can have our cake and it too:

#define RYU_MAX_D2S_SIZE 26 // what would be valid for this and other functions as well?
int d2s_buffered_sz(double f, char* result, int size)
{
     if(size < RYU_MAX_D2S_SIZE) { return RYU_MAX_D2S_SIZE; } // play safe by always requiring at least that much
     return d2s_buffered(f, result);
}

// likewise for f2s(), d2exp(), d2fixed() etc

@biojppm biojppm marked this pull request as draft October 12, 2020 18:20
@Artoria2e5
Copy link

Artoria2e5 commented Oct 14, 2020 •
edited
Loading

Drawing from https://reviews.llvm.org/D70631 there should be a few safe sizes. For the general-format case (referring to d2fixed exp and sci) we have:

constexpr int _Max_output_length =
    _IsSame<_Floating, float>::value
        ? 117
        : 773; // cases: 0x1.fffffep-126f and 0x1.fffffffffffffp-1022
constexpr int _Max_fixed_precision =
    _IsSame<_Floating, float>::value
        ? 37
        : 66; // cases: 0x1.fffffep-14f and 0x1.fffffffffffffp-14
constexpr int _Max_scientific_precision =
    _IsSame<_Floating, float>::value
        ? 111
        : 766; // cases: 0x1.fffffep-126f and 0x1.fffffffffffffp-1022

The non-printf ryu stuff should have much smaller values, but I don't know what they are.

@ulfjack
Copy link
Owner

ulfjack commented Oct 15, 2020

Max length for s2f is 15 (plus '\0') and for s2d is 24 (plus '\0'). If given a shorter buffer, another option is to generate into a temp buffer and then copy the first n characters over. That would avoid the performance penalty in the case where the buffer is large enough. I'd have to look into the fixed, exp, and sci cases more closely (life is hectic right now).

@biojppm biojppm closed this Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: add bounded versions of f2s(), d2s() et al
3 participants
@biojppm @Artoria2e5 @ulfjack