807575 - Only register activities when needed at startup

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: patch

We register activities at every startup, but we only need to do that when we are running after a system update.

This patch also adds a manifest cache that saves a lot of time in the parent for calls like mgmt.getAll().

Comment 1

[[:fabrice] Fabrice Desré]

Gene, one thing missing in this patch is the invalidation of the manifest cache when we are reinstalling an app or doing an app update.

Comment 2

[Gene Lian (I already quit Mozilla)]

Comment on attachment 677297 [details] [diff] [review]
patch

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

::: dom/apps/src/Webapps.jsm
@@ +138,5 @@
>      this._saveApps();
>    },
>  
>    // Registers all the activities and system messages.
> +  registerAppsHandlers: function registerAppsHandlers(aIsUpdate) {

If you don't mind, let's s/aIsUpdate/aRunUpdate for here and all the following codes, because what we want to do is to execute an extra process: *run* database *update* for apps' registries. |aRunUpdate| sounds more descriptive to me.

@@ +291,5 @@
>            this.updatePermissionsForApp(id);
>            this.updateOfflineCacheForApp(id);
>          }
>        }
> +      this.registerAppsHandlers(runUpdate);

And you can see here |runUpdate| can be consistent with your |aRunUpdate|. ;)

@@ +351,5 @@
>    },
>  
>    // |aEntryPoint| is either the entry_point name or the null in which case we
>    // use the root of the manifest.
> +  _createActivitiesToRegister: function(aManifest, aApp, aEntryPoint, aIsUpdate) {

s/aIsUpdate/aRunUpdate within this function.

@@ +387,5 @@
>    },
>  
>    // |aAppsToRegister| contains an array of apps to be registered, where
>    // each element is an object in the format of {manifest: foo, app: bar}.
> +  _registerActivitiesForApps: function(aAppsToRegister, aIsUpdate) {

s/aIsUpdate/aRunUpdate within this function.

@@ +411,5 @@
> +    if (aIsUpdate) {
> +      this.notifyAppsRegistryReady();
> +    } else {
> +      cpmm.sendAsyncMessage("Activities:Register", activitiesToRegister);
> +    }

Wait! Isn't it |!aIsUpdate|? We only need to register activities for first run. Right?

I think we can add an extra check for |activitiesToRegister.length == 0| because we would have chances that we don't have any activities to register. Therefore, we don't need to spend time sending async message for empty array. Also, use an early-return if-block here if you don't mind.

if (!aRunUpdate || activitiesToRegister.length == 0) {
  this.notifyAppsRegistryReady();
  return;
}

cpmm.sendAsyncMessage("Activities:Register", activitiesToRegister);

@@ +417,5 @@
>  
>    // Better to directly use |_registerActivitiesForApps()| if we have
>    // multiple apps to be registered for activities.
> +  _registerActivities: function(aManifest, aApp, aIsUpdate) {
> +    this._registerActivitiesForApps([{ manifest: aManifest, app: aApp }], aIsUpdate);

s/aIsUpdate/aRunUpdate within this function.

@@ +472,5 @@
>    _unregisterActivities: function(aManifest, aApp) {
>      this._unregisterActivitiesForApps([{ manifest: aManifest, app: aApp }]);
>    },
>  
> +  _processManifestForIds: function(aIds, aIsUpdate) {

s/aIsUpdate/aRunUpdate within this function.

Comment 4

[Gene Lian (I already quit Mozilla)]

Fabrice's solution can save 90% time for registering activities during not-first starting up! Awesome! According to the previous statics at [1], around 5s can be improved, which is very significant. Suggest bb+.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=799161#c2

Comment 5

[Andrew Overholt [:overholt]]

If we can improve startup time, let's do it.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Updated patch to address comments, and adding manifest cache cleanup when needed.

I ran stop watch timings and to my surprise we don't gain anything with this patch... I deactivated the lock screen and timed from a |adb shell stop b2g && adb shell start b2g| to the point where the dock icons appear. Absolutely no change... Gene, can you also do some timings?

Comment 7

[Gene Lian (I already quit Mozilla)]

Let me try and come back with some numbers. :)

Comment 8

[Gene Lian (I already quit Mozilla)]

Hi Fabrice,

I'm wondering if you didn't actually enter the first-run mode (or not)? Well, I don't know what the exact way is to simulate the first-run start-up, so I just hard-coded |aRunUpdate = true/false| to compare the performances.

1. I tried to diff the time between _processManifestForIds() and notifyAppsRegistryReady(). The numbers show:

  aRunUpdate = true:  7.707(s)
  aRunUpdate = false: 4.643(s)
  ----------------------------
  Improvement:        3.064(s)

  PS. No matter aRunUpdate is true or false, the manifest cache is included.

2. To test the effect of manifest cache, I tried to diff the time between DOMApplicationRegistry::init() and launchHomescreen() (in window_manager.js). The numbers show:

  No manifest cache:  10.361(s)
  Has manifest cache:  6.782(s)
  -----------------------------
  Improvement:         3.579(s)

  PS. No matter if manifest cache is considered or not, the aRunUpdate is false.

3. In summary (time diff between DOMApplicationRegistry::init() and launchHomescreen()):

  No manifest cache  & aRunUpdate = true:  11.017(s)
  Had manifest cache & aRunUpdate = false:  6.782(s)
  --------------------------------------------------
  Improvement:                              4.235(s)

As a result, we can have about 4.X seconds improvement by applying the patch. ;)

Comment 9

[Gene Lian (I already quit Mozilla)]

Comment on attachment 677577 [details] [diff] [review]
patch v2

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

In general, some flows here are beyond my knowledge but I doubt some places need to do the same trick. Need your confirmation. If you're fine then I'm fine. :)

