Last Comment Bug 853632 - AssertAppProcess is expensive because of GetOwnOrContainingApp cloning the app every time
: AssertAppProcess is expensive because of GetOwnOrContainingApp cloning the ap...
Status: RESOLVED FIXED
[fixed-in-birch]
:
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla23
Assigned To: Jon Hylands [:jhylands]
:
:
Mentors:
Depends on:
Blocks: 847399 857398
  Show dependency treegraph
 
Reported: 2013-03-21 14:02 PDT by Gregor Wagner [:gwagner]
Modified: 2013-05-16 12:16 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
fixed
+
fixed
wontfix
wontfix


Attachments
profile (22.96 KB, text/plain)
2013-03-21 14:04 PDT, Gregor Wagner [:gwagner]
no flags Details
Add a cache for own and containing apps in TabContext (4.45 KB, patch)
2013-04-16 06:37 PDT, Jon Hylands [:jhylands]
no flags Details | Diff | Splinter Review
Add a cache for own and containing apps in TabContext v2 (4.21 KB, patch)
2013-04-16 06:41 PDT, Jon Hylands [:jhylands]
no flags Details | Diff | Splinter Review
Add a cache for own and containing apps in TabContext v3 (3.72 KB, patch)
2013-04-16 08:32 PDT, Jon Hylands [:jhylands]
justin.lebar+bug: review+
Details | Diff | Splinter Review
Add a cache for own and containing apps in TabContext v4 (3.64 KB, patch)
2013-04-16 08:53 PDT, Jon Hylands [:jhylands]
jhylands: review+
akeybl: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2013-03-21 14:02:39 PDT
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] 2013-03-21 14:04:57 PDT
Created attachment 727890 [details]
profile

perf profile
Comment 2 Gregor Wagner [:gwagner] 2013-04-15 05:38:56 PDT
We need this for a faster call log list and SMS list performance.
Comment 3 Alex Keybl [:akeybl] 2013-04-15 08:30:02 PDT
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] 2013-04-16 06:37:51 PDT
Created attachment 737967 [details] [diff] [review]
Add a cache for own and containing apps in TabContext
Comment 5 Jon Hylands [:jhylands] 2013-04-16 06:41:29 PDT
Created attachment 737971 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v2
Comment 6 Justin Lebar (not reading bugmail) 2013-04-16 07:26:32 PDT
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 <jhylands@mozilla.com>
>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] 2013-04-16 08:32:48 PDT
Created attachment 738016 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v3
Comment 8 Justin Lebar (not reading bugmail) 2013-04-16 08:45:52 PDT
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) 2013-04-16 08:46:15 PDT
Oh, before we check this in, can you please do some performance testing to demonstrate a measurable improvement?
Comment 10 Jon Hylands [:jhylands] 2013-04-16 08:53:04 PDT
Created attachment 738028 [details] [diff] [review]
Add a cache for own and containing apps in TabContext v4
Comment 11 Jon Hylands [:jhylands] 2013-04-16 08:58:35 PDT
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] 2013-04-16 09:07:18 PDT
https://hg.mozilla.org/projects/birch/rev/7a382f597514
Comment 13 Justin Lebar (not reading bugmail) 2013-04-16 09:28:49 PDT
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] 2013-04-16 09:31:37 PDT
Working on it...
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-04-16 13:05:46 PDT
https://hg.mozilla.org/mozilla-central/rev/7a382f597514
Comment 16 Jon Hylands [:jhylands] 2013-04-17 04:55:58 PDT
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) 2013-04-17 05:10:05 PDT
Sounds great.  Thanks!
Comment 18 Jon Hylands [:jhylands] 2013-05-10 12:19:26 PDT
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] 2013-05-10 16:10:56 PDT
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) 2013-05-10 16:18:09 PDT
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] 2013-05-13 16:40:50 PDT
Jon - why did you call this risky?
Comment 22 Jon Hylands [:jhylands] 2013-05-13 16:54:21 PDT
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] 2013-05-16 12:16:36 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2aebbd3ba0aa

Note You need to log in before you can comment on or make changes to this bug.