Closed Bug 868184 Opened 11 years ago Closed 11 years ago

OdinMonkey: enable by default on beta/release


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox22 + verified
firefox23 + verified
relnote-firefox --- 22+


(Reporter: luke, Assigned: luke)




(1 file)

Attached patch patchSplinter Review
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.,  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)
Comment on attachment 744822 [details] [diff] [review]

[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?
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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
(In reply to from comment #4)
Automated tests are already in the tree (js/src/jit-tests/tests/asm.js).
Flags: needinfo?(luke)
Noted in FF22 as "asm.js optimizations (<a href="">OdinMonkey</a>) enabled for major performance improvements"

Please email me if you'd like to help in reworking that note.
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

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.
(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
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.