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

Improve number to string conversions #400

Merged
merged 1 commit into from
May 26, 2024

Conversation

chqrlie
Copy link
Collaborator

@chqrlie chqrlie commented May 8, 2024

integer conversions:

  • improve u32toa_radix and u64toa_radix, add i32toa_radix
  • use i32toa_radix for small ints in js_number_toString

floating point conversions (js_dtoa):

  • complete rewrite with fewer calls to snprintf
  • remove JS_DTOA_FORMAT, define 4 possible modes for js_dtoa
  • remove the radix argument in js_dtoa
  • merge js_dtoa1 into js_dtoa
  • add js_dtoa_infinite for non finite values
  • simplify sign handling
  • handle locale specific decimal point transparently

helper function js_fcvt:

  • simplify js_fcvt, remove js_fcvt1, reduce overhead
  • round up manually instead of using fesetround(FE_UPWARD).

helper function js_ecvt:

  • document js_ecvt and js_ecvt1 behavior
  • avoid redundant js_ecvt1 calls in js_ecvt
  • fixed buffer contents, no buffer copies
  • simplify decimal point handling
  • round up manually instead of using fesetround(FE_UPWARD).

miscellaneous:

  • this fixes a v8.sh bug on macOS: 0.5.toFixed(0) used to produce 0 instead of 1
  • add regression tests, update test_conv unit tests
  • add benchmarks for toFixed, toPrecision and toExponential number methods
  • benchmarks show all conversions are now 40 to 45% faster (M2)

@saghul
Copy link
Contributor

saghul commented May 8, 2024

Any chance this also fixes the rounding issues on Windows?

@chqrlie
Copy link
Collaborator Author

chqrlie commented May 8, 2024

Any chance this also fixes the rounding issues on Windows?

Windows was supposed to behave as RNDNA, round to nearest away from 0 by default, so the work around to implement round away from zero was not compiled for Windows. But this assumption is incorrect according to this page: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/printf-printf-l-wprintf-wprintf-l?view=msvc-170
The page for fesetround() also documents some quirks regarding the rounding modes that are mind boggling: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fegetround-fesetround2?view=msvc-170

If you have access to a Windows machine, try defining CONFIG_PRINTF_RNDN unconditionally and check the offending tests.

After discussing this with Fabrice, it appears CONFIG_PRINTF_RNDN should be defined for Windows targets too. RNDNA is no longer the default and has not been for 20 years.

Can you confirm if fesetround can be used for __wasi__ targets? If so, we could get rid of CONFIG_PRINTF_RNDN completely, or define fesetround() as a macro for these targets.

__wasi__ does not define FE_DOWNWARD so I changed the test and removed CONFIG_PRINTF_RNDN completely.

The conversion tests now all pass on all platforms.

The extreme Date conversion tests still fail but probably for a separate reason. I am not sure the code can be written in a fully defined way for such corner cases.

@chqrlie chqrlie force-pushed the improve-js-dtoa branch 5 times, most recently from 9b458a8 to cce7876 Compare May 8, 2024 13:43
cutils.c Outdated Show resolved Hide resolved
cutils.c Show resolved Hide resolved
tests/test_builtin.js Show resolved Hide resolved
quickjs.c Show resolved Hide resolved
quickjs.c Show resolved Hide resolved
quickjs.c Show resolved Hide resolved
integer conversions:
- improve `u32toa_radix` and `u64toa_radix`, add `i32toa_radix`
- use `i32toa_radix` for small ints in `js_number_toString`

floating point conversions (`js_dtoa`):
- complete rewrite with fewer calls to `snprintf`
- remove `JS_DTOA_FORMAT`, define 4 possible modes for `js_dtoa`
- remove the radix argument in `js_dtoa`
- merge `js_dtoa1` into `js_dtoa`
- add `js_dtoa_infinite` for non finite values
- simplify sign handling
- handle locale specific decimal point transparently

helper function `js_fcvt`:
- simplify `js_fcvt`, remove `js_fcvt1`, reduce overhead
- round up manually instead of using `fesetround(FE_UPWARD)`.

helper function `js_ecvt`:
- document `js_ecvt` and `js_ecvt1` behavior
- avoid redundant `js_ecvt1` calls in `js_ecvt`
- fixed buffer contents, no buffer copies
- simplify decimal point handling
- round up manually instead of using `fesetround(FE_UPWARD)`.

miscellaneous:
- remove `CONFIG_PRINTF_RNDN`. This fixes some of the conversion errors
  on Windows. Updated the tests accordingly
- this fixes a v8.sh bug on macOS: `0.5.toFixed(0)` used to produce `0` instead of `1`
- add regression tests, update test_conv unit tests
- add benchmarks for `toFixed`, `toPrecision` and `toExponential` number methods
- benchmarks show all conversions are now 40 to 45% faster (M2)
@chqrlie chqrlie merged commit 9e67b47 into quickjs-ng:master May 26, 2024
47 checks passed
@chqrlie chqrlie deleted the improve-js-dtoa branch May 26, 2024 06:06
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.

None yet

2 participants