1459611 - Use NumberEqualsInt32 instead of NumberIsInt32 were applicable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

NumberIsInt32 tests for negative zero <https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/mfbt/FloatingPoint.h#425-427> and then proceeds with calling NumberEqualsInt32 if the input is not negative zero. That means two things:

1. Callers to NumberIsInt32 always pay the (small) cost of testing for negative zero.
2. If the input is negative zero, NumberIsInt32 returns false which means most of the time a slow path will be taken for this caller.

But it turns out we're currently calling NumberIsInt32 a few times where NumberEqualsInt32 is perfectly valid:


math_round_impl(double) and math_roundf_impl(float):
- If the input is negative zero, returning negative zero is actually correct.
- https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/js/src/jsmath.cpp#847-849
- https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/js/src/jsmath.cpp#864-866


BytecodeEmitter::emitSwitch(ParseNode*):
- switch-statements use StrictEquals semantics, so it's okay to handle negative zero as positive zero.
- This also matches the corresponding semantics for JSOP_TABLESWITCH in the interpreter.
- https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/js/src/frontend/BytecodeEmitter.cpp#4670-4673
- https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/js/src/vm/Interpreter.cpp#3366-3368


Various double to string/index conversion functions (FracNumberToCString, NumberToCString, NumberToStringWithBase, NumberToAtom, ValueFitsInInt32, IsDefinitelyIndex)
- ECMAScript doesn't preserve the sign in the string representation for negative zero, so we can handle it like positive zero.
- The same goes for indices.

Comment 1

[André Bargull [:anba]]

Attachment: bug1459611.patch

Replaces mozilla::NumberIsInt32 with mozilla::NumberEqualsInt32 where applicable per comment #0.

js/src/jsnum.cpp
- Replaces a no longer relevant if-statement with an assertion.

js/src/jsnum.h
- ValueFitsInInt32() was only used for JS::Value to jsid conversions.
- Renamed the function to ValueToIntJSID() and moved it into its sole remaining caller in js/src/vm/JSAtom-inl.h.

js/src/builtin/JSON.cpp
- Restructured the code so it no longer follows exactly every steps from the spec.
- This allows to remove the call to ValueFitsInInt32() and it makes the code more readable.
- I couldn't observe any performance regressions when µ-benchmarking it. Tested with:
  
    var replacer = Array(10).fill(0);
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        JSON.stringify(undefined, replacer);
    }
    print(dateNow() - t);

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8973668 [details] [diff] [review]
bug1459611.patch

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

::: js/src/builtin/JSON.cpp
@@ +674,2 @@
>                  RootedId id(cx);
> +                if (!ValueToId<CanGC>(cx, item, &id))

Nice, I agree this is more readable.

::: js/src/vm/JSAtom-inl.h
@@ +35,5 @@
>  inline jsid
>  AtomToId(PropertyName* name) = delete;
>  
> +MOZ_ALWAYS_INLINE bool
> +ValueToIntJSID(const Value& v, jsid* id)

Nit: ValueToIntId would match AtomToId, ValueToId, etc.

@@ +38,5 @@
> +MOZ_ALWAYS_INLINE bool
> +ValueToIntJSID(const Value& v, jsid* id)
> +{
> +    int32_t i;
> +    if (v.isInt32()) {

Nit: no {}

Comment 3

[André Bargull [:anba]]

Attachment: bug1459611.patch

Updated per review comments, carrying r+.

Comment 4

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2860bbcf47df304252da336a384ce9a4bbc3db02

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4fdee19123
Use NumberEqualsInt32 when negative and positive zero are treated the same. r=jandem

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ce4fdee19123