Closed Bug 940925 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox26 + fixed
firefox27 + fixed
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Attached patch PatchSplinter Review
See bug 940062 comment 7.
Attachment #8335206 - Flags: review?(bhackett1024)
Attachment #8335206 - Flags: review?(bhackett1024) → review+
This patch will fix a 5x perf regression since Firefox 22 on a Tom's Hardware benchmark (WebXPRT 2013 Face Detection bug 940062).
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.
Flags: needinfo?(jdemooij)
Flags: needinfo?(cpeterson)
(In reply to lsblakk@mozilla.com [: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...
Flags: needinfo?(jdemooij)
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.
Flags: needinfo?(cpeterson)
https://hg.mozilla.org/mozilla-central/rev/1750f842e783
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment on attachment 8335206 [details] [diff] [review]
Patch

(In reply to lsblakk@mozilla.com [: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.
Attachment #8335206 - Flags: approval-mozilla-beta?
Attachment #8335206 - Flags: approval-mozilla-aurora?
Attachment #8335206 - Flags: approval-mozilla-beta?
Attachment #8335206 - Flags: approval-mozilla-beta+
Attachment #8335206 - Flags: approval-mozilla-aurora?
Attachment #8335206 - Flags: approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.