Improve Math.round(). Fix the bug in r4146. Further improve performance by ch...

Improve Math.round(). Fix the bug in r4146. Further improve performance by checking the exponent instead of comparing doubles. Add several tests for numbers near the limits of SMI and several tests from WebKit.


Committed: http://code.google.com/p/v8/source/detail?r=4180

[fschneider, 2010-03-17 23:29:19]

LGTM.

http://codereview.chromium.org/1008004/diff/5001/6002
File src/runtime.cc (right):

http://codereview.chromium.org/1008004/diff/5001/6002#newcode5330
src/runtime.cc:5330: // Must be SMI. Return the argument unchanged for all the
other types
SMI -> smi

http://codereview.chromium.org/1008004/diff/5001/6002#newcode5335
src/runtime.cc:5335: CONVERT_DOUBLE_CHECKED(x, args[0]);
CONVERT_DOUBLE_CHECKED checks for IsNumber too. But at this point we know that
args[0] is a HeapNumber. Could you avoid the redundant check?

http://codereview.chromium.org/1008004/diff/5001/6002#newcode5340
src/runtime.cc:5340: if (!negative && exponent <= kSmiValueSize - 3) {
Just for my understanding: Is it kSmiValueSize - 3 because smis have one less
positive integer than negative integers (-2^30 to 2^30 - 1)?

http://codereview.chromium.org/1008004/diff/5001/6002#newcode5344
src/runtime.cc:5344: // if the magnitude is big enough, there's no place for
fraction part. If we
if -> If

[Rico, 2010-03-18 05:41:10]

Drive by comment:

Maybe add a note saying that this method assumes that the callsite does a
ToNumber on the input. If this has not been done by the callsite this will not
follow spec (i.e., if we get a string as input this will also output that string
- but it should output NaN). I know that we are always guaranteed that this
happens from Math.round but this method could potentially be used from somewhere
else (e.g., date functions)

[Erik Corry, 2010-03-18 08:21:22]

I think Rico's objection could be solved with a rename eg to
Runtime_RoundNumber() so that it is clear that the input must be a number.

[Oleg Eterevsky, 2010-03-18 13:24:23]

http://codereview.chromium.org/1008004/diff/5001/6002
File src/runtime.cc (right):

http://codereview.chromium.org/1008004/diff/5001/6002#newcode5330
src/runtime.cc:5330: // Must be SMI. Return the argument unchanged for all the
other types
On 2010/03/17 23:29:19, fschneider wrote:
> SMI -> smi

Done.

http://codereview.chromium.org/1008004/diff/5001/6002#newcode5335
src/runtime.cc:5335: CONVERT_DOUBLE_CHECKED(x, args[0]);
On 2010/03/17 23:29:19, fschneider wrote:
> CONVERT_DOUBLE_CHECKED checks for IsNumber too. But at this point we know that
> args[0] is a HeapNumber. Could you avoid the redundant check?

Done.

http://codereview.chromium.org/1008004/diff/5001/6002#newcode5340
src/runtime.cc:5340: if (!negative && exponent <= kSmiValueSize - 3) {
On 2010/03/17 23:29:19, fschneider wrote:
> Just for my understanding: Is it kSmiValueSize - 3 because smis have one less
> positive integer than negative integers (-2^30 to 2^30 - 1)?

To be precise, it's because (2^30 - 0.1) has exponent 29 and  should be rounded
to 2^30, which is not smi.

P.S. I've added a comment and a few tests for values around 2^29. just in case.

http://codereview.chromium.org/1008004/diff/5001/6002#newcode5344
src/runtime.cc:5344: // if the magnitude is big enough, there's no place for
fraction part. If we
On 2010/03/17 23:29:19, fschneider wrote:
> if -> If

Done.

[Oleg Eterevsky, 2010-03-18 13:32:49]

On 2010/03/18 08:21:22, Erik Corry wrote:
> I think Rico's objection could be solved with a rename eg to
> Runtime_RoundNumber() so that it is clear that the input must be a number.

Done.