944846 - toExponential rounding error

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[swediomurital]

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.48 Safari/537.36

Steps to reproduce:

(12345).toExponential(3);


Actual results:

Result is 1.234e+4, which differ from ECMA specification and other engines (at least Opera and V8).


Expected results:

1.235e+4

http://ecma-international.org/ecma-262/5.1/#sec-15.7.4.6
Section 15.7.4.6 Number.prototype.toExponential (fractionDigits)

Pt. 9.a: "If there are two such sets of e and n, pick the e and n for which n × 10e–f is larger."

1234 × 10^(4 - 3) - 12345 = -5
1235 × 10^(4 - 3) - 12345 =  5

a = 1
b = 235
c = "+"
d = 4

result a + "." + b + "e" + c + d = 1.235e+4

Comment 1

[Paul Silaghi, QA [:pauly]]

Confirmed 28.0a1 (2013-12-09), Win 7 x64

Comment 2

[Tooru Fujisawa [:arai]]

maybe we could change the behavior by changing the following lines?

https://dxr.mozilla.org/mozilla-central/rev/09221c72fcb03a7c2b4ab27c75304eae3247e3d1/js/src/dtoa.c#2961
> 				dval(d) += dval(d);
> 				if (dval(d) > ds || (dval(d) == ds && L & 1)) {
>  bump_up:

at 1st line, dval(d) is 5000 (`the last digit of 12345` * 10 * 10 * 10), and ds is 10000.
so we don't "bump_up" because `5000+5000 > 10000` is false.

however, iiuc, the following line in the spec just says that we should round up 5.

> If there are two such sets of e and n, pick the e and n for which n × 10e–f is larger.

so, changing the condition to `dval(d) >= ds` will round up the case.

I'm still not sure how much it affects other cases/usage, and also worrying because there's one more copy in-tree tho...

https://dxr.mozilla.org/mozilla-central/rev/09221c72fcb03a7c2b4ab27c75304eae3247e3d1/nsprpub/pr/src/misc/prdtoa.c#3099
> 				dval(d) += dval(d);
> 				if (dval(d) > ds || (dval(d) == ds && L & 1)) {
>  bump_up:

Comment 3

[Tooru Fujisawa [:arai]]

at least it passes all existing tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b247807c029b

Comment 4

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 5

[Reese Williams]

Hey folks, just following up here since it's been a while since there's been any activity on this. It looks like the fix above in dtoa.c fixes the tests for this, and as far as I can tell it doesn't cause any other regressions, although I'm not sure how best to handle the in-tree copy of this. Any thoughts? I have a patch with the previously mentioned changes, but I didn't change the copy yet since I wasn't sure if those files were intentionally duplicated.

Comment 6

[Reese Williams]

Attachment: Bug 944846 - Fix rounding issue in `Number.prototype.toExponential`

Comment 7

[Jeff Walden [:Waldo]]

Searchfox indicates dtoa is only called by Number.prototype.to{Fixed,Exponential,Precision} and by our internal JSONPrinter used only for CacheIR spew. So, we need only care about this change with respect to three algorithms.

I'm attempting to review this now. It is going to be awhile. dtoa.c is a wretched hive of scum and villainy that either needs to be drastically cleaned up and commented, rewritten in Rust (with exceptionally few unsafe blocks), or nuked from orbit and the ground salted and replaced with some modern equivalent that is maintained and understandable (even at mild expense of performance, seeing as throughput of float->string operations ought not be a real-world limiting factor and safety/correctness are more important in these circumstances).

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Unfortunately bug 450392 and bug 384244 imply our copy is heavily modified so we can't just update from upstream, which has in fact been fixing things for years: http://www.netlib.org/fp/changes

The relevant code upstream (http://www.netlib.org/fp/dtoa.c) reads:

				dval(&u) += dval(&u);
#ifdef ROUND_BIASED
				if (dval(&u) >= ds)
#else
				if (dval(&u) > ds || (dval(&u) == ds && L & 1))
#endif

Which suggests that the patch here could lose the second half of the condition, but perhaps also that there's a bunch more ROUND_BIASED fixes that would need to be applied.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

It looks like we have a more modern dtoa in https://searchfox.org/mozilla-central/source/mfbt/double-conversion/double-conversion

Comment 10

[Gian-Carlo Pascutto [:gcp]]

dtoa.c is a wretched hive of scum and villainy that either needs to be drastically cleaned up and commented, rewritten in Rust

It appears you wishes have been granted given that we have third_party/rust/dtoa.

That said, js/src/util/DoubleToString.cpp seems to require knowledge of dtoa internals, so at this point this thing is too scary for me to touch.

Comment 11

[Lars T Hansen [:lth]]

Also see https://news.ycombinator.com/item?id=26035495 for a survey of recent work.

dtoa.c is to JS as water is to the Earth: you can (probably) make it work with other substances, but try it at your peril.

Comment 12

[Jan de Mooij [:jandem]]

I think we should move this code to DoubleToStringConverter::ToExponential. That's what other JS engines are using and it fixes the bug for me (and passes all shell tests).

Comment 13

[Jan de Mooij [:jandem]]

So looking at what JSC does:

• For toExponential, they use DoubleToStringConverter::ToExponential.
• For toPrecision, they use numberToStringFixedPrecision which calls DoubleToStringConverter::ToPrecision.
• For toFixed, they use numberToStringFixedWidth which ends up calling DoubleToStringConverter::ToFixed.

The annoying part is that ToFixed supports precision (requested_digits) up to 60, whereas JS requires up to 100. JSC made changes to the double-conversion code to support this. We however made other changes to that code last week.

So long story short: I think we can use double-conversion, but for toFixed with large precision values we either need to fall back to dtoa or change the code.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 944846 part 1 - Use DoubleToStringConverter::ToExponential for toExponential. r?anba!

This avoids a rounding issue in the old dtoa code.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Bug 944846 part 2 - Use DoubleToStringConverter::ToPrecision for toPrecision. r?anba!

Depends on D104519

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Bug 944846 part 3 - Increase DoubleToStringConverter::kMaxFixedDigitsAfterPoint from 60 to 100. r?glandium!

We need to support values up to 100 for Number.prototype.toFixed.

Depends on D104520

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Bug 944846 part 4 - Use DoubleToStringConverter::ToFixed for toFixed. r?anba!

Depends on D104521

Comment 18

[Jan de Mooij [:jandem]]

The patches move {toExponential, toPrecision, toFixed} from js_dtostr to the double-conversion code in MFBT. After that the only caller of js_dtostr is JSONPrinter::floatProperty and that should be easy to fix in a follow-up.

I did some micro-benchmarking and the double-conversion code is a bit faster than the dtoa implementation.

Comment 19

[Mike Hommey [:glandium]]

Cool. I was planning to look into moving js to double-conversion soon, looks like I won't have to :)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d06ce5533326
part 1 - Use DoubleToStringConverter::ToExponential for toExponential. r=anba
https://hg.mozilla.org/integration/autoland/rev/2560c2763df0
part 2 - Use DoubleToStringConverter::ToPrecision for toPrecision. r=anba
https://hg.mozilla.org/integration/autoland/rev/613260ab04e0
part 3 - Increase DoubleToStringConverter::kMaxFixedDigitsAfterPoint from 60 to 100. r=glandium
https://hg.mozilla.org/integration/autoland/rev/86cdeb4f7d76
part 4 - Use DoubleToStringConverter::ToFixed for toFixed. r=anba

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d06ce5533326
https://hg.mozilla.org/mozilla-central/rev/2560c2763df0
https://hg.mozilla.org/mozilla-central/rev/613260ab04e0
https://hg.mozilla.org/mozilla-central/rev/86cdeb4f7d76