849103 - IonMonkey: Completely enable IonMonkey on B2G

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: Enable IonMonkey on B2G

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 1

[Sean Stangl [:sstangl]]

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.

Comment 2

[Nicolas B. Pierron [:nbp]]

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

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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

Comment 4

[Marty Rosenberg [:mjrosenb]]

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.

Comment 5

[David Mandelin [:dmandelin]]

(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.

Comment 6

[cmtalbert]

(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).

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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...

Comment 8

[Nicolas B. Pierron [:nbp]]

(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?

Comment 9

[Nicolas B. Pierron [:nbp]]

(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

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Remove redundant code used to disable IonMonkey.

[Redirect to gonk-misc pull request]

Comment 11

[John Ford [:jhford] CET/CEST Berlin Time]

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 12

[Alex Keybl [:akeybl]]

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.

Comment 13

[Nicolas B. Pierron [:nbp]]

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 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 15

[Guillaume Destuynder [:kang] [this account is no longer active]]

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 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 722636 [details] [diff] [review]
Enable IonMonkey on B2G

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

Let's do it!

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 724065 [details]
Remove redundant code used to disable IonMonkey.

https://github.com/mozilla-b2g/gonk-misc/commit/9429d142f7f46add49f4665ce81ac0c0416dd9c1

Comment 18

[Nicolas B. Pierron [:nbp]]

Comment on attachment 722636 [details] [diff] [review]
Enable IonMonkey on B2G

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2f7f9a1dc7

Comment 19

[Nicolas B. Pierron [:nbp]]

Attachment: Oops … Enable IonMonkey on tbpl for B2G buils

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 20

[Nicolas B. Pierron [:nbp]]

Comment on attachment 727394 [details] [diff] [review]
Oops … Enable IonMonkey on tbpl for B2G buils

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2cb4c8c2b58

Comment 21

[Nicolas B. Pierron [:nbp]]

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.

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

(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.