972076 - Modify Apps.mgmt.getAll() to take an optional filter parameter to limit the number of callbacks

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P3)

Description

[Dave Huseby [:huseby]]

While working on Bug 952640, I noticed that currently it is really resource inefficient for the Settings app to count how many homescreen apps are installed on the device.  It uses the Apps.mgmt.getAll() call which causes a callback for each app installed on the system.  It causes the creation of a lot of XPCOM objects and the vast majority of them are immediately thrown away.

It makes more sense for the getAll() function to take an optional filter parameter (e.g. { rote: String }) that the implementation would use to filter out which app objects are passed back to the success callback.  The settings app would then only receive callbacks for each homescreen installed and we'd use a lot less resources in the process.

Comment 1

[[:fabrice] Fabrice Desré]

getAll() sends all its results in a single callback (in the .result property of the dom request object). I'm not sure we want to add filtering/querying to the api. I fear we'll end up with countless custom cases (Give me only the apps that do this or that!).

About the performance issue, it's real but I'd like to see if we get the expected improvements from bug 903291 before trying something else.

Comment 2

[Dave Huseby [:huseby]]

Attachment: 400ms_regression_in_settings.png

Comment 3

[Dave Huseby [:huseby]]

The getAll() call made from the Settings app costs us 400 ms of execution time to create all of the objects being returned in the callback (see attachment 8375224 [details]).  And all but the homescreen app objects are immediately thrown away.  It seems like filtering is a good solution.

Comment 4

[[:fabrice] Fabrice Desré]

It looks like we're blocked for "only" 150ms in your profile (I got similar numbers locally). Why does that cause 400ms of regression?
Also, please try again with the wip from bug 903291 applied. It should remove at least 50ms from your profile (the for-each addMessageListeners part),

Comment 5

[[:fabrice] Fabrice Desré]

Please justify why this should block 1.4 after answering comment 4.

Comment 6

[Dave Huseby [:huseby]]

We concluded that the workaround for the blocker that this blocks is the better immediate solution.  I still think this should be fixed eventually as custom homescreens become more popular.

The workaround for the blocked blocker is to delay the homescreen section population to an idle timer so that it doesn't happen during Settings app startup.

Comment 7

[Dave Huseby [:huseby]]

Attachment: Bug_952640_full_bad.jpg

Here's a screenshot of the full (not just JS) profile.  As you can see that the total time is 328ms when including the C++ code as well.  The total wall clock time of ~400ms includes the main thread IO that you can see as the line of markers in the bottom graph row.

Comment 8

[Dave Huseby [:huseby]]

Currently the impact on the Settings app load is no longer an issue because of Bug 958318.  This api call should still be fixed at some point so that the overhead is not O(n) where n is the number of installed apps.  Eventually this will come back to bite us again.

Right now, we have more immediate issues to deal with so I'm going to backlog it and lower it's priority.

Comment 9

[Kevin Grandon :kgrandon]

This bug would help homescreen performance and third party homescreens. Currently each homescreen would need to have something like the following:

const HIDDEN_ROLES = ['system', 'keyboard', 'homescreen', 'search'];

It would then need to check to see if each app role is within HIDDEN_ROLES before we decide to display the app or not.

My recommendation is to make getAll() take a simple object which may contain a role filter.

navigator.mozApps.mgmt.getAll({
  role: "homescreen"
})

For homescreens, we would want the capability to filter by an "app" role. Currently it looks like webapps.jsm is defaulting the role to an empty string, so we should decide if we should match on that, or default to a type of "app".

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #9)
> This bug would help homescreen performance and third party homescreens.
> Currently each homescreen would need to have something like the following:
> 
> const HIDDEN_ROLES = ['system', 'keyboard', 'homescreen', 'search'];
> 
> It would then need to check to see if each app role is within HIDDEN_ROLES
> before we decide to display the app or not.
> 
> My recommendation is to make getAll() take a simple object which may contain
> a role filter.
> 
> navigator.mozApps.mgmt.getAll({
>   role: "homescreen"
> })
> 
> For homescreens, we would want the capability to filter by an "app" role.
> Currently it looks like webapps.jsm is defaulting the role to an empty
> string, so we should decide if we should match on that, or default to a type
> of "app".

This is painful especially for third party homescreens as they have to know a list of things that they should not displayed and this list can change over time.

It would be nice if by default we can return only real 'apps', or even just returns 'apps' for for application that have the 'homescreen-webapps-manage' permission proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=899994#c17

Comment 11

[[:fabrice] Fabrice Desré]

In any case getAll() is not available without some permission. I'm unconvinced that doing filtering on the platform side is a good idea - we saw how painful that was for the sms db. Note that the first justification for this change is a performance issue that I believe can be fixed differently.

