853632 - AssertAppProcess is expensive because of GetOwnOrContainingApp cloning the app every time

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Gregor Wagner [:gwagner]]

Profiling insertion of 500 contacts shows that the security check is pretty expensive. We have to clone the app object every single time:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#65

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: profile

perf profile

Comment 2

[Gregor Wagner [:gwagner]]

We need this for a faster call log list and SMS list performance.

Comment 3

[Alex Keybl [:akeybl]]

No new leo perf requirements. Please renominate as leo? if this is needed to hit even v1.0 perf requirements.

Comment 4

[Jon Hylands [:jhylands]]

Attachment: Add a cache for own and containing apps in TabContext

Comment 5

[Jon Hylands [:jhylands]]

Attachment: Add a cache for own and containing apps in TabContext v2

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 737971 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v2

This looks pretty good to me, but I'd like to take another look after you fix
up a few things.

When you attach a patch for checkin, you'll need to attach a patch with hg
headers, not git headers.  moz-git-tools has a script for converting git
patches to hg patches.  You can also use it to post patches directly to
bugzilla.

https://github.com/jlebar/moz-git-tools

btw, you'll probably get prettier diffs with --patience.

>From 049f2162d1fa6611f9e690950e3e894f98faa4de Mon Sep 17 00:00:00 2001
>From: Jon Hylands <[e-mail]>
>Date: Tue, 16 Apr 2013 15:34:11 +0200
>Subject: [PATCH] Bug 853632 - cached the app in TabContext

Nit: s/cached/Cache/.  And you're caching more than one app.

>diff --git a/dom/ipc/TabContext.h b/dom/ipc/TabContext.h
>index b9ecd44..49a74e2 100644
>--- a/dom/ipc/TabContext.h
>+++ b/dom/ipc/TabContext.h

>@@ -154,40 +154,57 @@ protected:
>    * Set this TabContext to be a browser frame inside the given app (which may
>    * be null).
>    */
>   bool SetTabContextForBrowserFrame(mozIApplication* aBrowserFrameOwnerApp,
>                                     ScrollingBehavior aRequestedBehavior);
> 
> private:
>   /**
>-   * Translate an appId into a mozIApplication.
>+   * Translate an appId into a mozIApplication, using lazy caching.
>    */
>   already_AddRefed<mozIApplication> GetAppForId(uint32_t aAppId) const;
> 
>   /**
>+   * Translate an appId into a mozIApplication.
>+   */
>+  already_AddRefed<mozIApplication> PrivateGetAppForId(uint32_t aAppId) const;
>+
>+  /**
>    * Has this TabContext been initialized?  If so, mutator methods will fail.
>    */
>   bool mInitialized;
> 
>   /**
>    * This TabContext's own app id.  If this is something other than NO_APP_ID,
>    * then this TabContext corresponds to an app, and mIsBrowser must be false.
>    */
>   uint32_t mOwnAppId;
> 
>   /**
>+   * This TabContext's own app, cached.  If mOwnAppId is NO_APP_ID, then this
>+   * will be nullptr
>+   */
>+  mutable nsCOMPtr<mozIApplication> mOwnApp;

Nit: Maybe something like

  Cache of this TabContext's own app.  If mOwnAppId is NO_APP_ID, this is
  guaranteed to be nullptr.  Otherwise, it may or may not be null.

Also, please end the sentence with a period.

>+  /**
>    * The id of the app which contains this TabContext's frame.  If mIsBrowser,
>    * this corresponds to the ID of the app which contains the browser frame;
>    * otherwise, this correspodns to the ID of the app which contains the app
>    * frame.
>    */
>   uint32_t mContainingAppId;
> 
>   /**
>+   * The app that contains this TabContext's frame, cached.  If mContainingAppId
>+   * is NO_APP_ID, then this will be nullptr
>+   */
>+  mutable nsCOMPtr<mozIApplication> mContainingApp;

ibid.

>diff --git a/dom/ipc/TabContext.cpp b/dom/ipc/TabContext.cpp
>index f0975f5..7b1999f 100644
>--- a/dom/ipc/TabContext.cpp
>+++ b/dom/ipc/TabContext.cpp

> TabContext::TabContext()
>   : mInitialized(false)
>   , mOwnAppId(nsIScriptSecurityManager::NO_APP_ID)
>+  , mOwnApp(nullptr)
>   , mContainingAppId(nsIScriptSecurityManager::NO_APP_ID)
>+  , mContainingApp(nullptr)
>   , mScrollingBehavior(DEFAULT_SCROLLING)
>   , mIsBrowser(false)

