OdinMonkey: enable by default on beta/release

VERIFIED FIXED in Firefox 22

Status

()

Core
JavaScript Engine
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(firefox22+ verified, firefox23+ verified, relnote-firefox 22+)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Created attachment 744822 [details] [diff] [review]
patch

At this point, we expect the asm.js subset to only grow over time.  Bug fallout has also been minor and well in line with regular JS engine development.  Thus, there isn't a good reason to leave OdinMonkey turned off in Beta/Release and there are several reasons to turn it on by default (viz., http://www.unrealengine.com/html5).  Lastly, the patch removes the "experimental_" prefix of "javascript.options.experimental_asmjs" since the pref is now on by default.
Attachment #744822 - Flags: review?(brendan)
Attachment #744822 - Flags: review?(brendan) → review+
(Assignee)

Comment 1

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ac16cbea47
(Assignee)

Comment 2

4 years ago
Comment on attachment 744822 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 840282
User impact if declined: asm.js optimizations won't be enabled on beta/release
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
Attachment #744822 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d4ac16cbea47
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 840282
status-firefox22: --- → affected
status-firefox23: --- → fixed
tracking-firefox22: --- → +
tracking-firefox23: --- → +
relnote-firefox: --- → ?
Attachment #744822 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Can you provide tips on a test plan QA can use & add to automated tests in order to keep on top of potential regressions from enabling this new feature?
Flags: needinfo?(luke)
Keywords: verifyme
(Assignee)

Comment 5

4 years ago
(In reply to lsblakk@mozilla.com from comment #4)
Automated tests are already in the tree (js/src/jit-tests/tests/asm.js).
Flags: needinfo?(luke)
https://hg.mozilla.org/releases/mozilla-aurora/rev/957075eb2de1
status-firefox22: affected → fixed

Comment 7

4 years ago
Noted in FF22 as "asm.js optimizations (<a href="https://blog.mozilla.org/luke/2013/03/21/asm-js-in-firefox-nightly/">OdinMonkey</a>) enabled for major performance improvements"

Please email me if you'd like to help in reworking that note.
relnote-firefox: ? → 22+
Is there any need for manual testing on this (I see in comment 5 that automated test are already in the tree)?

I can confirm that the preference javascript.options.asmjs is enabled by default on Firefox 23 beta 2. But Firefox closes and I get the following message in the terminal "Aborted (core dumped)" when loading http://www.unrealengine.com/html5.

Does anyone have any idea why this is happening?
Can you file a separate bug on that, and include OS and other support info?
Note that the asm.js syntactic rules have been changing over the last several weeks in response to various bug reports and such, so it's not necessarily the case that something that passes as asm.js now (or at first landing) passed (or will pass now) as asm.js.  I do believe some care has been taken to keep the Unreal demo working, but at some point it wouldn't surprise me if some asm.js change means that that demo has to be regenerated with a newer version of Emscripten to preserve its performance gains.

Anyway, just a warning that it's not *necessarily* the case that an observed performance regression on a page that uses asm.js is a definite bug.
(Assignee)

Comment 11

4 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
Indeed, care was taken; Unreal, BananaBread, and all the other demos should work on all release channels of Firefox atm.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> Can you file a separate bug on that, and include OS and other support info?

Filed bug 889802.
Verified that the pref is enabled by default on all OS's on FF 22 and FF 23b4. As long Simona has filed a new bug for that crash on Ubuntu, I'm marking this bug as Verified
Status: RESOLVED → VERIFIED
status-firefox22: fixed → verified
status-firefox23: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.