836605 - AppsService.getAppBy*() is way more expensive than callers expect

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We have several consumers of the appssservice that do things like

GetApp() {
  return AppsService.getAppBy*();
}

if (GetApp()) {
  ...
}

or otherwise assume getting an app is cheap.  It's not!!!  We clone a pretty large JS object and inject a new closure into it for each call.  In addition to the C++->JS call overhead (in the callers I'm looking at).

We should either optimize the get*() helpers or fix callers to cache more aggressively.

I don't have an exact count of the amount of time spent in these helpers because it's a bit hard to count, but it's at least 30-40 samples, and likely more.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

fabrice, what would you prefer to do here?  I'm not sure we can cache in the AppsService without breaking the API contract.  Maybe we should assume all callers treat the get*() result as immutable?

Comment 2

[Gene Lian (I already quit Mozilla)]

Currently, all the getAppBy*() are using linear search to match the input. We might construct some extra mapping tables in advance to speed up the searching performance.

I *guess* the cloning part is inevitable because each web content should have an independent copy of that for their own purposes.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

fabrice already fixed getAppByLocalId() (bug 835596), and it helped, but those samples are still showing up significantly in profiles.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Notably, cloneApp() is showing up significantly.

Comment 5

[[:fabrice] Fabrice Desré]

We either query by localId or by manifestURL, so we can add a manifestURL based index, and of course fixing the callers can not hurt. For cloneApp, I have a wip patch that may give us better perf.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: wip patch

Let's try using a prototype, hoping that the js engine magically does something smart.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> We either query by localId or by manifestURL, so we can add a manifestURL
> based index, and of course fixing the callers can not hurt.

I'm pretty sure that on the path I'm looking at, we can keep the same cached object all the way from IFrameElement down to TabParent(TabContext).  I'll try your improvement here and look into that in parallel.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Created attachment 708467 [details] [diff] [review]
> wip patch
> 
> Let's try using a prototype, hoping that the js engine magically does
> something smart.

This looks like it might be a 20 sample or more win, but the data I'm seeing with this patch applied are really noisy.  Digging a bit more.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 708467 [details] [diff] [review]
wip patch

Yeah, after a few more runs, this looks like it should be winning 15-20 samples.  Definitely cheaper.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> Comment on attachment 708467 [details] [diff] [review]
> wip patch
> 
> winning 15-20 samples

*ms.  (Samples were too noisy here, had to recalibrate.)

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(FTR, I'm working on fixing the callers here to cache their immutable mozIApplications.)

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Initial results indicate fixing callers is a big win.  Final patch tomorrow.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Cache mozIApplication wherever possible on critical startup path

WIP.  This is making lots of stuff go away on profiles, but I need to do some more measuring of real win.  The patch itself is pretty trivial but touches some scary code.

This is 100% complementary to fabrice's patch.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Samples from the profiler seem to be getting noisier, but this looks like a 35-40ms win.  (Testing without fabrice's patch applied.)  Fabrice's patch will cut that down, but not by the entire amount in comment 10 since we're calling into AppsService less.

So overall we should be around 40-50ms win from the two patches.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Created attachment 708467 [details] [diff] [review]
> wip patch
> 
> Let's try using a prototype, hoping that the js engine magically does
> something smart.

Any remaining work needed here before requesting review?

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> 
> Any remaining work needed here before requesting review?

No, since it does the good things we want.

Comment 17

[Gene Lian (I already quit Mozilla)]

Comment on attachment 708467 [details] [diff] [review]
wip patch

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

Cool stuff! Interesting!

