Closed Bug 1862273 Opened 7 months ago Closed 19 days ago

Consider having fatter typeof test bytecodes

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
127 Branch
Tracking Status
firefox127 --- fixed

People

(Reporter: jrmuizel, Assigned: arai)

Details

Attachments

(3 files)

This JS code from Angular:

function isTypeProvider(value) {  return "function" == typeof value;}

SM:

compiles to the following 
00000:  18  String "function"           # "function"
00005:  18  GetArg 0                    # "function" value
00008:  18  Typeof                      # "function" (typeof value)
00009:  18  Eq                          # ("function" == (typeof value))
00010:  18  Return                      # 
00011:  18  RetRval                     # !!! UNREACHABLE !!!

V8:

         0x3e110019ab9c @    0 : 0b 03             Ldar a0
         0x3e110019ab9e @    2 : 20 06             TestTypeOf #6
         0x3e110019aba0 @    4 : aa                Return

JSC:

bb#1
Predecessors: [ ]
[   0] enter
[   1] typeof_is_function dst:loc5, operand:arg1
[   4] ret                value:loc5
Successors: [ ]

Both, V8 and JSC uses a fatter opcode (TestTypeOf, typeof_is_function)

I don't have any evidence that doing this is super important but it seems like on the right code it could make some difference in early tiers.

This is one of these cases where our bytecode is a bit too unoptimized and skewed towards optimizing tiers where this gets optimized away.

Another common one is x === null where just having JSOp::IsNull would be nice. We also have an unnecessary to-boolean conversion in the interpreters for if (x === y) even though the bytecode emitter should know that the equality expression is always pushing a boolean.

It would be interesting to do some analysis searching for common bytecode sequences that we could replace with a single op.

Severity: -- → S3
Priority: -- → P3

Some more context about typeof in JIT.

with the current opcode and MIR, there's a heuristics about folding typeof, and it's done based on the number of use.
if we add dedicate opcode for typeof val == "type", the similar heuristics should be applied (if we use different MIR)

https://searchfox.org/mozilla-central/rev/98a54cbdc5964e778af0e6b325f9e7831f00c05b/js/src/jit/MIR.cpp#4660-4680

// If there's only a single use, assume this |typeof| is used in a simple
// comparison context.
//
// if (typeof thing === "number") { ... }
//
// It'll be compiled into something similar to:
//
// if (IsNumber(thing)) { ... }
//
// This heuristic can go wrong when repeated |typeof| are used in consecutive
// if-statements.
//
// if (typeof thing === "number") { ... }
// else if (typeof thing === "string") { ... }
// ... repeated for all possible types
//
// In that case it'd more efficient to emit MTypeOf compared to MTypeOfIs. We
// don't yet handle that case, because it'd require a separate optimization
// pass to correctly detect it.
if (typeOfName->hasOneUse()) {
  return MTypeOfIs::New(alloc, input, jsop(), type);
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #9396498 - Attachment description: Bug 1862273 - Add JSOp::TypeofEq for typeof val == "type". r?jandem! → Bug 1862273 - Part 1: Add JSOp::TypeofEq for typeof val == "type". r?jandem!
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/8e93e3353435
Part 1: Add JSOp::TypeofEq for typeof val == "type". r=jandem
https://hg.mozilla.org/integration/autoland/rev/1da36a021156
Part 2: Support not-equal case in JSOp::TypeofEq. r=jandem
https://hg.mozilla.org/integration/autoland/rev/2f9259bc56ba
Part 3: Add testcases. r=jandem

Backed out for causing bustages in PortableBaselineInterpret.cpp:

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: /builds/worker/checkouts/gecko/js/src/vm/PortableBaselineInterpret.cpp:3438:35: error: calling a private constructor of class 'js::TypeofEqOperand'
Flags: needinfo?(arai.unmht)
Flags: needinfo?(arai.unmht)
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/7f0c1c5f6a1c
Part 1: Add JSOp::TypeofEq for typeof val == "type". r=jandem
https://hg.mozilla.org/integration/autoland/rev/2618f655dc70
Part 2: Support not-equal case in JSOp::TypeofEq. r=jandem
https://hg.mozilla.org/integration/autoland/rev/29f2fb6a6189
Part 3: Add testcases. r=jandem
Status: ASSIGNED → RESOLVED
Closed: 19 days ago
Resolution: --- → FIXED
Target Milestone: --- → 127 Branch

(In reply to Pulsebot from comment #8)

Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/7f0c1c5f6a1c
Part 1: Add JSOp::TypeofEq for typeof val == "type". r=jandem
https://hg.mozilla.org/integration/autoland/rev/2618f655dc70
Part 2: Support not-equal case in JSOp::TypeofEq. r=jandem
https://hg.mozilla.org/integration/autoland/rev/29f2fb6a6189
Part 3: Add testcases. r=jandem

Perfherder has detected a talos performance change from push 29f2fb6a6189c4d600b770246fb072374f095e63.

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
3% pdfpaint issue12841_reduced.pdf macosx1015-64-shippable-qr e10s fission stylo webrender-sw 2,196.79 -> 2,128.60
3% pdfpaint issue12841_reduced.pdf macosx1015-64-shippable-qr e10s fission stylo webrender-sw 2,193.61 -> 2,135.42

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 123

For more information on performance sheriffing please see our FAQ.

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

Attachment

General

Created:
Updated:
Size: