Optimize symbol equality operations

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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

a year ago
Created attachment 8839282 [details] [diff] [review]
Implement symbol equality comparison in Ion
Attachment #8839282 - Flags: review?(hv1989)
(Assignee)

Comment 2

a year ago
I think we should be able to allow symbols for Compare_Bitwise as well, because they have pointer equality.
(Assignee)

Comment 3

a year ago
Created attachment 8839285 [details] [diff] [review]
Implement symbol equality comparison in SharedIC
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8839285 - Flags: review?(hv1989)
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+
(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.
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

a year 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

a year ago
Created attachment 8839425 [details] [diff] [review]
Implement bitwise symbol comparison
Attachment #8839425 - Flags: review?(hv1989)
(Assignee)

Comment 9

a year ago
Created attachment 8839426 [details] [diff] [review]
v2 - Implement symbol equality comparison in Ion
Attachment #8839426 - Flags: feedback?(hv1989)
(Assignee)

Updated

a year ago
Attachment #8839282 - Attachment is obsolete: true
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 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

a year 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

a year 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
Last Resolved: a year 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.