Closed Bug 927408 Opened 6 years ago Closed 6 years ago

Disable Ion float32 optimization for now

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

We should disable this on Aurora and probably Nightly as well. Until bug 913282 is fixed, float32 can regress performance for things like Math.abs(float32array[x]), because the abs call is no longer inlined.

See the following benchmarks (from bug 924856) for instance:

http://opera-mage.github.io/webarraymath/benchmark/
Attached patch Patch for Aurora (obsolete) — Splinter Review
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #821800 - Flags: review?(benj)
Attached patch Patch for AuroraSplinter Review
I will do a Try push before landing this.
Attachment #821800 - Attachment is obsolete: true
Attachment #821800 - Flags: review?(benj)
Attachment #821831 - Flags: review?(benj)
Comment on attachment 821831 [details] [diff] [review]
Patch for Aurora

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

Sooo, I lost my bugzilla rights when changing email address and I can't even r+ this patch, so this message can be considered as a voucher for a r+.

::: js/src/jit/x86/CodeGenerator-x86.cpp
@@ +594,5 @@
>  
>      Address dstAddr(ptr, (int32_t) mir->base());
>      if (vt == ArrayBufferView::TYPE_FLOAT32) {
> +        JS_ASSERT(mir->value()->type() == MIRType_Double);
> +        masm.cvtsd2ss(ToFloatRegister(value), ScratchFloatReg);

Could you add a reference to this bug here too, please?
You have your bits back now.  (And, uh, really, the de-MoCo-ification bit strips those?  That's kind of really surprising.)
Comment on attachment 821831 [details] [diff] [review]
Patch for Aurora

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

Sooo, I lost my bugzilla rights when changing email address and I can't even r+ this patch, so this message can be considered as a voucher for a r+.
Attachment #821831 - Flags: review?(benj) → review+
AFAIK, the intention was to have float32 enabled on Nightly, but disabled for Beta through Release. Turning float32 off entirely will hurt, since fuzzers will no longer exercise any of the float32-related code.

Could we instead hide float32 support behind one of those preferences that automatically switch to false for Beta/Release builds?
(In reply to Sean Stangl [:sstangl] from comment #6)
> AFAIK, the intention was to have float32 enabled on Nightly, but disabled
> for Beta through Release. Turning float32 off entirely will hurt, since
> fuzzers will no longer exercise any of the float32-related code.

This patch will only disable it for Aurora (26). I think with bug 913282 and bug 930477 we can leave it turned on for 27.
Comment on attachment 821831 [details] [diff] [review]
Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 888109
User impact if declined: Performance regressions
Testing completed (on m-c, etc.): Green on Try [0]
Risk to taking this patch (and alternatives if risky): Low, disables a new optimization.
String or IDL/UUID changes made by this patch: None.

[0] https://tbpl.mozilla.org/?tree=Try&rev=b2d64fa5d225
Attachment #821831 - Flags: approval-mozilla-aurora?
Attachment #821831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I assume this bug can be resolved fixed because the patch landed on Aurora (26) and bug 913282 was fixed in Nightly 27.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Depends on: 913282
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.