IonMonkey is mature now, I don't see any reasons why we should keep it disabled on B2G. https://tbpl.mozilla.org/?tree=Try&rev=6053b9a29dcb

Comment on attachment 722636 [details] [diff] [review] Enable IonMonkey on B2G Review of attachment 722636 [details] [diff] [review]: ----------------------------------------------------------------- Marty is a more qualified reviewer for this patch.

Is there a a way to check the try push made previously with what is available at https://datazilla.mozilla.org/b2g ?

If we do this, can we also turn off the Android NoIon builds for the affected trees?

Actually, can't we do that right now, since we now have b2g tests? I seem to remember that they were a temp solution until we got testing b2g devices working properly.

(In reply to Marty Rosenberg [:mjrosenb] from comment #4) > Actually, can't we do that right now, since we now have b2g tests? > I seem to remember that they were a temp solution until we got testing b2g > devices working properly. In TBPL, I still see J1-J3 being run only on Android builds, not B2G, so AFAIK that's the closest thing we have to JS engine tests for B2G. Moving them to be run on B2G and turning off the NoIon builds would be great, though.

(In reply to Nicolas B. Pierron [:nbp] from comment #2) > Is there a a way to check the try push made previously with what is > available at https://datazilla.mozilla.org/b2g ? There is, you'd need a unagi though. Do you have one? If so, we'd download the unagi build generated from the try push, flash it to your unagi and then we can set up the simple python script that runs those tests and you can run it pretty easily via command line and compare the numbers you get with what datazilla is reporting. So, it's not easy, but it's also not all that hard. If it's something you'd like to do, I'd be happy to help you with it. I don't have any spare unagis to run it on at the moment (all the ones I can find are being used by the automation powering the stuff you see on datazilla).

Just FYI, the default mozconfig used on B2G has an explicit --disable-ion in it: https://github.com/mozilla-b2g/gonk-misc/blob/master/default-gecko-config#L59 So I'm not sure this change will make much difference in our actual builds...

(In reply to ben turner [:bent] from comment #7) > Just FYI, the default mozconfig used on B2G has an explicit --disable-ion in > it: > > https://github.com/mozilla-b2g/gonk-misc/blob/master/default-gecko-config#L59 > > So I'm not sure this change will make much difference in our actual builds... Thanks, I already commented the line in my clone. I just forgot to extract the patch, otherwise I would have had a huge unexplained variance in benchmarks. For your information, I don't know why this modification was needed on the 26 of September knowing that it was disabled in gecko before landing IonMonkey. jhford: Why was this change needed? I cannot find the Bug number related to this change?

(In reply to Nicolas B. Pierron [:nbp] from comment #8) > jhford: Why was this change needed? I cannot find the Bug number related to > this change? Ok, after discussing with john over IRC, the "regression" documented[1] in the code refers to potential regressions which might have come with IonMonkey. And the patch was just to be extra careful. I think we can safely remove this option right now from gonk-misc, as the js/src/configure.in check in gecko is already doing the job by checking against b2g. [1] https://github.com/mozilla-b2g/gonk-misc/commit/ad0a4a7c54636f345adcc23dbec3ddf56a30dedc

[Redirect to gonk-misc pull request]

Comment on attachment 724065 [details] Remove redundant code used to disable IonMonkey. This looks OK to me, and I can confirm that there is a section in configure.in that should still disable ionmonkey. In js/src/configure.in: # Disable IonMonkey for B2G, for now. if test "$MOZ_APP_NAME" = b2g; then ENABLE_ION= fi This will require another snapshot rebuild for the master branch. Let's wait on the outcome of this bug before we land this patch though. No reason to remove this line if we aren't enabling IonMonkey.

Comment on attachment 722636 [details] [diff] [review] Enable IonMonkey on B2G We'll defer to Jonas on this review - there have been complaints about code divergence between master/v1-train, so we may want to hold off on this change.

Ok, I made additional measures to check that IonMonkey does not regress anything. For running benchmarks, I disabled the interrupt-check by setting: user_pref("dom.max_script_run_time", 0); 1/ SunSpider results (in the browser) are about 4.1s and the results evolves under the noise level of 2%, with both JM / Ion. [1] 2/ Octane results are about the same with JM and Ion [1] when tests are ran individually. Mandreel does not run because it runs out-of-memory in both cases. Both raytrace & gb-emu are triggering the interrupt-check message. When running them in batches, while disabling all benchmarks after (and including) Pdf.js, The Ion score is around 300, and JM score is around 250, mostly because of Richards & NavierStokes scores. Ion seems to regress DeltaBlue (from 265 with JM to 165 with Ion), but I bet this might also be related to the scheduling of GC cycles. Also Ion is running out-of-memory when pdfjs benchmark is loaded while JM is able to run it. These variations are likely caused by octane benchmark harness which does not enforce a GC between benchmarks, and do multiple runs of benchmarks if they are running faster. 3/ Start-up memory does not seems to be affected by enabling IonMonkey. I checked the memory usage by running: $ adb shell reboot; sleep 60; ./tools/get_about_memory -om JM Ion RSS / DMD RSS / DMD b2g 57.2M / 23.1M 55.6M / 23.0M Homescreen 28.9M / 8.5M 29.2M / 8.6M Prealloc 20.2M / 4.2M 20.5M / 4.2M RSS: Resident Memory DMD: DMD enabled build, total memory allocated (unreported & reported) 4/ Start-up time does not seems to be affected by enabling IonMonkey. I checked the startup time by running: $ ./profile.sh start; sleep 20; ./profile.sh capture JM Ion Homescreen 4555 ms 4583 ms This is measured from the beginning of the profile until first wait on processNextNative after the first paint. The b2g profiles is not comparable because one handle the crash case where the other doesn't, but they are in the same order of magnitude (JM 13.0s, Ion 13.5s) [1] Results were so identical, that I thought for a while that Ion was not running in the B2G browser, but adding a SEGV inside EnterIon (js/src/Ion.cpp) show that the function was at least called once while running Crypto benchmark. There might be an issue which is holding Ion back.

Comment on attachment 722636 [details] [diff] [review] Enable IonMonkey on B2G Review of attachment 722636 [details] [diff] [review]: ----------------------------------------------------------------- Guillaume: Any concers security-wise with enabling the Jit for B2G child processes?

Comment on attachment 722636 [details] [diff] [review] Enable IonMonkey on B2G I currently have no direct concerns against having IM enabled for content processes. Eventually, strong sandboxing should be used in order to prevent resource access and privilege escalation from the content processes when a JIT bug is exploited (see bug 790923 for example) Also, FYI, the fuzzing on ARM is currently stalled due to bug 814552. Hopefully this is also resolved soon.

Comment on attachment 722636 [details] [diff] [review] Enable IonMonkey on B2G Review of attachment 722636 [details] [diff] [review]: ----------------------------------------------------------------- Let's do it!

Comment on attachment 724065 [details] Remove redundant code used to disable IonMonkey. https://github.com/mozilla-b2g/gonk-misc/commit/9429d142f7f46add49f4665ce81ac0c0416dd9c1

Comment on attachment 722636 [details] [diff] [review] Enable IonMonkey on B2G https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2f7f9a1dc7

https://tbpl.mozilla.org/?tree=Try&rev=60997b7f7859&showall=1 If this try build fails for B2G compared to https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0f2f7f9a1dc7&showall=1 both patches should be reverted as this will cause errors in local builds which do not appear on tbpl. Thanks RyanVM for noticing this.

Comment on attachment 727394 [details] [diff] [review] Oops … Enable IonMonkey on tbpl for B2G buils https://hg.mozilla.org/integration/mozilla-inbound/rev/e2cb4c8c2b58

Ok, checking the compilations logs, I can confirm that both "B2G ARM opt/dbg" are compiling with IonMonkey enabled. (search for IonBuilder.cpp) https://tbpl.mozilla.org/php/getParsedLog.php?id=20907351&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=20908055&tree=Mozilla-Inbound On the other hand, images made for the Panda board and the Unagi have not yet taken into account the changes reported in comment 17. I guess we probably need to update the submodule within the B2G repository.

https://hg.mozilla.org/mozilla-central/rev/0f2f7f9a1dc7 https://hg.mozilla.org/mozilla-central/rev/e2cb4c8c2b58

(In reply to Nicolas B. Pierron [:nbp] from comment #21) > Ok, checking the compilations logs, I can confirm that both "B2G ARM > opt/dbg" are compiling with IonMonkey enabled. (search for IonBuilder.cpp) > > https://tbpl.mozilla.org/php/getParsedLog.php?id=20907351&tree=Mozilla- > Inbound > https://tbpl.mozilla.org/php/getParsedLog.php?id=20908055&tree=Mozilla- > Inbound > > On the other hand, images made for the Panda board and the Unagi have not > yet taken into account the changes reported in comment 17. I guess we > probably need to update the submodule within the B2G repository. Yep, we need to generate a new master branch snapshot. I'll file that bug now.