837816 - GeckoAppShell.loadMozGlue gets called three times on startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Once from GeckoAppShell.loadSQLiteLibs, once from GeckoAppShell.loadNSSLibs, and once from GeckoApp.onCreate, just for good measure. (Not necessarily in that order)

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Also, I just noticed the synchronization blocks in loadNSSLibs and loadSQLiteLibs are pretty bad. The sSQLiteLibsLoaded and sNSSLibsLoaded objects are booleans, and specifically Boolean.TRUE and Boolean.FALSE because of autoboxing. That means any other code accidentally synchronizing on those objects will block these functions (I verified this by inserting this code in GeckoApp.create:

        new Thread() {
            public void run() {
                synchronized (Boolean.FALSE) {
                    try {
                        Thread.sleep(60 * 1000);
                    } catch (Exception e) {
                    }
                }
            }
        }.start();

and noticed that it blocked gecko startup for a minute, as expected). Additionally the object being synchronized on is reassigned within the synchronization block, which is also generally bad practice and can defeat the synchronization block in subtle ways on platforms where assignments are not atomic.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Working on a fix.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This does a bunch of cleanup with the bool/locking to fix the issues I identified above. It also adds a new bool to guard against loading mozglue multiple times unnecessarily.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=ce81e1d1de54 - I expect this to show a startup time improvement as well.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 709908 [details] [diff] [review]
Patch

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

LGTM

Comment 5

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/755d7faad3c0

Comment 6

[Geoff Brown [:gbrown]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Try run at https://tbpl.mozilla.org/?tree=Try&rev=ce81e1d1de54 - I expect
> this to show a startup time improvement as well.

The ts startup times from this try push look a little worse (compared to m-i).

Comment 7

[Kartikaya Gupta (email:[e-mail])]

I looked at the recent inbound ts numbers before and after my change. Looks like it got a bit worse on 2.2 and a bit better on 4.0 but on both it's well within the margin of error. I'd call it insigificant.

               | Before my change | After my change
---------------+------------------+-----------------
Android 2.2 ts | Samples: 16      | Samples: 21
               | Average: 4163.5  | Average: 4580.86
               | Stddev: 1285.51  | Stddev: 1257.27
               | Max: 5564        | Max: 5708
               | Min: 3506        | Min: 3518
---------------+------------------+-----------------
Android 4.0 ts | Samples: 17      | Samples: 21
               | Average: 3881.53 | Average: 3866.29
               | Stddev: 944.467  | Stddev: 846.491
               | Max: 4047        | Max: 4016
               | Min: 3738        | Min: 3703

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/755d7faad3c0