1862273 - Consider having fatter typeof test bytecodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Tooru Fujisawa [:arai]]

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);

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Bug 1862273 - Part 1: Add JSOp::TypeofEq for typeof val == "type". r?jandem!

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Bug 1862273 - Part 2: Support not-equal case in JSOp::TypeofEq. r?jandem!

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1862273 - Part 3: Add testcases. r?jandem!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Cristian Tuns]

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'

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/7f0c1c5f6a1c
https://hg.mozilla.org/mozilla-central/rev/2618f655dc70
https://hg.mozilla.org/mozilla-central/rev/29f2fb6a6189

Comment 10

[Andrej (:aglavic)]

(In reply to Pulsebot from comment #8)

Pushed by [e-mail]:
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.