Closed Bug 604031 Opened 10 years ago Closed 10 years ago

JM: fast path for JSOP_CALLELEM


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bhackett1024, Assigned: dvander)



(Whiteboard: fixed-in-tracemonkey)


(2 files)

The shell testcase for the gameboy emulator in bug 514765 uses CALLELEM in its dispatch loop, which is currently stubbed.  I think this is responsible for the slowdown remaining vs. v8 after accounting for bug 604029.
Do you know here if the array if dense or not? It definitely sounds like a use case to keep in mind for bug 602641.
It is dense.  I don't see where CALLELEM is more complex than GETELEM; do a getelem, then push the lval.  Will write a patch today.
Okay, so no IC needed, but just keep in mind that "push the lval" comes with the catch that you can't just copy it, since the slow path could overwrite it.

By "use case", bug 602641 and 603779 are refactoring array use in JM. If this patch can wait, it'll be a lot cleaner.
Yeah, this would be done by passing a flag to jsop_getelem, as the stub call would be pushing both values.  Also, date-format-xparb looks like it does 8000 callelems ("this[func]()", where func is one of two different strings) so using the getelem PIC on those would be nice.  No hurry here.
Attached patch patchSplinter Review
Stealing :) Sleep-deprived and this seemed like a good test of whether the new ICs were flexible (seems: they are).

before tm: 33 ms. - 212.9 MHZ
after  tm: 22 ms. - 319.3 MHZ
       v8: 25 ms. - 281.0 MHZ
Assignee: general → dvander
Attachment #486052 - Flags: review?(dmandelin)
Attachment #486052 - Flags: review?(dmandelin) → review+
(This was about a 1-2ms win on date-format-xparb, as Brian hinted.)
Depends on: 608810
Blamed for bug 608810, a b7 blocker. This needs to come out of m-c and probably tm, where a reduced test goes in to guard that gate.

No citation of m-c landing via link here, with resolved fixed change to match -- so nothing to undo now that this patch has been backed out of m-c :-P.

Bug bookkeeping matters, be safe out there.

Duplicate of this bug: 609082
Depends on: 609200
Did this get backed out of TM? If not this (and the regressions) will show up on m-c again when we merge...
Attachment #488601 - Flags: review?(dmandelin) → review+
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.