Closed
Bug 1073576
Opened 10 years ago
Closed 10 years ago
Optimize strict compares with equal operands
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(1 file, 1 obsolete file)
4.48 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
We have code like SameValueZero that uses x !== x to check for NaN. We can speed up Array#contains by optimizing x === x and x !== x, where we know the logical results. I.e. the operands aren't NaN.
Assignee | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
Duplicate of Bug 1073575?
Comment 4•10 years ago
|
||
Comment on attachment 8496022 [details] [diff] [review] equal-compare-jit Review of attachment 8496022 [details] [diff] [review]: ----------------------------------------------------------------- I totally forgot my review queue Monday and Tuesday. That's the reason for my late response, sorry. - What is the reason "Compare_Value" is not allowed? Compare_Value does a bitwise value compare, which is only allowed for primitives/objects, but disallowed for NaN. As a result this NaN testing isn't an issue for folding. - Can you add tests for the different compare types doing this NaN check. I think this isn't well tested in current tests. ::: js/src/jit/MIR.cpp @@ +2836,5 @@ > + return false; > + > + if (compareType_ == Compare_Value || compareType_ == Compare_Unknown) > + return false; > + Can you add MOZ_ASSERT(compareType == XXX || compareType == XXX || ...) for all cases that are supported here? In case a new compareType gets added, we get an assert, hinting that we need to check if this is allowed to fold.
Attachment #8496022 -
Flags: review?(hv1989)
Assignee | ||
Comment 5•10 years ago
|
||
I have to admit, I didn't really check how Compare_Value works, but it looks like we can't end up with doubles so we are good. I think Compare_Boolean, Compare_Undefined and Compare_Null are fine, because one side needs to be boolean, undefined or null. It would be good if we could get some targeted fuzzing on that pattern (x === x and x !== x) somehow.
Attachment #8496022 -
Attachment is obsolete: true
Attachment #8498416 -
Flags: review?(hv1989)
Comment 6•10 years ago
|
||
Comment on attachment 8498416 [details] [diff] [review] v2 Review of attachment 8498416 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/basic/strict-compare-same-operands.js @@ +35,5 @@ > + assertEq(h !== h, false); > + assertEq(i === i, true); > + assertEq(i !== i, false); > + assertEq(j === j, true); > + assertEq(j !== j, false); This will probably get filtered out in the JS parser or atleast a lot of them. You will need to create functions that you prepare with the right types: function undefinedEqualString(a, b) { return a === b } undefinedEqualString(undefined, "s") undefinedEqualString(undefined, "t") function undefinedEqualValue(a, b) { return a === b } undefinedEqualValue(undefined, "s") undefinedEqualValue(undefined, false) undefinedEqualValue(undefined, undefined) undefinedEqualValue(undefined, "s") undefinedEqualValue(undefined, false) undefinedEqualValue(undefined, undefined) function NaNEqualValue(a, b) { return a === b } NaNEqualValue(NaN, "s") NaNEqualValue(NaN, false) NaNEqualValue(NaN, "s") NaNEqualValue(NaN, false) ... Also run them twice. In a for loop or manually like I did here. Since it might detect a type change and get compiled, but not enter yet. ::: js/src/jit/MIR.cpp @@ +2840,5 @@ > + > + switch (compareType_) { > + case Compare_Undefined: > + case Compare_Null: > + MOZ_ASSERT(lhs()->type() != MIRType_Value); MIRType_Value for lhs() is also good, since it is strict equality with undefined/null @@ +2848,5 @@ > + case Compare_Int32MaybeCoerceBoth: > + case Compare_Int32MaybeCoerceLHS: > + case Compare_Int32MaybeCoerceRHS: > + case Compare_UInt32: > + MOZ_ASSERT(true); No need for this MOZ_ASSERT(true); @@ +2865,5 @@ > + case Compare_StrictString: > + MOZ_ASSERT(lhs()->type() == MIRType_String); > + break; > + case Compare_Object: > + MOZ_ASSERT(true); No need for this MOZ_ASSERT(true); @@ +2868,5 @@ > + case Compare_Object: > + MOZ_ASSERT(true); > + break; > + case Compare_Value: > + MOZ_ASSERT(!lhs()->mightBeType(MIRType_Double)); MOZ_ASSERT(!rhs()->mightBeType(MIRType_Double)); @@ +2872,5 @@ > + MOZ_ASSERT(!lhs()->mightBeType(MIRType_Double)); > + break; > + case Compare_Unknown: > + default: > + MOZ_ASSERT(false); you should use MOZ_CRASH() here. @@ +2874,5 @@ > + case Compare_Unknown: > + default: > + MOZ_ASSERT(false); > + break; > + } Oh I don't think this gives extra value. I was just fine with doing: MOZ_ASSERT(compareType_ == Compare_Undefined || compareType == Compare_Null || compareType == Compare_Int32 ...); Which I would prefer. (But feel free to keep this, if you fix the remarks I had.)
Attachment #8498416 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 7•10 years ago
|
||
> This will probably get filtered out in the JS parser or atleast a lot of them. I don't think so, I actually checked the MIR to make sure. >You will need to create functions that you prepare with the right types: Okay I see. Coming up with a code pattern for all these different compare types is highly annoying.
Assignee | ||
Comment 8•10 years ago
|
||
Oh and I also don't see how your examples would work, because a and b are obviously different, so it would already fail the first check. I am going to land my patch for now and ask the fuzzers if they can target this pattern.
Assignee | ||
Updated•10 years ago
|
Summary: Optimize compares with equal operands → Optimize strict compares with equal operands
Assignee | ||
Comment 9•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=87e3705c59e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/1724a6c07316
Assignee | ||
Comment 10•10 years ago
|
||
Uhh I screwed up. Backing out.
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5d631abcd56
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e5d631abcd56
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•