Closed Bug 1481247 Opened Last year Closed Last year

MacroAssembler::compareStrings checks twice for pointer equivalence

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 1 obsolete file)

MacroAssembler::compareStrings currently emits for x64:
---
[Codegen] instruction CompareS
[Codegen] cmpq       %r8, %rsi
[Codegen] jne        .Lfrom8017
[Codegen] movl       $0x1, %edi
[Codegen] jmp        .Lfrom8027
[Codegen] .set .Llabel8027, .
[Codegen] .set .Lfrom8017, .Llabel8027
[Codegen] testl      $0x1, 0x0(%rsi)
[Codegen] jne        .Lfrom8039
[Codegen] testl      $0x1, 0x0(%r8)
[Codegen] jne        .Lfrom8052
[Codegen] cmpq       %r8, %rsi
[Codegen] sete       %dil
[Codegen] movzbl     %dil, %edi
[Codegen] jmp        .Lfrom8068
[Codegen] .set .Llabel8068, .
[Codegen] .set .Lfrom8052, .Llabel8068
[Codegen] .set .Lfrom8039, .Llabel8068
[Codegen] movl       0x4(%rsi), %edi
[Codegen] cmpl       %edi, 0x4(%r8)
[Codegen] je         .Lfrom8081
[Codegen] xorl       %edi, %edi
[Codegen] .set .Llabel8083, .
[Codegen] .set .Lfrom8068, .Llabel8083
[Codegen] .set .Lfrom8027, .Llabel8083
[Codegen] .set .Llabel8083, .
---


But can be optimized to:
---
[Codegen] instruction CompareS
[Codegen] cmpq       %r8, %rsi
[Codegen] jne        .Lfrom8017
[Codegen] movl       $0x1, %edi
[Codegen] jmp        .Lfrom8027
[Codegen] .set .Llabel8027, .
[Codegen] .set .Lfrom8017, .Llabel8027
[Codegen] testl      $0x1, 0x0(%rsi)
[Codegen] jne        .Lfrom8039
[Codegen] testl      $0x1, 0x0(%r8)
[Codegen] je         .Lfrom8052
[Codegen] .set .Llabel8052, .
[Codegen] .set .Lfrom8039, .Llabel8052
[Codegen] movl       0x4(%rsi), %edi
[Codegen] cmpl       %edi, 0x4(%r8)
[Codegen] je         .Lfrom8065
[Codegen] .set .Llabel8065, .
[Codegen] .set .Lfrom8052, .Llabel8065
[Codegen] xorl       %edi, %edi
[Codegen] .set .Llabel8067, .
[Codegen] .set .Lfrom8027, .Llabel8067
[Codegen] .set .Llabel8067, .
---
Comment on attachment 8997973 [details] [diff] [review]
bug1481247.patch

Review of attachment 8997973 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find

::: js/src/jit/MacroAssembler.cpp
@@ +1428,5 @@
>      MOZ_ASSERT(IsEqualityOp(op));
>  
>      Label done;
>      Label notPointerEqual;
> +    // Pointer comparison to detect identical strings.

"Identical" seems to be ambiguous between pointer equality and content equality.

// If operands point to same JSString (or JSAtom) instance, set result for equalality.

@@ +1436,5 @@
>  
>      bind(&notPointerEqual);
>  
>      Label notAtom;
> +    Label bothAtom;

s/bothAtom/setNotEqualResult/

Let's describe the action instead
Attachment #8997973 - Flags: review?(tcampbell) → review+
Attached patch bug1481247.patchSplinter Review
(In reply to Ted Campbell [:tcampbell] from comment #2)
> > +    // Pointer comparison to detect identical strings.
> 
> "Identical" seems to be ambiguous between pointer equality and content
> equality.
> 
> // If operands point to same JSString (or JSAtom) instance, set result for
> equalality.
> 

Ended up with "If operands point to the same instance, the strings are trivially equal.", which still fits in 79 columns.

Carrying r+.
Attachment #8997973 - Attachment is obsolete: true
Attachment #8999225 - Flags: review+
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7311e3026a47
Don't compare pointers twice in MacroAssembler::compareStrings. r=tcampbell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7311e3026a47
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.