Closed Bug 1482359 Opened 4 years ago Closed 4 years ago

Use StrictEq semantics for more cases in


(Core :: JavaScript Engine: JIT, enhancement)

Not set



Tracking Status
firefox63 --- fixed


(Reporter: anba, Assigned: anba)



(1 file, 2 obsolete files)

Bug 1433432 was a bit conservative when to convert into JSOP_STRICTEQ, for example MCompare::Compare_Bitwise is currently not enabled.
Attached patch bug1482359.patch (obsolete) — Splinter Review
Use JSOP_STRICTEQ for more cases in
- When both operands are typed as MIRType::Value, but are definitely not floating-point types, we can use JSOP_STRICTEQ.
- When one operand is typed as MIRType::Value, but also definitely not a floating-point type, we can use JSOP_STRICTEQ, too.

And when inlining as JSOP_STRICTEQ, directly call |jsop_compare| instead of just |compareTrySpecialized| to be able to use MCompare::Compare_Bitwise via |compareTryBitwise|.

Improves test cases like this one from 240ms to 28ms for me:
function f() {
    var xs = [1,2,3,null];
    var ys = [1,2,3,null];
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 10000000; ++i) {
        if ([i & 3], ys[i & 3]))
    print(dateNow() - t, q);
Attachment #8999149 - Flags: review?(jdemooij)
Comment on attachment 8999149 [details] [diff] [review]

Review of attachment 8999149 [details] [diff] [review]:

Nice speedup and I like reusing more/most of the jsop_compare code.

::: js/src/jit/MCallOptimize.cpp
@@ +2499,5 @@
> +        // they aren't floating-point types or values which might be floating-
> +        // point types. Otherwise we need to use MSameValue.
> +        strictEq = leftType != MIRType::Value
> +                   ? !IsFloatingPointType(leftType)
> +                   : !mightBeFloatingPointType(left) && !mightBeFloatingPointType(right);

Nit: I would add parentheses around the && because I never remember the evaluation order in such cases and I'm too lazy to look it up :)
Attachment #8999149 - Flags: review?(jdemooij) → review+
Attached patch bug1482359.patch (obsolete) — Splinter Review
Updated per review comments, carrying r+.
Attachment #8999149 - Attachment is obsolete: true
Attachment #8999602 - Flags: review+
Clearing "checkin-needed" because the patch needs to be rebased now that bug 1341261 has landed.
Keywords: checkin-needed
Attached patch bug1482359.patchSplinter Review
Rebased patch to apply cleanly on inbound.
Attachment #8999602 - Attachment is obsolete: true
Attachment #8999732 - Flags: review+
Pushed by
Use more JSOP_STRICTEQ optimizations for r=jandem
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.