Skip to content

Minimize use of GC for doubles in printFloat. #7767

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
Merged
merged 1 commit into from
Jan 28, 2021
Merged

Minimize use of GC for doubles in printFloat. #7767

merged 1 commit into from
Jan 28, 2021

Conversation

berni44
Copy link
Contributor

@berni44 berni44 commented Jan 24, 2021

A fixup for #7393. Now it's guaranteed, that printing float and double with %a or %e will not use gc as long as the output is restricted to 500 characters. (Without this change, this guarantee did only hold for floats but not for doubles.)

@berni44 berni44 requested a review from andralex as a code owner January 24, 2021 15:52
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7767"

berni44
berni44 commented Jan 24, 2021
View reviewed changes
std/format.d
@@ -8448,7 +8448,7 @@ printFloat_done:
import core.memory;
auto stats = GC.stats;

char[256] buf;
char[512] buf;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this, because the call in formatValueImpl also provides a buffer with 512 chars.

berni44 added a commit to berni44/phobos that referenced this pull request Jan 24, 2021
@berni44
[SQUASH] Fixup, see PR dlang#7767.
5d84d8e
RazvanN7
RazvanN7 reviewed Jan 25, 2021
View reviewed changes
Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan on making printFloatE completely @nogc ?

std/format.d
@@ -7754,7 +7754,7 @@ if (is(T == float) || is(T == double) || (is(T == real) && T.mant_dig == double.
// digits. If still necessary, we decide the rounding type, mainly by looking at the
// next digit.

ulong[4] bigbuf;
ulong[18] bigbuf;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N00b question: why 18?

std/format.d
@@ -7764,7 +7764,7 @@ if (is(T == float) || is(T == double) || (is(T == real) && T.mant_dig == double.
int count = exp / 60 + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that the value of count cannot ever be greater than 18?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. For float it cannot be greater than 3 and for double it cannot be greater than 18. (exp is maximum 127/1023).

RazvanN7
RazvanN7 approved these changes Jan 25, 2021
View reviewed changes
Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if in the future we could get rid of allocations for the entire function and maybe introduce a @nogc unittest.

@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jan 25, 2021
@berni44
Copy link
Contributor Author

berni44 commented Jan 25, 2021

Do you plan on making printFloatE completely @nogc ?

This will be hard to achieve this goal and might reduce the speed considerably. The reason is, that the output could be arbitrarily large (in theory). To make this @nogc it would be necessary to process the output from left to right, which makes for a much more complicated algorithm. One of the hard things is to calculating the exact length of the useful characters in advance for all combinations of flags, precision, width and rounding mode.

Processing from left to right involves a BigInt multiplication with a large and a very large number. When not using std.BigInt but a specialized algorithm to do that, this might improve the speed for %e (bit will slow down %f). I plan to implement this (for %e only), but this would only be the first step toward @nogc on a long road.

If you wonder where's the difference between %e and %f here, compare format("%e",double.max) (1.797693e+308) vs. format("%f",double.max) (179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000). The current algorithm calculates the whole 330 digits for %e and %f. It's difficult to imagine any improvement for %f, while it's obvious, that for %e it would suffice to calculate only the first view digits; but due to rounding issues, you cannot predict in advance the number of needed digits...

@RazvanN7 RazvanN7 added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Jan 28, 2021
@dlang-bot dlang-bot merged commit c53e9f5 into dlang:master Jan 28, 2021
berni44 added a commit to berni44/phobos that referenced this pull request Jan 28, 2021
@berni44
[SQUASH] Fixup, see PR dlang#7767.
a9ee88c
@berni44 berni44 deleted the float_gc branch February 13, 2021 13:46
berni44 added a commit to berni44/phobos that referenced this pull request Feb 15, 2021
@berni44
[SQUASH] Fixup, see PR dlang#7767.
46f49d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RazvanN7 RazvanN7 RazvanN7 approved these changes

@andralex andralex Awaiting requested review from andralex

Assignees
No one assigned
Labels
Merge:auto-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@berni44 @dlang-bot @RazvanN7