JM: Fix JSNES fps regression around Aug 21

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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: --- → ?
(Assignee)

Comment 1

9 years ago
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.
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Updated

9 years ago
Summary: JM: Investigate JSNES fps regression around Aug 21 → JM: Fix JSNES fps regression around Aug 21
(Assignee)

Comment 4

9 years ago
Posted patch PatchSplinter Review
Didn't see the other "hack" section. We should fix that soon.
Assignee: general → dmandelin
Attachment #469318 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #469320 - Flags: review?(dvander)
(Assignee)

Comment 5

9 years ago
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+
(Assignee)

Comment 6

9 years ago
http://hg.mozilla.org/projects/jaegermonkey/rev/169f616ec5bc
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
blocking2.0: ? → final+
You need to log in before you can comment on or make changes to this bug.