1286114 - Do not allocate NaN and Infinity strings in num_toFixed_impl

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Rajinder Yadav]

We allocate the strings here, however we should use pre-allocated atoms.

https://dxr.mozilla.org/mozilla-central/rev/214884d507ee369c1cf14edb26527c4f9a97bf48/js/src/dtoa.c#2662-2664

Comment 1

[Rajinder Yadav]

Attachment: bug1286114.patch

Please review.

Comment 2

[Tom S. (please needinfo tschuster)]

We should just add NegativeInfinity (-0) to CommonPropertyNames instead of using StringBuilder.

Comment 3

[Rajinder Yadav]

Tom agreed, I should have suggested this :P

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8770044 [details] [diff] [review]
bug1286114.patch

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

Thanks :)
Please address the following comments, and ask review again.

::: js/src/jsnum.cpp
@@ +916,5 @@
>  {
>      MOZ_ASSERT(IsNumber(args.thisv()));
>  
> +    // If 'this' (callee) value is -Infinity, Infinity or NaN
> +    // return String representation.

This comment is redundant. please remove.

@@ +917,5 @@
>      MOZ_ASSERT(IsNumber(args.thisv()));
>  
> +    // If 'this' (callee) value is -Infinity, Infinity or NaN
> +    // return String representation.
> +    double d = Extract(args.thisv());

Just like toExponential, can you add spec step comments?

@@ +919,5 @@
> +    // If 'this' (callee) value is -Infinity, Infinity or NaN
> +    // return String representation.
> +    double d = Extract(args.thisv());
> +
> +    if (mozilla::IsNaN(d)) {

So you'll see that this is step 4. and we should have step 2 and 3 before this.
Both are already done in this function, so move those block before this line.

Please add testcase for the execution order between them (see the comment below)

@@ +922,5 @@
> +
> +    if (mozilla::IsNaN(d)) {
> +        args.rval().setString(cx->names().NaN);
> +        return true;
> +    } else if (mozilla::IsInfinite(d)) {

Do not use "else" after return.

@@ +927,5 @@
> +        if(d > 0) {
> +            args.rval().setString(cx->names().Infinity);
> +            return true;
> +        }
> +        StringBuffer sb(cx);

As evilpie says, please change negative infinity case to return cx->names().NegativeInfinity, and add NegativeInfinity to js/src/vm/CommonPropertyNames.h.

Also, apply the same change to toExponential and toPrecision.


Actually, I was about to ask merge those 3 code for NaN/Infinity handling (as they're exactly same), but if we can return atom directly, the code should be simple enough and we won't have to merge them.

::: js/src/tests/ecma_6/Number/ToNumber.js
@@ +18,5 @@
>  assertEq(+"0o66", 54);
>  
> +assertEq(Number.prototype.toFixed.call(-Infinity), "-Infinity");
> +assertEq(Number.prototype.toFixed.call(Infinity), "Infinity");
> +assertEq(Number.prototype.toFixed.call(NaN), "NaN");

This file is a testcase for Number constructor.
Can you create a new dedicated file for toFixed?

Also, please check passing out-of-range fractionDigits throws RangeError even with NaN and Infinity (this is different behavior than toExponential and toPrecision...), with assertThrowsInstanceOf.

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Tom Schuster [:evilpie] from comment #2)
> We should just add NegativeInfinity (-0) to CommonPropertyNames instead of
> using StringBuilder.

Thank you for pointing that out :)

Comment 6

[Rajinder Yadav]

Attachment: bug1286114.patch

Feedback addressed, please review.

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8770072 [details] [diff] [review]
bug1286114.patch

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

Thank you again :)
Almost there, please address the following comments, mostly cosmetic changes.

::: js/src/jsnum.cpp
@@ +909,5 @@
>  
>  /*
>   * In the following three implementations, we allow a larger range of precision
>   * than ECMA requires; this is permitted by ECMA-262.
>   */

Please add the following comment here, so that "step N" makes sense in function body.

// ES 2017 draft rev f8a9be8ea4bd97237d176907a1e3080dce20c68f 20.1.3.3.

@@ +936,5 @@
> +        args.rval().setString(cx->names().NaN);
> +        return true;
> +    }
> +
> +    // Steps 5-6.

in toFixed, Infinity is handled in Steps 5-7, 9, and no special case is defined in the spec.

so it would be better changing the comment to

  // Steps 5-7, 9 (optimized path for Infinity).

or something like that.

(I'm wondering why there are so much differences in the spec...)

@@ +947,5 @@
> +        args.rval().setString(cx->names().NegativeInfinity);
> +        return true;
> +    }
> +
> +    // Steps 7-9.

Also, steps 5-6 are also handled here, for normal number, so please change it to "// Steps 5-9."

I overlooked this for toExponential and toPrecision, sorry.  can you fix the step numbers there too?

@@ +996,4 @@
>          return true;
>      }
>  
>      // Steps 8-15.

So, here, "// Steps 5-6, 8-15."

@@ +1054,4 @@
>          return true;
>      }
>  
>      // Steps 8-14.

and "// Steps 5-6, 8-14." here.

Comment 8

[Rajinder Yadav]

Attachment: bug1286114.patch

Feedback applied, please review.

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8770087 [details] [diff] [review]
bug1286114.patch

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

Thank you!

Comment 10

[Tooru Fujisawa [:arai]]

try is running (with other patches)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad83665a4e0e

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28f796a18c0c74ae04b0b7ec4a49ea5194632411
Bug 1286114 - Do not allocate NaN and Infinity strings in num_toFixed_impl. r=arai

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28f796a18c0c