Last Comment Bug 622348 - JavaScript Math.round incorrect for (2^53)-1
: JavaScript Math.round incorrect for (2^53)-1
Status: RESOLVED FIXED
[from-beta-tester]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-01 11:06 PST by Allen Wirfs-Brock
Modified: 2011-12-31 19:03 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.20 KB, patch)
2011-12-19 08:48 PST, Tom Schuster [:evilpie]
bzbarsky: review+
Details | Diff | Review
v2 (3.73 KB, patch)
2011-12-25 14:43 PST, Tom Schuster [:evilpie]
jwalden+bmo: review+
Details | Diff | Review

Description Allen Wirfs-Brock 2011-01-01 11:06:06 PST
Math.round produces incorrect result for largest representable integer value:

Math.round(9007199254740991)
   ==> returns: 9007199254740992, should be 9007199254740991
also:
Math.round(-9007199254740991)
   ==> returns: -19007199254740990, should be -9007199254740991

Note that 9007199254740991  is the largest value - 1 of the span of continuous integers that are precisely representable using IEEE doubles. It should round to itself. Correct results for values at this boundary are important for some integer numeric algorithms.

Tested with same result on Mac using FF4b9 and 3.6.13 and on Windows using 3.6.13.

Also, this does not appear to be a numeric literal conversion bug.  The problem also occurs if the value is computed (for example: 9007199254740990+1).

Finally, note that that the ECMAScript spec. provides the identity that Math.round(x) is the same as Math.floor(x+0.5).  While that is mathematically correct it isn't necessarily correct when operating at the precision limit of floating point arithmetic.  If the Math.round is implemented using that identify, it may be the source of the problem.

Finally, note that both Safari and Chrome produce the correct answer.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-01-01 11:58:33 PST
> Finally, note that that the ECMAScript spec. provides the identity that
> Math.round(x) is the same as Math.floor(x+0.5). 

That's how we seem to implement round(), yes.  Sounds like we could use a spec change here to add to the "except" bits in that note...
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-01-01 11:59:25 PST
And in particular:

  584 js_math_round_impl(jsdouble x)
  585 {
  586     return js_copysign(floor(x + 0.5), x);
Comment 3 Tom Schuster [:evilpie] 2011-01-01 12:13:23 PST
V8 checks if exponent is greater or equal 52, and in this case just returns the number.
Comment 4 Tom Schuster [:evilpie] 2011-12-19 08:48:15 PST
Created attachment 582841 [details] [diff] [review]
v1
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-21 12:40:48 PST
Comment on attachment 582841 [details] [diff] [review]
v1

r=me
Comment 7 Tom Schuster [:evilpie] 2011-12-25 06:35:23 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/6f31489f62d6 Arg this is a follow up for this bug, but I failed and somehow copied Bug 582841.
Comment 8 Tom Schuster [:evilpie] 2011-12-25 06:50:56 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/504f00e1124e Backed out, because of test failures and general stupidy surrounding this landing. Going to investigate, but in theory the inlined Math.round method doesn't match anymore.
Comment 9 Tom Schuster [:evilpie] 2011-12-25 14:43:57 PST
Created attachment 584291 [details] [diff] [review]
v2

So well, no idea how I didn't catch that :/ But the mistake is rather obvious when you think about it, I didn't really extract the exponent.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-25 14:51:19 PST
Comment on attachment 584291 [details] [diff] [review]
v2

This should probably get review from someone competent, since it seems my assumption about how much I know about this was wrong.
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 19:45:35 PST
Comment on attachment 584291 [details] [diff] [review]
v2

Review of attachment 584291 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/mathRoundBig.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/
> + */
> +
> +assertEq(Math.round(9007199254740991), 9007199254740991);

Add identical versions that test the string forms of these, for extra checking that the right number gets through?  Like so:

  assertEq(Math.round("9007199254740991"), 9007199254740991);

I'd prefer if this were in js/src/tests/ecma_5/Math/round-large-number.js, but this is already written, so you don't need to change the location.

::: js/src/jsmath.cpp
@@ +580,1 @@
>  JSBool

Add a /* ES5 15.8.2.15. */ comment

@@ +588,4 @@
>      }
> +
> +    double x;
> +    if (!ToNumber(cx, args[0], &x))

It might be worth checking for integers and returning early in that case, maybe?

@@ +594,5 @@
> +    jsdpun u;
> +    u.d = x;
> +    int exponent = ((u.s.hi & JSDOUBLE_HI32_EXPMASK) >> JSDOUBLE_HI32_EXPSHIFT) - JSDOUBLE_EXPBIAS;
> +
> +    /* Some numbers are so big that adding 0.5 would give the wrong number */

Move the |int exponent = ...| below the comment, adjacent to the |if| below?
Comment 13 Phil Ringnalda (:philor) 2011-12-31 19:03:42 PST
https://hg.mozilla.org/mozilla-central/rev/37348c68ba0b

Note You need to log in before you can comment on or make changes to this bug.