Closed
Bug 1433432
Opened 6 years ago
Closed 6 years ago
Inline Object.is in Ion
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file, 1 obsolete file)
30.96 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
If we add inlining support for Object.is(...), users don't need to worry whether or not calling Object.is(...) is (much) slower than using the triple-equals operator.
Assignee | ||
Comment 1•6 years ago
|
||
This patch adds inlining support for Object.is(...). Notable changes: MCallOptimize.cpp: - If possible, |Object.is| is optimized as |===| using the existing MCompare class. - If that's not possible, the new MSameValue class is used. - I've used |callInfo.argc() < 2| as the inlining guard instead of the more common exact arguments count matching |callInfo.argc() == 2| just in case users start to use patterns like: var isNull = Object.is.bind(null, null); var hasNullElements = array.some(isNull); because that'd call Object.is with more than two arguments. TypePolicy.{h,cpp} - Added a separate type policy for MSameValue, because when lowered, only three cases are supported: - Both arguments are doubles. - Both arguments are values. - The first argument is a value, the second a double. - This approach matches how MCompare optimises certain argument combinations: https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/js/src/jit/IonBuilder.cpp#5799-5808 IonBuilder.cpp: - Adding the extra bool argument to IonBuilder::compareTrySpecialized(...) feels a bit clumsy. But I don't quite see how to improve this right now... CodeGenerator.cpp The code to check for negative zero was copied (including the comments!) from https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/js/src/jit/CodeGenerator.cpp#12242-12250. I know that this won't result in efficient machine code [1], but I didn't see any more applicable MacroAssembler methods for this problem. But maybe someone who actually knows Assembler can make this faster in a follow-up bug. :-) [1] For example using MacroAssemblerX86Shared::branchNegativeZero(...) made µ-benchmarks ~20% faster, but branchNegativeZero(...) is only available when targeting X86.
Attachment #8945761 -
Flags: review?(jdemooij)
Comment 2•6 years ago
|
||
Comment on attachment 8945761 [details] [diff] [review] bug1433432.patch Review of attachment 8945761 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks! Maybe we should extend jit-test/tests/basic/object-is.js (or add a new file) with some tests for at least the double cases that are inlined? Please make sure that adding a subtle bug to the masm code results in a test failure somewhere, at least with jit_test.py --ion/--tbpl :) ::: js/src/jit/CodeGenerator.cpp @@ +7594,5 @@ > + // is -Infinity instead of Infinity. > + Label isNegInf; > + masm.loadConstantDouble(1.0, temp); > + masm.divDouble(left, temp); > + masm.branchDouble(Assembler::DoubleLessThan, temp, left, &isNegInf); Please file a follow-up bug to use branchNegativeZero :) ::: js/src/jit/MCallOptimize.cpp @@ +2422,5 @@ > + // Specialize |Object.is(lhs, rhs)| as |lhs === rhs|. > + bool emitted = false; > + MOZ_TRY(compareTrySpecialized(&emitted, JSOP_STRICTEQ, left, right, false)); > + if (!emitted) > + return InliningStatus_NotInlined; Could we use MSameValue in this case too? ::: js/src/jit/shared/LIR-shared.h @@ +3163,5 @@ > + LSameValueV(const LBoxAllocation& left, const LAllocation& right, const LDefinition& temp1, > + const LDefinition& temp2) > + { > + setBoxOperand(LhsInput, left); > + setOperand(1, right); BOX_PIECES instead of 1, so it works on 32-bit.
Attachment #8945761 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2) > ::: js/src/jit/CodeGenerator.cpp > @@ +7594,5 @@ > > + // is -Infinity instead of Infinity. > > + Label isNegInf; > > + masm.loadConstantDouble(1.0, temp); > > + masm.divDouble(left, temp); > > + masm.branchDouble(Assembler::DoubleLessThan, temp, left, &isNegInf); > > Please file a follow-up bug to use branchNegativeZero :) I was hoping for better generated assembler than the one emitted when branchNegativeZero is used. :-) For example this is what GCC emits for the equivalent in C++: --- _ZL9SameValuedd: movq %xmm0, %rax movq %xmm1, %rcx cmpq %rcx, %rax je .L5 ucomisd %xmm0, %xmm0 setp %dl ucomisd %xmm1, %xmm1 setp %al andl %edx, %eax ret .L5: movl $1, %eax ret --- Source: --- #include <cstdint> #include <cstdlib> inline uint64_t BitwiseCast(const double aFrom) { union { double mFrom; uint64_t mTo; } u; u.mFrom = aFrom; return u.mTo; } [[gnu::noinline]] static bool SameValue(double aValue1, double aValue2) { return BitwiseCast(aValue1) == BitwiseCast(aValue2) || (aValue1 != aValue1 && aValue2 != aValue2); } int main(int argc, char** argv) { double d1 = std::atof(argv[0]); double d2 = std::atof(argv[1]); return int(SameValue(d1, d2)); } --- But I don't know how difficult it is to implement this for each architecture and whether or not it's worthwhile to spend too much time improving this at the assembly level. > > ::: js/src/jit/MCallOptimize.cpp > @@ +2422,5 @@ > > + // Specialize |Object.is(lhs, rhs)| as |lhs === rhs|. > > + bool emitted = false; > > + MOZ_TRY(compareTrySpecialized(&emitted, JSOP_STRICTEQ, left, right, false)); > > + if (!emitted) > > + return InliningStatus_NotInlined; > > Could we use MSameValue in this case too? Yes, we can use MSameValue here, too. I assumed compareTrySpecialized() handles every case which ends up here and only added the if-check instead of an assertion for extra safety. But when I just went through the logic in MCompare::determineCompareType(), it turned out that (Value x Object) and (Value x Symbol) aren't supported by MCompare (all other unsupported cases from MCompare don't apply here). Do you think it makes sense to file a bug to add support for (Value x Object) and (Value x Symbol) in MCompare? Implementation-wise it shouldn't too hard (probably just a type-check and a pointer-comparison if the types match?).
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2) > Maybe we should extend jit-test/tests/basic/object-is.js (or add a new file) > with some tests for at least the double cases that are inlined? > > Please make sure that adding a subtle bug to the masm code results in a test > failure somewhere, at least with jit_test.py --ion/--tbpl :) The original patch already had a subtle bug which was caught when I added a new test for the double inlining case (the comparison used DoubleNotEqual instead of DoubleNotEqualOrUnordered). :-)
Assignee | ||
Comment 5•6 years ago
|
||
Updated per review comments: - Added jit-test/tests/basic/object-is-inlined.js to test when Object.is is called with (Double|Int32|Value) x (Double|Int32|Value). - Changed IonBuilder::inlineObjectIs() to emit MSameValue if compareTrySpecialized() failed. - Fixed LSameValueV to use the correct operand index (BOX_PIECES instead of 1). Fixed issues: - Fixed the first comparison in CodeGenerator::emitSameValue() to use DoubleNotEqualOrUnordered instead of DoubleNotEqual. - Also changed the second comparison from DoubleNotEqualOrUnordered to DoubleNotEqual, because the second comparison is never executed for NaN values, and using DoubleNotEqual instead of DoubleNotEqualOrUnordered seems to be preferable if I understand this comment correctly: https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/js/src/jit/x86-shared/Assembler-x86-shared.h#317-319,334 Carrying r+.
Attachment #8945761 -
Attachment is obsolete: true
Attachment #8946772 -
Flags: review+
Assignee | ||
Comment 6•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7f4fac486c592d39679d6ea672851c94b365c0d
Keywords: checkin-needed
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2) > Please file a follow-up bug to use branchNegativeZero :) Filed bug 1434650.
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/269c43e9132e Inline Object.is(...) in Ion. r=jandem
Keywords: checkin-needed
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/269c43e9132e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•