Bug 145992 – Strict Equality on objects should only check that one of the two sides is an object.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Keith Miller]

If the JIT speculates that a strict equality is only checking objects we should not check that both sides are objects. Instead, it would be more efficient to check that one side is an object then do a pointer comparison on the operands.

[Keith Miller]

Created attachment 254974 [details]
The Patch

[WebKit Commit Bot]

Attachment 254974 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1310:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 254974 [details]
The Patch

Attachment 254974 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4871315928383488

New failing tests:
js/dom/document-all-strict-eq.html

[Build Bot]

Created attachment 254977 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 254974 [details]
The Patch

Attachment 254974 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6521584097427456

New failing tests:
js/dom/document-all-strict-eq.html

[Build Bot]

Created attachment 254981 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Keith Miller]

Created attachment 254984 [details]
Fixed Patch

whoops...

[Build Bot]

Comment on attachment 254984 [details]
Fixed Patch

Attachment 254984 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4656447304499200

New failing tests:
js/dom/document-all-strict-eq.html

[Build Bot]

Created attachment 254986 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 254984 [details]
Fixed Patch

Attachment 254984 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5930505329442816

New failing tests:
js/dom/document-all-strict-eq.html

[Build Bot]

Created attachment 254987 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Keith Miller]

Created attachment 254990 [details]
Patch Expected File

Added new line to the end of the patch. Not sure why this was a not a problem on my machine.

[Darin Adler]

Comment on attachment 254990 [details]
Patch Expected File

I’m probably not qualified to review the patch. But was just wondering: Would it be faster to do an early exit if the two values are identical before doing any type checking? Then if the type is object we can return false. It seems that this makes strict equality comparisons of objects that turn out to be equal faster without slowing down the ones that turn out to be not equal. But maybe it would slow down some other important cases? Besides objects, I believe that undefined, null, booleans, and integers would all benefit from this. Floating point numbers and strings might be slowed down slightly by the extra check, I guess.

[Keith Miller]

I could be wrong but I believe the issue with your idea Darin is that I don't believe our current abstract interpreter is set up to handle cases where type checks only happen under certain conditions. I think currently, we wouldn't be able to propagate any type check had occurred. Thus, if the condition was false we would have to type check the operand twice (once at the equality and again the next time the operand has a use kind of Object).

[Michael Saboff]

Comment on attachment 254990 [details]
Patch Expected File

View in context: https://bugs.webkit.org/attachment.cgi?id=254990&action=review

Almost there.

> LayoutTests/ChangeLog:7
> +

Explain that you added this test for the new optimization.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:5
> +        return 0;

It is clearer to return true or false.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:7
> +    return 1;

