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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

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,
              sampleRate);
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.

Comment 1

[Jan de Mooij [:jandem]]

@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?

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

This compiles jsop_case as jsop_stricteq + jsop_ifeq. This passes jstests and jit-tests but I still want to add some jit-tests.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch v2

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.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

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?

Comment 6

[Jan de Mooij [:jandem]]

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

Comment 7

[Jan de Mooij [:jandem]]

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...

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Brendan Eich [:brendan]]

Want this in Firefox 4.x for x <= 1. :-|

/be

Comment 10

[Jan de Mooij [:jandem]]

Anybody willing to land this patch? I'd like to have this in FF5 (and brendan and shaver too..)

Comment 11

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/5cf1f751cb5b

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/5cf1f751cb5b