590380 - JM: Fix JSNES fps regression around Aug 21

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

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.

Comment 1

[David Mandelin [:dmandelin]]

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.

Comment 2

[David Mandelin [:dmandelin]]

OK, I got it. The problem is here, in the file ui.js (in the git repo):

                writeFrame: function(buffer, prevBuffer) {
                    var imageData = this.canvasImageData.data;
                    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.

Comment 3

[David Mandelin [:dmandelin]]

Attachment: WIP fast path for gnameinc ops

This doesn't quite pass trace-tests: I get a stack depth assert on 3 tests:

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

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Patch

Didn't see the other "hack" section. We should fix that soon.

Comment 5

[David Mandelin [:dmandelin]]

Btw the patch passes trace-tests and jstests. And it gets me to 57fps or so. Should be good with that plus stricteq.

Comment 6

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/projects/jaegermonkey/rev/169f616ec5bc