1. confirmInstall(): the function is aimed for checking the installing status. Right? If yes, we don't need delete the manifest cache for it. Need your double check here.

2. updatePermissionsForApp()/updateOfflineCacheForApp(): it seems we need to update something for the app. Does it also change the manifest (especially the former one)?

3. updateApps()/wipe(): this functions would check .removable so I'm wondering the cached manifest needs to be deleted for the removed app.

::: dom/apps/src/Webapps.jsm
@@ +406,4 @@
>        }
>      }, this);
>  
>      // Send the array carrying all the activities to be registered.

Move this comment to below (before cpmm.sendAsyncMessage(...)).

@@ +826,5 @@
>        return;
>      }
>  
>      let id = this._appIdForManifestURL(app.manifestURL);
>  

Let's add a comment for this:

// Clean up the deprecated manifest cache.

@@ +924,5 @@
>  
>      function updateHostedApp(aManifest) {
>        debug("updateHostedApp");
>        let id = this._appId(app.origin);
>  

Add a comment:

// Clean up the deprecated manifest cache.

@@ +1254,5 @@
>      }
>  
>      let index = aIndex || 0;
>      let id = aData[index].id;
>  

Add a comment:

// Use the cached manifest instead of redundantly reading the file.

@@ +1431,5 @@
>        }
>  
>        if (!this.webapps[id].removable)
>          return;
>  

Add a comment:

// Clean up the deprecated manifest cache.

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Gene Lian [:gene] from comment #9)

> In general, some flows here are beyond my knowledge but I doubt some places
> need to do the same trick. Need your confirmation. If you're fine then I'm
> fine. :)
> 
> 1. confirmInstall(): the function is aimed for checking the installing
> status. Right? If yes, we don't need delete the manifest cache for it. Need
> your double check here.

When we re-install an application we need to clear the manifest cache, and this is taking this code path.

> 2. updatePermissionsForApp()/updateOfflineCacheForApp(): it seems we need to
> update something for the app. Does it also change the manifest (especially
> the former one)?

No, no manifest change here.

> 3. updateApps()/wipe(): this functions would check .removable so I'm
> wondering the cached manifest needs to be deleted for the removed app.


Yes, good point! I tend to forgot about this unused sync code.

> ::: dom/apps/src/Webapps.jsm
 
I'll add all these comments.

About the timings I find the same results as you do for your 1) and 2), but they don't lead to a start time improvement from starting the phone to having the dock populated with icons (with the lock screen disabled) for me...

Comment 11

[[:fabrice] Fabrice Desré]

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

Comment 12

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/a07b751f25a0

Comment 13

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d6d8e68c973