940925 - IonMonkey: Don't inspect baseline binary arithmetic IC if it had unoptimizable operands

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

See bug 940062 comment 7.

Comment 1

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1750f842e783

Comment 2

[Chris Peterson [:cpeterson]]

This patch will fix a 5x perf regression since Firefox 22 on a Tom's Hardware benchmark (WebXPRT 2013 Face Detection bug 940062).

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

How risky is it to land this so late in Beta?  We're too late for speculative fixes so knowing the risk here (reward is obviously great) would help in determining whether we can track this and uplift.  It would have to be nominated/landed before tomorrow's beta 7 go to build.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to [e-mail] [:lsblakk] from comment #3)
> How risky is it to land this so late in Beta?  We're too late for
> speculative fixes so knowing the risk here (reward is obviously great) would
> help in determining whether we can track this and uplift.

The patch disables a certain optimization in some cases. That makes it very low risk from a correctness point-of-view. It *could* regress some benchmarks, but this is not very likely and AWFY confirms none of the major ones are affected. Furthermore, we *do* know for sure that it fixes at least one important benchmark, bug 940062...

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for this to be nominated for approval/landing in time for tomorrow's beta.  Since we only have one beta next week before our last one we should ensure this gets nominated as soon as it's on central successfully.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1750f842e783

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8335206 [details] [diff] [review]
Patch

(In reply to [e-mail] [:lsblakk] from comment #5)
> Tracking for this to be nominated for approval/landing in time for
> tomorrow's beta.  Since we only have one beta next week before our last one
> we should ensure this gets nominated as soon as it's on central successfully.

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Unknown, bug was likely introduced in Firefox 23.

> User impact if declined:
Slower websites/benchmarks.
 
> Testing completed (on m-c, etc.): 
On m-i and m-c.

> Risk to taking this patch (and alternatives if risky): 
Very low, see comment 4.

> String or IDL/UUID changes made by this patch:
None.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/51aaa91fb655
https://hg.mozilla.org/releases/mozilla-beta/rev/fc80c61d1ef6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/fc80c61d1ef6
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/fed056992aaa