Skip to content

number_format: float within range of int can be cast to int … #12333

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

Closed
wants to merge 3 commits into from

Conversation

marc-mabe
Copy link
Contributor

@marc-mabe marc-mabe commented Sep 30, 2023

…before rounding of numbers above 2^52 to not loose precision.

On casting float to int fractional digits gets truncated but as far as I'm aware of double numbers above 2^52 can't store fractions anymore.

This is the diff of the added test numbers without this change:

--
     --- testing: float(9.223372036854775E+18)
     ... with precision 5: string(31) "9,223,372,036,854,774,784.00000"
     ... with precision 0: string(25) "9,223,372,036,854,774,784"
169- ... with precision -1: string(25) "9,223,372,036,854,774,780"
170- ... with precision -5: string(25) "9,223,372,036,854,800,000"
169+ ... with precision -1: string(25) "9,223,372,036,854,774,784"
170+ ... with precision -5: string(25) "9,223,372,036,854,800,384"
     ... with precision -10: string(25) "9,223,372,040,000,000,000"
     ... with precision -11: string(25) "9,223,372,000,000,000,000"
     ... with precision -17: string(25) "9,200,000,000,000,000,000"
--
     --- testing: float(-9.223372036854775E+18)
     ... with precision 5: string(32) "-9,223,372,036,854,774,784.00000"
     ... with precision 0: string(26) "-9,223,372,036,854,774,784"
180- ... with precision -1: string(26) "-9,223,372,036,854,774,780"
181- ... with precision -5: string(26) "-9,223,372,036,854,800,000"
180+ ... with precision -1: string(26) "-9,223,372,036,854,774,784"
181+ ... with precision -5: string(26) "-9,223,372,036,854,800,384"
     ... with precision -10: string(26) "-9,223,372,040,000,000,000"
     ... with precision -11: string(26) "-9,223,372,000,000,000,000"
     ... with precision -17: string(26) "-9,200,000,000,000,000,000"
--

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

This looks sensible to me, might need an upgrading entry

@marc-mabe
Copy link
Contributor Author

marc-mabe commented Sep 30, 2023

According to Wikipedia numbers above 2^52 can't have fractions anymore:

Between 2^52=4,503,599,627,370,496 and 2^53=9,007,199,254,740,992 the representable numbers are exactly the integers. For the next range, from 2^53 to 2^54, everything is multiplied by 2, so the representable numbers are the even ones, etc. Conversely, for the previous range from 2^51 to 2^52, the spacing is 0.5, etc.

If that's true we could also improve precision for even more cases.

@marc-mabe
Copy link
Contributor Author

Updated to 2^52 now.

Also a quick benchmark:

<?php

$number = 4503599627370496.0;
$iterations = 100000;
$start = hrtime(true);
for ($i = 0; $i < $iterations; $i += 1) {
    $tmp = number_format($number);
}
$end = hrtime(true);
$spend = $end - $start;
printf('%s iterations: %Fs (%Fns per run)', $iterations, $spend / 1000000000, $spend / $iterations);

Before: 100000 iterations: 0.093053s (930.532260ns per run)
After :: 100000 iterations: 0.034346s (343.463090ns per run)

Comment on lines +1334 to +1335
(Z_DVAL_P(num) >= 4503599627370496.0 || Z_DVAL_P(num) <= -4503599627370496.0)
&& ZEND_DOUBLE_FITS_LONG(Z_DVAL_P(num))
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't ZEND_DOUBLE_FITS_LONG(Z_DVAL_P(num)) a little bit faster than the first condition? We have the difference of one execution of Z_DVAL_P.

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 don't think so.

  1. This is first checking the unusual case for numbers above 2^52
  2. ZEND_DOUBLE_FITS_LONG will cast ZEND_LONG_[MIN|MAX] and compare double values as well !((d) >= (double)ZEND_LONG_MAX || (d) < (double)ZEND_LONG_MIN)
  3. Z_DVAL_P(num) results in *num.value.dval

Copy link
Contributor

Choose a reason for hiding this comment

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

For number outside the order doesn't matter because two conditions must be met. But for number inside the range the second condition has only one Z_DVAL_P evaluation and first condition has two of them. The comparison of doubles occurs in both conditions.

However, I think that the difference is so small that you can ignore my comment. Probably even it may be optimized by compiler.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. I will leave it open for another month to see if there are any comments and if not, I think we can merge it.

@marc-mabe
Copy link
Contributor Author

@bukka Can this be merged now?

@bukka bukka closed this in b3f259d Dec 9, 2023
@marc-mabe marc-mabe deleted the number_format_float_int branch February 26, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants