Closed Bug 590380 Opened 11 years ago Closed 11 years ago

JM: Fix JSNES fps regression around Aug 21


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- final+


(Reporter: dmandelin, Assigned: dmandelin)




(1 file, 1 obsolete file)

We used to get about 60fps. But now we get more like 22-25 fps on my machine. A slowdown of similar magnitude was also observed on Linux.

Using the exact browser build I demo'd at the summit, I now get the same 22 fps, when I showed about 57 fps then. So, it seems likely that the JSNES site itself changed. I emailed the author to ask about this.

I checked for JM aborts, and saw a couple in jQuery, on js_in_str and delelem.
blocking2.0: --- → ?
Shark shows that we are spending ~50% of total JS time in functions related to non-PIC'd property access. It seems like finding out what's issuing those calls (which line of JS, which stub call/interp case, etc.) will show the problem.
OK, I got it. The problem is here, in the file ui.js (in the git repo):

                writeFrame: function(buffer, prevBuffer) {
                    var imageData =;
                    var pixel, j;

                    for (i=0; i<256*240; i++) {

|i| is a global variable. This seems to be accidental. The op generated for |i++| is |gnameinc|. We have no fast path for that op. I'll email the author again. But we should probably spin off a bug about fixing this.

dvander, what's the way to go? Breaking down gnameinc into its parts would seem to do the trick, but ISTR there is some kind of nastiness with the prop cache with certain inc ops.
Attached patch WIP fast path for gnameinc ops (obsolete) — Splinter Review
This doesn't quite pass trace-tests: I get a stack depth assert on 3 tests:

    -m c:\sources\jaegermonkey\js\src\trace-test\tests\basic\testIncDec.js
    -m c:\sources\jaegermonkey\js\src\trace-test\tests\jaeger\bug549393-1.js
    -m c:\sources\jaegermonkey\js\src\trace-test\tests\jaeger\bug577705.js

I thought adding JOF_TMPSLOTS3 (like JSOP_NAMEINC) was the fix for that, but apparently not. It dumps on the first DUP2, which almost suggests the front end is not seeing that. Not sure though.
Summary: JM: Investigate JSNES fps regression around Aug 21 → JM: Fix JSNES fps regression around Aug 21
Attached patch PatchSplinter Review
Didn't see the other "hack" section. We should fix that soon.
Assignee: general → dmandelin
Attachment #469318 - Attachment is obsolete: true
Attachment #469320 - Flags: review?(dvander)
Btw the patch passes trace-tests and jstests. And it gets me to 57fps or so. Should be good with that plus stricteq.
Attachment #469320 - Flags: review?(dvander) → review+
Closed: 11 years ago
Resolution: --- → FIXED
blocking2.0: ? → final+
You need to log in before you can comment on or make changes to this bug.