805293 - Make convertAppsArray faster

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: patch

Various little improvement, but the bulk of the win comes form using a getter to provide lazy loading of the apps in the array.

Comment 2

[Philipp von Weitershausen [:philikon]]

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

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

::: dom/apps/src/Webapps.js
@@ +20,5 @@
>  
>  function convertAppsArray(aApps, aWindow) {
> +  let util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDOMWindowUtils);
> +  let wId = util.currentInnerWindowID;

Don't all callers of this function already know their inner window ID? Yeah, you added it to WebappsRegistry. Which also creates WebappsApplicationMgmt. Both of those are the consumers of convertAppsArray(). So we can totally pass this ID as a parameter here.

@@ +27,5 @@
>      let app = aApps[i];
> +    // Use a getter to get lazy loading.
> +    apps.__defineGetter__(i, function() {
> +      return createApplicationObject(aWindow, app, wId);
> +    });

Hmm, so this makes convertAppsArray() essentially free, but consumers of this function will still naively iterate over the entire array in one gulp. We've now shifted the 700ms from chrome to Gaia code.

I think I would prefer we'd create the objects right away, but yield to the event loop after every item or so. That way content code can't lock up the main thread by doing something very normal (looping). I'm thinking something like this:

  function convertAppsArray(aApps, aWindow, aWindowId, aRequest) {
    let apps = Cu.createArrayIn(aWindow);
    function next() {
      let app = aApps[apps.length];
      if (!app) {
        Services.DOMRequest.fireSuccess(aRequest, apps);
        return;
      }
      apps.push(createApplicationObject(aWindow, app, aWindowId));
      Services.tm.currentThread.dispatch(next);
    }
    next();
  }


(Also, on a side note, I wonder if binding the parameters instead of a closure, e.g.

  createApplicationObject.bind(null, aWindow, app, wId);

might be more efficient instead of the closure.)

@@ +270,5 @@
>                                "Webapps:CheckInstalled:Return:OK" ]);
>  
>      let util = this._window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>      this._id = util.outerWindowID;
> +    this._innerId = util.currentInnerWindowID;

Good idea! Now let's also use `this._innerId` a few lines above in the call to `this.initHelper` (convert to a call to `this.initHelperWithWindowId`) and pass it to WebappsApplicationMgmt in the `mgmt` getter.

@@ +359,5 @@
> +function createApplicationObject(aWindow, aApp, aWindowId) {
> +  let manifestURL = aApp.manifestURL;
> +  if (manifestURL in gAppsPool) {
> +    return gAppsPool[manifestURL];
> +  }

Love the memoization. Let's also make sure we don't leak these:

  Services.obs.addObserver(function cleanUpAppsPool() {
    gAppsPool = null;
  }, "xpcom-shutdown", false);

(This goes at the top level of course.)

@@ +407,5 @@
> +                                 "Webapps:Uninstall:Return:KO",
> +                                 "Webapps:OfflineCache",
> +                                 "Webapps:CheckForUpdate:Return:OK",
> +                                 "Webapps:CheckForUpdate:Return:KO",
> +                                 "Webapps:PackageEvent"], aWindowId);

Nit: `aWindowId` on its own line for better readability (I stumbled over this at first).

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.