Use MConcat for more cases

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Blocks: 1 bug, {perf})

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Looking at pdfjs I saw that we didn't optimize 'str + undefined' or 'str + undefined|str'. We don't support it because our MToString didn't support every case. We now already support more. As a result we should enable MConcat in more cases.

This gives me a 4% on pdfjs.
(Assignee)

Comment 1

2 years ago
Created attachment 8823101 [details] [diff] [review]
Patch
Assignee: nobody → hv1989
Attachment #8823101 - Flags: review?(jdemooij)
(Assignee)

Updated

2 years ago
Blocks: 1307062
Priority: -- → P3
Comment on attachment 8823101 [details] [diff] [review]
Patch

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

Good catch.

::: js/src/jit/IonBuilder.cpp
@@ +3221,5 @@
>          trackOptimizationOutcome(TrackedOutcome::OperandNotString);
>          return Ok();
>      }
>  
> +    // The none-string input (if present) should be atleast easily coercible to string.

Pre-existing nit: non-string

@@ +3222,5 @@
>          return Ok();
>      }
>  
> +    // The none-string input (if present) should be atleast easily coercible to string.
> +    if (right->type() != MIRType::String && 

Nit: trailing whitespace. Also can we now remove this check (and for 'left') and just do the mightBeType() calls?
Attachment #8823101 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

2 years ago
Keywords: perf
(Assignee)

Comment 3

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 8823101 [details] [diff] [review]
> Patch
> 
> Review of attachment 8823101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good catch.
> 
> ::: js/src/jit/IonBuilder.cpp
> @@ +3221,5 @@
> >          trackOptimizationOutcome(TrackedOutcome::OperandNotString);
> >          return Ok();
> >      }
> >  
> > +    // The none-string input (if present) should be atleast easily coercible to string.
> 
> Pre-existing nit: non-string
> 
> @@ +3222,5 @@
> >          return Ok();
> >      }
> >  
> > +    // The none-string input (if present) should be atleast easily coercible to string.
> > +    if (right->type() != MIRType::String && 
> 
> Nit: trailing whitespace. Also can we now remove this check (and for 'left')
> and just do the mightBeType() calls?

Not really. That would be speculating. This would optimize [string int32] + [string int32] to always use MConcat (which would bail sometimes). We could probably try to specialize based on only seeing ICBinaryArith_StringConcat in the binary ics. Opened bug 1328826 for that

Comment 4

2 years ago
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d8af817497
IonMonkey - Use MConcat for more cases, r=jandem

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/94d8af817497
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1342882
You need to log in before you can comment on or make changes to this bug.