This isn't necessary (and is avoided); nsCOMPtr's (and nsRefPtr's and
nsAutoPtr's) initialize themselves to null in their default constructors.

>@@ -289,16 +291,43 @@ TabContext::AsIPCTabContext() const
> 
> already_AddRefed<mozIApplication>
> TabContext::GetAppForId(uint32_t aAppId) const
> {
>   if (aAppId == nsIScriptSecurityManager::NO_APP_ID) {
>     return nullptr;
>   }

Please add a comment here briefly explaining why we need to cache these,
perhaps referencing the bug.

>+  if (aAppId == mOwnAppId) {
>+    if (!mOwnApp) {
>+      mOwnApp = PrivateGetAppForId(aAppId);
>+    }
>+    NS_ADDREF(mOwnApp.get());
>+    return mOwnApp.get();

Raw ADDREF and RELEASE are usually a sign that code is accidentally leaking, so
even in simple cases like this I prefer

      nsCOMPtr<mozIApplication> ownApp = mOwnApp;
      return ownApp.forget();

>+already_AddRefed<mozIApplication>
>+TabContext::PrivateGetAppForId(uint32_t aAppId) const

We could just merge PrivateGetAppForId into GetAppForId, but if you think it's
cleaner to keep them separate (I don't care either way), I think GetAppForIdNoCache or something like that would be a better name.

Comment 7

[Jon Hylands [:jhylands]]

Attachment: Add a cache for own and containing apps in TabContext v3

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 738016 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v3

>diff --git a/dom/ipc/TabContext.cpp b/dom/ipc/TabContext.cpp
>--- a/dom/ipc/TabContext.cpp
>+++ b/dom/ipc/TabContext.cpp

>@@ -289,16 +289,46 @@ TabContext::AsIPCTabContext() const
> 
> already_AddRefed<mozIApplication>
> TabContext::GetAppForId(uint32_t aAppId) const
> {
>   if (aAppId == nsIScriptSecurityManager::NO_APP_ID) {
>     return nullptr;
>   }
> 
>+  // This application caching is needed to avoid numerous unecessary application clones.
>+  // see Bug 853632 for details.

Nit: Capital 's', please.

>+  if (aAppId == mOwnAppId) {
>+    if (!mOwnApp) {
>+      mOwnApp = GetAppForIdNoCache(aAppId);
>+    }
>+    nsCOMPtr<mozIApplication> ownApp = mOwnApp;
>+    return ownApp.forget();
>+  }
>+
>+  if (aAppId == mContainingAppId) {
>+    if (!mContainingApp) {
>+      mContainingApp = GetAppForIdNoCache(mContainingAppId);
>+    }
>+    nsCOMPtr<mozIApplication> containingApp = mContainingApp;
>+    return containingApp.forget();
>+  }
>+  // we need the fallthrough here because mOwnAppId/mContainingAppId aren't always
>+  // set before calling GetAppForId()...

Nit: Capital 'w', and s/..././

>+  return GetAppForIdNoCache(aAppId);
>+}
>+
>+already_AddRefed<mozIApplication>
>+TabContext::GetAppForIdNoCache(uint32_t aAppId) const
>+{
>+  if (aAppId == nsIScriptSecurityManager::NO_APP_ID) {
>+    return nullptr;
>+  }

We don't need this check twice, I think?

r=me with these nits addressed.  Thanks for the patch!

Comment 9

[Justin Lebar (not reading bugmail)]

Oh, before we check this in, can you please do some performance testing to demonstrate a measurable improvement?

Comment 10

[Jon Hylands [:jhylands]]

Attachment: Add a cache for own and containing apps in TabContext v4

Comment 11

[Jon Hylands [:jhylands]]

Comment on attachment 738028 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v4

Carrying forward the r+ from jlebar with final nits addressed.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/7a382f597514

Comment 13

[Justin Lebar (not reading bugmail)]

My fault for putting checkin-needed before we verified the perf change.

Jon, I'd still like you to do that, please.

Comment 14

[Jon Hylands [:jhylands]]

Working on it...

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a382f597514

Comment 16

[Jon Hylands [:jhylands]]

I added an accumulating timer to the call TabContext::GetAppForId. To bring up the dialer history with 500 entries, that function gets called about 700 times. The numbers work out like this:

Pre-Patch:  1.18 seconds, average time per call 1.69 ms
Post-Patch: 15.4 ms, average time per call 22 us.

So the patch cuts over 1 second off the time to display the names. The 700 calls to the permission check happen after the list of numbers shows up, before any of them are translated into names.

Comment 17

[Justin Lebar (not reading bugmail)]

Sounds great.  Thanks!

Comment 18

[Jon Hylands [:jhylands]]

Comment on attachment 738028 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): permission checks, which happen in quantity when the dialer history window or the sms window is opened (they look for phone number matches in contacts)
User impact if declined: costs an extra second for each ~700 entries in the list doing permission checks
Testing completed: m-c, try
Risk to taking this patch (and alternatives if risky): somewhat risky (fundamental infrastructure change)
String or UUID changes made by this patch: none

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 738028 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v4

Not a blocker for v1.1 and a risky change, this will first appear in v1.2.

Comment 20

[Justin Lebar (not reading bugmail)]

Comment on attachment 738028 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v4

This change is not risky at all, compared to what we commonly take.  We're simply caching a value here.

This is a big performance win that our developers were asking for.

Comment 21

[Alex Keybl [:akeybl]]

Jon - why did you call this risky?

Comment 22

[Jon Hylands [:jhylands]]

Alex,
This was my first gecko patch - I assumed since I was changing how something fundamental works, there is an element of risk. I'm not really familiar with the history of patches and what is considered risky or not - Justin would be a much better person to comment on this one, since its his area of code, and he's the one who reviewed it.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2aebbd3ba0aa