Comment 12

[(no longer active)]

I agree with Fabrice in that fixing the performance issue does not necessarily require changing any APIs here.  But as I've said in the dev-webapi thread on this <https://groups.google.com/forum/#!msg/mozilla.dev.webapi/HKCzYfnSg7A/hXqObFR9DdQJ>, we really need to know more about the nature of the performance problem here.

Comment 13

[Kevin Grandon :kgrandon]

It's more than just performance though - it's also about usability of the API in general. Being able to filter on HIDDEN_ROLES, or fetch a role of a specific type is currently a pain point that we need to implement in several places in gaia. We should not resort to creating shared libraries just to effectively consume an API - this should be basic functionality of the API.

Comment 14

[Kevin Grandon :kgrandon]

I agree though - the bug was opened under the guise of a performance problem. I can open another bug regarding the usability issue if that would be preferred.

Comment 15

[(no longer active)]

(In reply to comment #13)
> It's more than just performance though - it's also about usability of the API
> in general. Being able to filter on HIDDEN_ROLES, or fetch a role of a specific
> type is currently a pain point that we need to implement in several places in
> gaia. We should not resort to creating shared libraries just to effectively
> consume an API - this should be basic functionality of the API.

As mentioned in the dev-webapi thread, it's not clear where we would stop once we start exposing filtering options in the API, since the specific filtering capabilities really depend on what the consumers want to do with the data.  The use case today might be an easier way to filter on a specific role, tomorrow people may ask for an easier way to filter on a specific permission, and the day after they may start asking for a way to "and" or "or" these filters, and soon we will end up with a huge API which is too complicated to both use and implement.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ehsan: What counter proposal do you have?

I agree that we don't want to end up with super-complex filtering here.

But we also don't want to spend 400ms getting the list of applications that we don't need. And we also don't want to make common use cases for apps require a lot of code.

Could someone go through and find the various types of filtering that we do, and why we do them, so that we can see what common patterns emerge?

Comment 17

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #16)
> Ehsan: What counter proposal do you have?
> 
> I agree that we don't want to end up with super-complex filtering here.
> 
> But we also don't want to spend 400ms getting the list of applications that
> we don't need. And we also don't want to make common use cases for apps
> require a lot of code.
> 
> Could someone go through and find the various types of filtering that we do,
> and why we do them, so that we can see what common patterns emerge?

IIRC Dave Huesby did some profiling on the 400ms delay and the profiling implicated wrapping of large jsval's to be the issue here which we expect to be fixed by a combination of bug 903291 and moving the mozApps API to WebIDL.

Comment 18

[[:fabrice] Fabrice Desré]

Attachment: faster-getall.patch

No filtering needed, but this patch removes the need to do ipc for getAll(). On a flame timing went from ~300ms to ~60ms.

Comment 19

[Kevin Grandon :kgrandon]

Nice work. This should also improve the vertical homescreen launch timing as well.

Comment 20

[[:fabrice] Fabrice Desré]

Yep. I don't want to rain on your parade but uplifting bug 903291 to 2.0 looks very much too risky to me...

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8451399 [details] [diff] [review]
faster-getall.patch

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

::: dom/apps/src/AppsServiceChild.jsm
@@ +334,5 @@
>      }
>    },
>  
> +  getAll: function(aCallback) {
> +    if (!aCallback) {

if (!aCallback || typeof aCallback !== "function")

Comment 22

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/213c6fbcd833

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for mochitest-1 failures on b2g desktop: https://tbpl.mozilla.org/php/getParsedLog.php?id=43280198&tree=B2g-Inbound

https://hg.mozilla.org/integration/b2g-inbound/rev/a3816cd9d6da

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

14:37:57     INFO -  Non-local network connections are disabled and a connection attempt to marketplace.allizom.org (63.245.217.111) was made.  You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.

Comment 25

[Marco Castelluccio [:marco]]

This is the same failure I'm seeing on try runs for bug 1007402. I can't see how this patch or the one from bug 1007402 could cause it.

Comment 26

[[:fabrice] Fabrice Desré]

Yep, I think it's unrelated. The network access comes from the update timer that is triggered:

UTM:SVC TimerManager:notify - notified @mozilla.org/b2g/webapps-update-timer;1

Bug 984274 should help

Comment 27

[[:fabrice] Fabrice Desré]

I pushed to try again, and I'm baffled by the results:

https://tbpl.mozilla.org/?tree=Try&rev=5f51fe0702bb

Comment 28

[Lucas Adamski [:ladamski]]

This bug blocks a bug that is the CAF metabug.  If we aren't going to fix this in 2.0 we should remove it from that meta so it does not block CAF.

Comment 29

[Kevin Grandon :kgrandon]

[Blocking Requested - why for this release]:

(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #28)
> This bug blocks a bug that is the CAF metabug.  If we aren't going to fix
> this in 2.0 we should remove it from that meta so it does not block CAF.

I would like to block and nominate this for 2.0 as we are running short on things we can do in gaia. Due to the architecture changes in the vertical homescreen, cache invalidation of a single unified indexedDB is not really an option for us - it proved extremely brittle and regression prone in the last homescreen.

The mozApps interface is currently the single largest point of loading time in the vertical homescreen, on the magnitude of several hundred ms. The patches for this and bug 903291 look like they are nearly ready to go, and I am more than happy to help out and provide support if needed.

Comment 30

[Jason Smith [:jsmith]]

Fabrice - Can you weigh in on whether this is safe to take to 2.0 to help resolve the CAF FC blocking bug?

Comment 31

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #30)
> Fabrice - Can you weigh in on whether this is safe to take to 2.0 to help
> resolve the CAF FC blocking bug?

This bugs depends on bug 903291 which I think is too risky. So it's not possible to get this one either.

Comment 32

[Lucas Adamski [:ladamski]]

If we aren't going to fix this for 2.0 I propose we remove this bug as a dependency of bug 1037706, as that is a CAF blocker.

Comment 33

[Kevin Grandon :kgrandon]

(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #32)
> If we aren't going to fix this for 2.0 I propose we remove this bug as a
> dependency of bug 1037706, as that is a CAF blocker.

If we don't fix this in 2.0, I don't think I can fix bug 1037706.

Comment 34

[Tony Chung [:tchung]]

Kevin, Fabrice, is there any less risky ways to fix this bug, and still feel good about fixing but 1037706?  I worry about Fabrice's risk comment, yet i'd like to know if there are other workarounds in order to decouple this from the blocker bug.

Triage awaits your response before making a blocking call.

Comment 35

[Kevin Grandon :kgrandon]

A caching strategy may be possible in the homescreen, but I feel it's more risky than bug 903291 personally. This would also mean that the very first launch of the homescreen is still slow, which I assume breaks our performance acceptance criteria. I'm currently investigating the caching strategy along with some other things, but I currently have zero patches that provide any benefit for the vertical homescreen. Mozapps fetching is currently our biggest slowdown, and this patch will shave off several hundred ms. I would still like to take it.

Comment 36

[[:fabrice] Fabrice Desré]

I'm not sure how many times I will need to say that I consider that too risky to uplift. We are landing in the beginning of the 2.1 cycle to get enough time to bake that and address regressions (look, we just broke android). I too would like ponies and pot of golds!

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One option here is to use a similar strategy as we use for Contacts API. I.e. add a function to apps.mgmt which returns a "generation number" which increases any time there's an app install/uninstall/update.

This way we can use a relatively simple IndexedDB caching strategy and throw out all cached data as the generation number is different from last time we cached data.

This should allow us to use cached data the vast majority of startups. And only get a slow startup after a installed/uninstall/update.

It does still mean that the first start after buying a phone is one of those slow times. But I think that's acceptable.

Comment 38

[Kevin Grandon :kgrandon]

Fabrice, Antonio - Is the patch here still needed? I noticed that in bug 903291 it appears that we've implemented a similar solution, removing the Webapps:GetAll:Return:OK in favor of a local iteration.

I'm also seeing a ~500ms speed improvement in master over 2.0, so I'm wondering if this came from bug 903291.

Let me know what you think, mozApps might be fast enough now that we can obsolete this patch/close the bug. Thanks!

Comment 39

[[:fabrice] Fabrice Desré]

Hm, that's not true, we still do the ipc roundtrip, see https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#759

Comment 40

[Kevin Grandon :kgrandon]

Ok, sounds like there might be room for more perf improvement here which is nice. Thanks.

Comment 41

[bhavana bajaj [:bajaj]]

Clearing the nom here as I discussed offline with :kgrandon and he is going to file a separate bug for mozApps performance that may help here and nom that one once filed.

Comment 42

[Mike Lee [:mlee]]

Kevin, please cc [e-mail] on that new bug.

Comment 43

[Kevin Grandon :kgrandon]

(In reply to bhavana bajaj [:bajaj] from comment #41)
> Clearing the nom here as I discussed offline with :kgrandon and he is going
> to file a separate bug for mozApps performance that may help here and nom
> that one once filed.

I've filed bug 1045842 for this.

Comment 44

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/9c6c2a98b9e7

Comment 45

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c6c2a98b9e7