578528 - JM: Int32 fast-path for JSOP_STRICTEQ, JSOP_STRICNE.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: Int32 fast-path for JSOP_STRICTEQ.

JSNES frequently uses strict equality comparison. Making a fast-path for Int32 yields a significant perf win.

A separate patch will get rid of StrictEqWithStore() by having rejoin() respect ReturnReg, but that patch is more of an architectural change than appropriate here.

SS/v8 do not avail themselves of JSOP_STRICTEQ, so there is no perf diff.

Comment 1

[Brendan Eich [:brendan]]

JSLint IIRC requires === and !==, so we should do this (crock dogma, == and != are not equivalence relations if operand types in typeof sense differ). Good that JSNES caught it.

/be

Comment 2

[Sean Stangl [:sstangl]]

Attachment: Int32 fast-path for stricteq, strictne.

Rebased the patch from two months ago. This adds an easy +5fps to JSNES for me.

Jan filed a bug to give stricteq the same treatment as jsop_equality(). This bug blocks that one.

Comment 3

[Brendan Eich [:brendan]]

=== and !== are well worth optimizing. JSLint and various such things require 'em.

/be

Comment 4

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

Comment on attachment 468941 [details] [diff] [review]
Int32 fast-path for stricteq, strictne.


>+    /*
>+     * TODO: x64 can do full-Value comparisons. This is beneficial
>+     * to do if the payload/type are not yet in registers.
>+     */

No :TODO: - bug number or something softer like "NB: x64 could do ..."

>+
>+    if (frame.haveSameBacking(lhs, rhs)) {
>+        /* False iff NaN. */
>+        if (lhs->isTypeKnown() && lhs->getKnownType() != JSVAL_TYPE_DOUBLE) {

lhs->isNotType(JSVAL_TYPE_DOUBLE)

Again, no :TODO: in this path either. If it's important it should be filed.

>+    /* Is it impossible that both Values are ints? */
>+    if ((lhs->isTypeKnown() && lhs->getKnownType() != JSVAL_TYPE_INT32) ||
>+        (rhs->isTypeKnown() && rhs->getKnownType() != JSVAL_TYPE_INT32)) {

ditto, use isNotType()

>+
>+    if (!rhs->isTypeKnown() && !frame.haveSameBacking(lhs, rhs)) {
>+        Jump j= frame.testInt32(Assembler::NotEqual, rhs);

Missing space before =

>+        if (op == JSOP_STRICTEQ)
>+            stubcc.call(stubs::StrictEqWithStore);
>+        else
>+            stubcc.call(stubs::StrictNeWithStore);

I prefer just putting an extra store in StrictEq/StrictNe. The less stubs to maintain the better.

>+    /*
>+     * TODO: We need a rejoin function that respect values

*cough* - TODO again. You can hack this by rejoining before you push, and using crossJump() after. Probably doesn't matter, up to you.

r=me with these picked

Comment 5

[Sean Stangl [:sstangl]]

(In reply to comment #4)
> I prefer just putting an extra store in StrictEq/StrictNe. The less stubs to
> maintain the better.

Definitely. I'm going to add the extra store instead of adding hacky code. I'll file a follow-up bug, because x64 can take serious advantage of being able to return whole-values in registers. I (vaguely) recall that when I was writing this patch I measured the effect on a microbenchmark of storing the stub call result versus returning it in a register, and the slowdown from the store was noticeable.

Sorry about all the TODOs -- I forgot they designated important items.

Comment 6

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/projects/jaegermonkey/rev/c8fbdb5eda1f