It is clearer to return true or false.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:13
> +for (var i = 1; i < 1000; i++) {

This executes 999 times.  Start with i = 0.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:14
> +    shouldBe("f(test, test)", "0");

Construct this so that you keep track of the the result and provide one shouldBeXXX() or test{Passed,Failed} at the end of the loop.  For example, as long as f(test, test) == 0, you continue the loop.

var result = true;
for (var i = 0; i < 1000; ++i) {
    if (f(test, test) != 0) {
        result = false;
        break;
    }
}

if (result)
    testPassed("f(test, test) compared correctly");
else
    testFailed("f(test, test) did not compare correctly");

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:25
> +    for (var i = 1; i < 1000; i++) {
> +        shouldBe("f(test, test)", "0");
> +    }

Same comments as above.

> Source/JavaScriptCore/ChangeLog:9
> +        to hoist type checks out of a loop we can be cleverer about how we choose

Is cleverer a word?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1175
> +        BasicBlock* tmp = taken;
> +        taken = notTaken;
> +        notTaken = tmp;

Use std::swap()

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3950
> +            ASSERT(false);

Is this debug code?  It needs to be removed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1206
> +    MacroAssembler::Jump falseCase = m_jit.branchPtr(MacroAssembler::NotEqual, op1GPR, op2GPR);
> +    m_jit.move(TrustedImm32(1), resultPayloadGPR);
> +    MacroAssembler::Jump done = m_jit.jump();
> +    falseCase.link(&m_jit);
> +    m_jit.move(TrustedImm32(0), resultPayloadGPR);
> +    done.link(&m_jit);

You could replace this with:
    m_jit.compare32(JITCompiler::Equal, op1GPR, op2GPR, resultPayloadGPR);

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1319
> +    MacroAssembler::Jump falseCase = m_jit.branch64(MacroAssembler::NotEqual, op1GPR, op2GPR);
> +    m_jit.move(TrustedImm32(ValueTrue), resultGPR);
> +    MacroAssembler::Jump done = m_jit.jump();
> +    falseCase.link(&m_jit);
> +    m_jit.move(TrustedImm32(ValueFalse), resultGPR);
> +    done.link(&m_jit);

You can replace this with:
    m_jit.compare64(JITCompiler::Equal, op1GPR, op2GPR, resultGPR);
    m_jit.or32(TrustedImm32(ValueFalse), resultGPR);

[Keith Miller]

Created attachment 255042 [details]
The Patch

[Keith Miller]

Cleverer is in fact a word :P (http://dictionary.reference.com/browse/cleverer)

[Keith Miller]

Created attachment 255056 [details]
New Patch

Fixed unforeseen issues. Should be ok now.

[Build Bot]

Comment on attachment 255056 [details]
New Patch

Attachment 255056 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6089779896647680

New failing tests:
platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-horizontal.html

[Build Bot]

Created attachment 255059 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Filip Pizlo]

Comment on attachment 255056 [details]
New Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255056&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1177
> +    SpeculateCellOperand op2(this, node->child2(), ManualOperandSpeculation);

This is wrong.  You should be using JSValueOperand if the right hand side is UntypedUse.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1185
> +    if (m_state.forNode(node->child1()).m_type & ~SpecObject) {
> +        speculationCheck(
> +            BadType, JSValueSource::unboxedCell(op1GPR), node->child1(), m_jit.branchIfNotObject(op1GPR));
> +    }

You should use DFG_TYPE_CHECK.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1190
> +    JSValueOperand op2(this, node->child2(), ManualOperandSpeculation);

You shouldn't need ManualOperandSpeculation if you're calling this with child2 having UntypedUse.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1302
> +    JSValueOperand op2(this, node->child2(), ManualOperandSpeculation);

Ditto.

[Keith Miller]

Created attachment 255131 [details]
Patch

[Keith Miller]

Created attachment 255137 [details]
Patch

[Filip Pizlo]

Comment on attachment 255137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255137&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1188
> +    SpeculateCellOperand op1(this, node->child1());
> +    JSValueOperand op2(this, node->child2());
> +
> +    GPRReg op1GPR = op1.gpr();
> +#if USE(JSVALUE64)
> +    GPRReg op2GPR = op2.gpr();
> +#else
> +    GPRReg op2GPR = op2.payloadGPR();
> +#endif
> +    DFG_TYPE_CHECK(
> +        JSValueSource::unboxedCell(op1GPR), node->child1(), SpecObject, m_jit.branchIfNotObject(op1GPR));
> +
> +    branchPtr(condition, op1GPR, op2GPR, taken);

This appears wrong on 32-bit, since there you still have to test the tag word.  Also, you could avoid the #if's by doing:

JSValueRegs op2Regs = op2.jsValueRegs();
GPRReg op2Payload = op2Regs.payloadGPR();

But that doesn't really help you with the tag issue on 32-bit.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1204
> +    SpeculateCellOperand op1(this, node->child1());
> +    JSValueOperand op2(this, node->child2());
> +
> +    GPRReg op1GPR = op1.gpr();
> +    GPRReg op2GPR = op2.payloadGPR();
> +
> +    DFG_TYPE_CHECK(
> +        JSValueSource::unboxedCell(op1GPR), node->child1(), SpecObject, m_jit.branchIfNotObject(op1GPR));
> +
> +    GPRTemporary resultPayload(this, Reuse, op1);
> +    GPRReg resultPayloadGPR = resultPayload.gpr();
> +
> +    // At this point we know that we can perform a straight-forward equality comparison on pointer
> +    // values because we are doing strict equality.
> +    m_jit.compare32(MacroAssembler::Equal, op1GPR, op2GPR, resultPayloadGPR);
> +    booleanResult(resultPayloadGPR, node);

Ditto.  Need a story for the tag word on 32-bit.

[Keith Miller]

Created attachment 255266 [details]
Patch

Changed the way the optimization works to what it probably should have been from the beginning. Now as long as one of the two arguments is an object we can just type check that side and ignore the type of the other (other than checking the tag is a cell tag in the 32-bit case). We still could be smarter about how we pick the side if both are objects but until we hoist type checks out of loops it probably doesn't matter.

[WebKit Commit Bot]

Attachment 255266 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4171:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4172:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4173:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:489:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 255267 [details]
Patch

Fixed some minor issues

[Keith Miller]

Created attachment 255268 [details]
Patch

forgot to set commit-queue

[Mark Lam]

Comment on attachment 255268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255268&action=review

> LayoutTests/ChangeLog:9
> +        with document.all.

I think it’d be useful to add a comment about document.all masquerading as undefined.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:9
> +var test = {};

nit: Normally, when we say test, we expect some code that runs the test.  I think you mean a testObj or something like that.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:14
> +for (var i = 0; i < 1000; i++) {
> +    if (!f(test,test))
> +        testFailed("f(test,test) should have been true but got false");
> +}

Please add a comment above this to indicate that you’re warming up the DFG function here before the "masquerades as undefined” watchpoint fires.

> LayoutTests/js/dom/script-tests/document-all-strict-eq.js:24
> +    for (var i = 0; i < 1000; i++) {
> +        if (!f(test,test))
> +            testFailed("f(test,test) should have been true but got false");
> +    }

Please add a comment above this to indicate that you’re warming up the DFG function here after the "masquerades as undefined” watchpoint fires.

> Source/JavaScriptCore/ChangeLog:8
> +        This patch makes it so as long as we know that at least one side of a strict
> +        equality check is an speculated to be an object we will only type check
> +        that side. Equality is then determined by a pointer comparision between the

typo?  I think you meant, “is speculated to be”.  Also it’d be clearer if you replace “object we” with “object, then we”.

> Source/JavaScriptCore/ChangeLog:9
> +        two values (although in the 32-bit case we must also check the other side

typo: must also check *that*

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1234
> +    MacroAssembler::Jump op2NotCellJump = m_jit.branchIfNotCell(op2.jsValueRegs());

Per our offline discussion, this is incorrect if (taken == nextBlock()).  Let’s not do the swap thing.  Let’s just have 2 different cases.

> Source/JavaScriptCore/jsc.cpp:884
> +    JSValue cell = exec->argument(0);

nit: I think it’s better to call this “value” instead of “cell” since we don’t know if it’s actually a cell.

> Source/JavaScriptCore/jsc.cpp:888
> +    int64_t asNumber = reinterpret_cast<int64_t>(cell.asCell());

It’s better to use a uint64_t here (though we don’t expect the high bit of the cell pointer to be set in practice).

> Source/JavaScriptCore/tests/stress/equality-type-checking.js:18
> +function dfgify(obj, addr) {
> +
> +
> +}

Please delete.

> Source/JavaScriptCore/tests/stress/equality-type-checking.js:22
> +

Please add a comment to explain that you are testing that, on 32-bit builds, strict equality has not neglected to compare the tags as well.  I think this detail is not clear from just reading this code.

[Keith Miller]

Created attachment 255486 [details]
Patch

Made the changes Mark suggested. I'm not actually sure the reinterpret_cast<uint64_t> is necessary as I don't believe reinterpret_cast sign extends but I made the change anyway, since it won't hurt.

[Mark Lam]

Comment on attachment 255486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255486&action=review

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:9
> +        comparison between the two values (although in the 32-bit case we must also check

missing “that”: … check *that* the other ...

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1235
> +    if (taken == nextBlock()) {
> +        branch32(MacroAssembler::NotEqual, op1GPR, op2GPR, notTaken);
> +        jump(taken);
> +    } else {
> +        branch32(MacroAssembler::Equal, op1GPR, op2GPR, taken);
> +        jump(notTaken);
> +    }

This is so much cleaner and easier to read and to reason about.  Is there a reason why you wouldn’t do it this way for the 64-bit version as well?

> Source/JavaScriptCore/jsc.cpp:888
> +    // Need to cast to int64_t so bitwise_cast will play along.
> +    int64_t asNumber = reinterpret_cast<uint64_t>(value.asCell());

You are right that a reinterpret_cast won’t sign extend.  So, my previous suggestion to use uint64_t is not really necessary (though uint64_t does feel warm and fuzzier than int64_t).  That said, you’re casting to uint64_t here, but the result type is still int64_t.  Ditto for comment.

> Source/JavaScriptCore/tests/stress/equality-type-checking.js:3
> + * when determining equality via pointer comparision.

typo: comparison.

[Michael Saboff]

Comment on attachment 255486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255486&action=review

> Source/JavaScriptCore/tests/stress/equality-type-checking.js:23
> +if (address === undefined)

Since address is a Number, we should fail with the type check part of === and never get to compare "address" with the Cell*.  I'm trying to figure out how comparing against the address as a number adds to the testing.

> Source/JavaScriptCore/tests/stress/equality-type-checking.js:26
> +if (foo === address || address === foo)

Same comment about comparing the a cell* address as a number with the object.

[Mark Lam]

(In reply to comment #32)
> Comment on attachment 255486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255486&action=review
> 
> > Source/JavaScriptCore/tests/stress/equality-type-checking.js:23
> > +if (address === undefined)
> 
> Since address is a Number, we should fail with the type check part of ===
> and never get to compare "address" with the Cell*.  I'm trying to figure out
> how comparing against the address as a number adds to the testing.

The bug he is testing for is that there’s a 32-bit case where he / someone forgot to compare the tag as well.  In that case, the number would masquerade as the cell.  This test ensures that === compares the tag as well.

[Keith Miller]

Created attachment 255495 [details]
Patch

Added comments and fixed the things Mark mentioned.

[WebKit Commit Bot]

Comment on attachment 255495 [details]
Patch

Clearing flags on attachment: 255495

Committed r185920: <http://trac.webkit.org/changeset/185920>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.