Closed Bug 1328148 Opened 5 years ago Closed 5 years ago

Use MConcat for more cases

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file)

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.
Attached patch PatchSplinter Review
Assignee: nobody → hv1989
Attachment #8823101 - Flags: review?(jdemooij)
Blocks: jsperf
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+
Keywords: perf
(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
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d8af817497
IonMonkey - Use MConcat for more cases, r=jandem
https://hg.mozilla.org/mozilla-central/rev/94d8af817497
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1342882
You need to log in before you can comment on or make changes to this bug.