Closed
Bug 1341087
Opened 7 years ago
Closed 7 years ago
Optimize symbol equality operations
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: evilpie, Assigned: evilpie)
References
(Blocks 2 open bugs)
Details
Attachments
(3 files, 1 obsolete file)
6.68 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
4.13 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
10.90 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
We don't support symbol equality operations (==,!=,===,!==) in Ion and SharedIC. Symbol equality supports are extremely easy to implement: it's just a pointer compare. You basically do the same thing we do for objects. This is probably something like 5% of the ARES-6 runtime.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8839282 -
Flags: review?(hv1989)
Assignee | ||
Comment 2•7 years ago
|
||
I think we should be able to allow symbols for Compare_Bitwise as well, because they have pointer equality.
Assignee | ||
Comment 3•7 years ago
|
||
Comment 4•7 years ago
|
||
Comment on attachment 8839282 [details] [diff] [review] Implement symbol equality comparison in Ion Review of attachment 8839282 [details] [diff] [review]: ----------------------------------------------------------------- I think you forgot the TypePolicy updates: http://searchfox.org/mozilla-central/source/js/src/jit/TypePolicy.cpp#209 http://searchfox.org/mozilla-central/source/js/src/jit/TypePolicy.cpp#254 Can you add support for MIRType::Symbol here? Can you also add some testcases that would assert? Seems like if there were tests they would have triggered with this patch? ::: js/src/jit/MIR.cpp @@ +3813,5 @@ > // Handle string comparisons. (Relational string compares are still unsupported). > if (!relationalEq && lhs == MIRType::String && rhs == MIRType::String) > return Compare_String; > > + // Handle symbol comparisons. Can you add: (Relational compare will throw) @@ +4418,5 @@ > compareType_ == Compare_Double || compareType_ == Compare_DoubleMaybeCoerceLHS || > compareType_ == Compare_DoubleMaybeCoerceRHS || compareType_ == Compare_Float32 || > compareType_ == Compare_String || compareType_ == Compare_StrictString || > + compareType_ == Compare_Object || compareType_ == Compare_Bitwise || > + compareType_ == Compare_String); I suspect this needs to be "Compare_Symbol"
Attachment #8839282 -
Flags: review?(hv1989) → review+
Comment 5•7 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #2) > I think we should be able to allow symbols for Compare_Bitwise as well, > because they have pointer equality. I agree. Not sure how often it happens, but it seems obvious/easy to add.
Updated•7 years ago
|
Blocks: sm-js-perf
Comment 6•7 years ago
|
||
Comment on attachment 8839285 [details] [diff] [review] Implement symbol equality comparison in SharedIC Review of attachment 8839285 [details] [diff] [review]: ----------------------------------------------------------------- Good find!
Attachment #8839285 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 7•7 years ago
|
||
You are right, this patch totally only works after fixing the asserts and the type-policy. I wonder why I didn't get any failures while running ARES-6.
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8839425 -
Flags: review?(hv1989)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8839426 -
Flags: feedback?(hv1989)
Assignee | ||
Updated•7 years ago
|
Attachment #8839282 -
Attachment is obsolete: true
Comment 10•7 years ago
|
||
Comment on attachment 8839425 [details] [diff] [review] Implement bitwise symbol comparison Review of attachment 8839425 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding this! ::: js/src/jit/IonBuilder.cpp @@ +5596,2 @@ > > // Onlye allow loose and strict equality. Pre-exising. Can you remove the e on Onlye
Attachment #8839425 -
Flags: review?(hv1989) → review+
Comment 11•7 years ago
|
||
Comment on attachment 8839426 [details] [diff] [review] v2 - Implement symbol equality comparison in Ion Review of attachment 8839426 [details] [diff] [review]: ----------------------------------------------------------------- I assume this was a review request? Thanks for adding the testcase!
Attachment #8839426 -
Flags: feedback?(hv1989) → review+
Comment 12•7 years ago
|
||
Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a9f1f1516ec Implement symbol equality comparison in Ion. r=h4writer https://hg.mozilla.org/integration/mozilla-inbound/rev/f0e0d480f660 Implement bitwise symbol equality comparison in Ion. r=h4writer https://hg.mozilla.org/integration/mozilla-inbound/rev/1de8a2485e15 Implement symbol equality comparison in SharedIC. r=h4writer
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2a9f1f1516ec https://hg.mozilla.org/mozilla-central/rev/f0e0d480f660 https://hg.mozilla.org/mozilla-central/rev/1de8a2485e15
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•