::: dom/apps/src/AppsUtils.jsm
@@ +37,5 @@
> +}
> +
> +mozIApplication.prototype = {
> +  hasPermission: function(aPermission) {
> +      let uri = Services.io.newURI(this.origin, null, null);

NIT: the alignments from here (and below) look not comfortable. Would you please correct them with 2-space leftward? Thanks!

@@ +54,5 @@
> +    QueryInterface: function(aIID) {
> +      if (aIID.equals(Ci.mozIDOMApplication) ||
> +          aIID.equals(Ci.mozIApplication) ||
> +          aIID.equals(Ci.nsISupports))
> +        return this;

NIT: since you're here, could you please add a { } for this?

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 708828 [details] [diff] [review]
Cache mozIApplication wherever possible on critical startup path

What happens if we've cached an app's mozIApplication object and then the app gets uninstalled?

>diff --git a/dom/interfaces/html/nsIMozBrowserFrame.idl b/dom/interfaces/html/nsIMozBrowserFrame.idl
>index 973202d..ed24aeb 100644
>--- a/dom/interfaces/html/nsIMozBrowserFrame.idl
>+++ b/dom/interfaces/html/nsIMozBrowserFrame.idl

>@@ -1,47 +1,45 @@
> [scriptable, builtinclass, uuid(929AED00-3E15-49B7-8CA2-75003715B7E7)]

rev uuid

>   /**
>-   * Gets whether this frame really is an app frame.
>-   *
>-   * In order to really be an app frame, this frame must really be a browser
>-   * frame (this requirement will go away eventually), and the frame's mozapp
>-   * attribute must point to the manifest of a valid app.
>+   * Get this frame's app , if the frame really is an app frame.
>+   * Otherwise, return null.

" , "

>    */
>-  [infallible] readonly attribute boolean reallyIsApp;
>+  readonly attribute mozIApplication app;

Can we call this "ownApp"?  A principal's "app" is its own-or-containing-app,
so this is ambiguous.

>   /**
>-   * Gets this frame's app manifest URL, if the frame really is an app frame.
>-   * Otherwise, returns the empty string.
>-   *
>-   * This method is guaranteed not to fail.
>+   * Get the app that contains this frame, which might be |app|.  This
>+   * is the app that our security principal considers this frame to be
>+   * running as.
>    */
>-  readonly attribute AString appManifestURL;
>+  readonly attribute mozIApplication containingApp;

The comment describes the own-or-containing app, not the containing app.  It
looks like your implementation returns the containing app.

I'm pretty sure containingApp == ownApp iff an app embeds itself.

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
>index 194f019..372004a 100644
>--- a/content/base/src/nsFrameLoader.h
>+++ b/content/base/src/nsFrameLoader.h

>@@ -329,22 +329,16 @@ private:
>   /**
>    * Get our owning element's app manifest URL, or return the empty string if
>    * our owning element doesn't have an app manifest URL.
>    */
>-  void GetOwnerAppManifestURL(nsAString& aOut);
>-
>-  /**
>-   * Get the app for our frame.  This is the app whose manifest is returned by
>-   * GetOwnerAppManifestURL.
>-   */
>   already_AddRefed<mozIApplication> GetOwnApp();

Deleted the wrong comment?

>diff --git a/content/html/content/src/nsGenericHTMLFrameElement.h b/content/html/content/src/nsGenericHTMLFrameElement.h

>+  nsCOMPtr<mozIApplication> mApp;
>+  nsCOMPtr<mozIApplication> mContainingApp;

Please add a comment indicating that these are a cache, and explaining why the
cache exists.

>diff --git a/content/html/content/src/nsGenericHTMLFrameElement.cpp b/content/html/content/src/nsGenericHTMLFrameElement.cpp

>@@ -307,68 +308,115 @@ nsGenericHTMLFrameElement::GetReallyIsBrowserOrApp(bool *aOut)
>+nsresult
>+nsGenericHTMLFrameElement::CanPrincipalEmbedApps(bool* aCanEmbed)

I'd prefer "PrincipalCanEmbedApps", but actually this method does more than
just check the principal.  Perhaps "MayBeAppFrame", "MightBeAppFrame", or
"AppFrameSecurityCheck"?

Also, if you prefer, this could return a bool.

> {

*aCanEmbed = false;

>+NS_IMETHODIMP
>+nsGenericHTMLFrameElement::GetApp(mozIApplication** aApp)
>+{
>+  SAMPLE_LABEL("nsGenericHTMLFrameElement", "GetApp");

*aApp = nullptr;

>   if (app) {
>-    aOut.Assign(manifestURL);
>+    mApp = do_QueryInterface(app);
>   }

I'd get rid of the |if (app)|, so that the cache stays in line with what we
return.

>+  if (mApp) {
>+    nsCOMPtr<mozIApplication>(mApp).forget(aApp);
>+  }

You don't need this if statement, unless you think it clarifies things.

>+NS_IMETHODIMP
>+nsGenericHTMLFrameElement::GetContainingApp(mozIApplication** aApp)
>+{
>+  SAMPLE_LABEL("nsGenericHTMLFrameElement", "GetContainingApp");

*aApp = nullptr;

>diff --git a/dom/ipc/TabContext.h b/dom/ipc/TabContext.h
>index d1bdc8f..8b0f586 100644
>--- a/dom/ipc/TabContext.h
>+++ b/dom/ipc/TabContext.h

>+  nsCOMPtr<mozIApplication> mOwnApp;

Would you mind adding a comment indicating that this is a cache and describing
the cache's semantics?

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: faster cloneAsMozIApplication

Addressed comments and fixed the commit message, ready to land.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Cache mozIApplication wherever possible on critical startup path, v2

Still two outstanding issues, so re-r?.  (Missed you on IRC I guess.)

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Argh, I had written out a long response to your comment and didn't notice that I didn't paste it here.  Will recover as much as I can in a minute.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

M-x recover-lost-comment ...

(In reply to Justin Lebar [:jlebar] from comment #18)
> Comment on attachment 708828 [details] [diff] [review]
> Cache mozIApplication wherever possible on critical startup path
> 
> What happens if we've cached an app's mozIApplication object and then the
> app gets uninstalled?

What happens currently?

> >diff --git a/dom/interfaces/html/nsIMozBrowserFrame.idl b/dom/interfaces/html/nsIMozBrowserFrame.idl
> >index 973202d..ed24aeb 100644
> >--- a/dom/interfaces/html/nsIMozBrowserFrame.idl
> >+++ b/dom/interfaces/html/nsIMozBrowserFrame.idl
> 
> >@@ -1,47 +1,45 @@
> > [scriptable, builtinclass, uuid(929AED00-3E15-49B7-8CA2-75003715B7E7)]
> 
> rev uuid

Done.

> >   /**
> >-   * Gets whether this frame really is an app frame.
> >-   *
> >-   * In order to really be an app frame, this frame must really be a browser
> >-   * frame (this requirement will go away eventually), and the frame's mozapp
> >-   * attribute must point to the manifest of a valid app.
> >+   * Get this frame's app , if the frame really is an app frame.
> >+   * Otherwise, return null.
> 
> " , "

Fixed.

> 
> >    */
> >-  [infallible] readonly attribute boolean reallyIsApp;
> >+  readonly attribute mozIApplication app;
> 
> Can we call this "ownApp"?  A principal's "app" is its own-or-containing-app,
> so this is ambiguous.

Changed.

> >   /**
> >-   * Gets this frame's app manifest URL, if the frame really is an app frame.
> >-   * Otherwise, returns the empty string.
> >-   *
> >-   * This method is guaranteed not to fail.
> >+   * Get the app that contains this frame, which might be |app|.  This
> >+   * is the app that our security principal considers this frame to be
> >+   * running as.
> >    */
> >-  readonly attribute AString appManifestURL;
> >+  readonly attribute mozIApplication containingApp;
> 
> The comment describes the own-or-containing app, not the containing app.  It
> looks like your implementation returns the containing app.

I made the change, but this code is implementing the same algorithm
that nsFrameLoader::GetContainingApp() previously did (get the node
principal app ID and look that up).

So I went ahead and changed the nsFrameLoader interface name too.

> I'm pretty sure containingApp == ownApp iff an app embeds itself.

Yep, I believe that was our conclusion of NodePrincipal::GetAppId()
behavior.

> >diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
> >index 194f019..372004a 100644
> >--- a/content/base/src/nsFrameLoader.h
> >+++ b/content/base/src/nsFrameLoader.h
> 
> >@@ -329,22 +329,16 @@ private:
> >   /**
> >    * Get our owning element's app manifest URL, or return the empty string if
> >    * our owning element doesn't have an app manifest URL.
> >    */
> >-  void GetOwnerAppManifestURL(nsAString& aOut);
> >-
> >-  /**
> >-   * Get the app for our frame.  This is the app whose manifest is returned by
> >-   * GetOwnerAppManifestURL.
> >-   */
> >   already_AddRefed<mozIApplication> GetOwnApp();
> 
> Deleted the wrong comment?

D'oh, fixed.

> 
> >diff --git a/content/html/content/src/nsGenericHTMLFrameElement.h b/content/html/content/src/nsGenericHTMLFrameElement.h
> 
> >+  nsCOMPtr<mozIApplication> mApp;
> >+  nsCOMPtr<mozIApplication> mContainingApp;
> 
> Please add a comment indicating that these are a cache, and explaining why
> the
> cache exists.

Done.

> 
> >diff --git a/content/html/content/src/nsGenericHTMLFrameElement.cpp b/content/html/content/src/nsGenericHTMLFrameElement.cpp
> 
> >@@ -307,68 +308,115 @@ nsGenericHTMLFrameElement::GetReallyIsBrowserOrApp(bool *aOut)
> >+nsresult
> >+nsGenericHTMLFrameElement::CanPrincipalEmbedApps(bool* aCanEmbed)
> 
> I'd prefer "PrincipalCanEmbedApps", but actually this method does more than
> just check the principal.  Perhaps "MayBeAppFrame", "MightBeAppFrame", or
> "AppFrameSecurityCheck"?

I don't quite follow ... the code here asks the node principal whether
it has the "embed-apps" permission.  We skip that check if we're not
an app or browser frame since it can't pass.

I changed the name to AppFrameCanEmbedApps() to make the early bail if
no browser-or-app clearer.

> Also, if you prefer, this could return a bool.

Sure, the clients just eat the exception code anyway.


> >+NS_IMETHODIMP
> >+nsGenericHTMLFrameElement::GetApp(mozIApplication** aApp)
> >+{
> >+  SAMPLE_LABEL("nsGenericHTMLFrameElement", "GetApp");
> 
> *aApp = nullptr;
> 

Fixed.

> >   if (app) {
> >-    aOut.Assign(manifestURL);
> >+    mApp = do_QueryInterface(app);
> >   }
> 
> I'd get rid of the |if (app)|, so that the cache stays in line with what we
> return.
> 
> >+  if (mApp) {
> >+    nsCOMPtr<mozIApplication>(mApp).forget(aApp);
> >+  }
> 
> You don't need this if statement, unless you think it clarifies things.
> 

For both these, I just forget from time to time which xpcom helper
interfaces have nullable params ;).  I removed both if () {} checks.

> >+NS_IMETHODIMP
> >+nsGenericHTMLFrameElement::GetContainingApp(mozIApplication** aApp)
> >+{
> >+  SAMPLE_LABEL("nsGenericHTMLFrameElement", "GetContainingApp");
> 
> *aApp = nullptr;

Fixed.

> >diff --git a/dom/ipc/TabContext.h b/dom/ipc/TabContext.h
> >index d1bdc8f..8b0f586 100644
> >--- a/dom/ipc/TabContext.h
> >+++ b/dom/ipc/TabContext.h
> 
> >+  nsCOMPtr<mozIApplication> mOwnApp;
> 
> Would you mind adding a comment indicating that this is a cache and
> describing
> the cache's semantics?

Done.

Comment 23

[Justin Lebar (not reading bugmail)]

> What happens if we've cached an app's mozIApplication object and then the
> app gets uninstalled?
>
> What happens currently?

We're not caching mozIApplication objects (as much), so presumably we'd ask for the relevant app and get null back.

> So I went ahead and changed the nsFrameLoader interface name too.

GetContainingApp() was the right name, for both nsFrameLoader and nsIMozBrowserFrame.  The comment described own-or-containing-app, but the comment didn't match the implementation, and the implementation was correct.

Sorry I wasn't clear about that.

> I don't quite follow ... the code here asks the node principal whether
> it has the "embed-apps" permission.  We skip that check if we're not
> an app or browser frame since it can't pass.

But this element's principal can have embed-apps permission even if this element doesn't have the "mozbrowser" attribute.  Indeed, a principal can have embed-apps permission if its document doesn't contain any iframes.  So what this method checks isn't "AppFrameCanEmbedApps" but whether "/this/ frame may be an app frame, given its owner principal and attributes, but ignoring whether its manifest URL corresponds to a known app".

Or am I missing something?

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #23)
> > What happens if we've cached an app's mozIApplication object and then the
> > app gets uninstalled?
> >
> > What happens currently?
> 
> We're not caching mozIApplication objects (as much), so presumably we'd ask
> for the relevant app and get null back.

But anywhere this code is used, we have an open app frame for an app that's been uninstalled.  Surely something prevents that?

> > So I went ahead and changed the nsFrameLoader interface name too.
> 
> GetContainingApp() was the right name, for both nsFrameLoader and
> nsIMozBrowserFrame.  The comment described own-or-containing-app, but the
> comment didn't match the implementation, and the implementation was correct.
> 
> Sorry I wasn't clear about that.
> 
> > I don't quite follow ... the code here asks the node principal whether
> > it has the "embed-apps" permission.  We skip that check if we're not
> > an app or browser frame since it can't pass.
> 
> But this element's principal can have embed-apps permission even if this
> element doesn't have the "mozbrowser" attribute.  Indeed, a principal can
> have embed-apps permission if its document doesn't contain any iframes.  So
> what this method checks isn't "AppFrameCanEmbedApps" but whether "/this/
> frame may be an app frame, given its owner principal and attributes, but
> ignoring whether its manifest URL corresponds to a known app".
> 
> Or am I missing something?

I barely followed your latter comments here, so I'll try to make the changes you suggest without understanding them 100% and throw up a re-review.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Cache mozIApplication wherever possible on critical startup path, v3

Finishing up some testing, posting now in the hope that this finds you after dinner.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Fabrice Desré [:fabrice] from comment #19)
> Created attachment 710246 [details] [diff] [review]
> faster cloneAsMozIApplication
> 
> Addressed comments and fixed the commit message, ready to land.

So land it? :)

Comment 27

[Justin Lebar (not reading bugmail)]

> I barely followed your latter comments here

We should talk about it on IRC sometime.  I'm /pretty/ sure I'm correct...  :)

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Backed out

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

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Relanded with fix for clobber builds suggested by khuey, and rode fabrice's patch along

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2aa085d7ebd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab482df3cd8f

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Backed out again, different IDL bustage but different area.  Sigh

https://hg.mozilla.org/integration/mozilla-inbound/rev/11d2d81e066d

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also failing tests that I ran locally, and were passing.  So more work to do here.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The work in the backed-out patch here has been spun off to bug 839154, and is looking too risky to take.  Let's go ahead and uplift fabrice's patch for this bug.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0a8f71db4c77
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/cb15edb42ed3