JSOP_CASE aborts mjit compilation, rendering much of our *switch code useless




9 years ago
8 years ago


(Reporter: shaver, Assigned: jandem)


(Blocks 1 bug, {perf})

Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+, status2.0 wanted)


(Whiteboard: fixed-in-tracemonkey)


(1 attachment, 1 obsolete attachment)

20:36 <humph> [jaeger] Abort    opcode "case" not handled yet
              (file:///Users/dave/Dropbox/Kraddy/SceneBrowser/lib/dsp.js line

20:38 <humph> function IIRFilter(type, cutoff, resonance, sampleRate) {
20:38 <humph>   this.sampleRate = sampleRate;
20:38 <humph>   switch(type) {
20:38 <humph>     case DSP.LOWPASS:
20:38 <humph>     case DSP.LP12:
20:38 <humph>       this.func = new IIRFilter.LP12(cutoff, resonance,
20:38 <humph>       break;
20:38 <humph>   }

Surprised this didn't regress jsnes, but I bet they use literal ints for speed, meaning we get tableswitch.
blocking2.0: --- → .x
status2.0: --- → wanted
Blocks: 566369
Assignee: general → jandemooij
@dvander: I'm not sure how to compile JSOP_CASE in the methodjit. The interpreter pops the value when it takes the branch and otherwise it's kept on the stack, but afaik this is not how the methodjit works. Do you know if we can do this somehow?
Posted patch Patch (obsolete) — Splinter Review
This compiles jsop_case as jsop_stricteq + jsop_ifeq. This passes jstests and jit-tests but I still want to add some jit-tests.
Attachment #515179 - Flags: feedback?
Attachment #515179 - Flags: feedback? → feedback?(dvander)
Attachment #515179 - Flags: feedback?(dvander) → feedback+
Posted patch Patch v2Splinter Review
Added tests. In the interpreter JSOP_DEFAULT is a pop() + JSOP_GOTO. We don't need the pop before the jump in JM thanks to the bytecode analyzer, right? Just double-checking my understanding.
Attachment #515179 - Attachment is obsolete: true
Attachment #515198 - Flags: review?(dvander)
Attachment #515198 - Flags: review?(dvander) → review+
Duplicate of this bug: 618050
Risks: This patch could perturb some trace tuning that expects methods with slow |switch| statements not to method JIT. It could introduce correctness issues with such cases, or expose existing ones by compiling more code than we used to. Since (1) this is a straight-up primitive op, no dynamic generation, and (2) we don't method JIT until methods are hot, these are less likely.

Benefits: Right now non-trivial |switch| cases get interpreted, so if benchmark XYZ (like bug 636096) happens to use it, we could fall apart.

Jan, how much does this affect bug 636096?
(In reply to comment #5)
> Jan, how much does this affect bug 636096?

With -m -j -p it becomes 7 fps faster (30 fps to 37 fps)
I instrumented a browser build to see how common JSOP_CASE is. Gmail seems to use it a lot, opening my mail and clicking on a label hits JSOP_CASE about 65.000 times...
With the patch applied jsop_case is interpreted about 2,000-3,000 times. Without the patch this can easily go up to 70,000-80,000.
Want this in Firefox 4.x for x <= 1. :-|

Keywords: checkin-needed, perf
Anybody willing to land this patch? I'd like to have this in FF5 (and brendan and shaver too..)
Keywords: checkin-needed
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: fixed-in-tracemonkey
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.