1045842 - mozApps.mgmt.getAll() is ~200ms slower in 2.0 than 2.1

Status: RESOLVED WORKSFORME

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

Description

[Kevin Grandon :kgrandon]

Noticed this while profiling bug 1037706. Here are the numbers I'm seeing for an engineering build with 47 apps.

Master Mozapps getAll() performance: 140 - 320
2.0 Mozapps getAll() performance: 300 - 460

Comment 1

[Mike Lee [:mlee]]

[Blocking Requested - why for this release]:bug 1037706 is a 2.0 blocker and the issue reported in this bug is a strong candidate for significantly reducing the problem reported in bug 1037706.

Kevin,
Are you taking this issue or should I find someone on our fxOS Perf team to own it?

Comment 2

[Kevin Grandon :kgrandon]

(In reply to Mike Lee [:mlee] from comment #1)
> Kevin,
> Are you taking this issue or should I find someone on our fxOS Perf team to
> own it?

I would prefer someone on perf or an apps owner/peer drive this. I am happy to assist in debugging this though.

Comment 3

[Mike Lee [:mlee]]

Dave, need you to take this issue and work with Kevin to identify the root and solution.

Comment 4

[[:fabrice] Fabrice Desré]

Well, the cause is because 2.1 has bug 903291 but not 2.0. However we won't uplift it so I'm not sure there's much to do here.

Comment 5

[Kevin Grandon :kgrandon]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Well, the cause is because 2.1 has bug 903291 but not 2.0. However we won't
> uplift it so I'm not sure there's much to do here.

Is there some way of achieving the same thing but with a less-risky patch?

Comment 6

[Dave Huseby [:huseby]]

So the main speed cost is in reading the manifest data.  And since we aren't going to uplift the manifest cache patch in Bug 903291, we have to get creative.

1. we can strip down the patch in Bug 903291 to just do the manifest caching and not everything else.
2. kevin, is there a way that the homescreen can know what apps it wants to show icons for above the fold?  maybe cache them?  because, fabrice, then we can talk about adding optional parameters to getAll() or add a getByName() or getByIndexOffest() functions to the API to work around the O(n) complexity of getAll().

to make this faster we have to read fewer manifest file and/or cache the manifest file data after it is read the first time.  I just thought of another idea:

3. kevin, maybe the homescreen app can cache the manifest data and not call getAll() every time?  as long as it is aware of when apps are installed/removed, then it can keep its own cache consistent.

Comment 7

[Dave Huseby [:huseby]]

BTW, getAll() is *much* faster now than it was 6 months ago.  I'm measuring between 10-11ms per app.  The main performance problem is the O(n) complexity of getAll().

Comment 8

[[:fabrice] Fabrice Desré]

Not sure what you expect of me there. Feel free to try something of course, but don't make api changes.
If you do 2. you can likely use checkInstalled(in DOMString manifestUrl).

Comment 9

[Kevin Grandon :kgrandon]

Cache invalidation in the new homescreen is going to be extremely difficult to implement in 2.0. I would imagine that this would be much more risky than an uplift of bug 903291.

If we can do some magic manifest caching, I think it would be worthwhile to implement and perform some benchmarking.

Comment 10

[Dave Huseby [:huseby]]

I'm going to attempt to strip down the patch(es) in Bug 903291 to only do manifest caching.  Fabrice, what is it in Bug 903291 that is too risky for uplift?  Will I have any chance of landing a stripped down patch that does manifest caching?

Comment 11

[[:fabrice] Fabrice Desré]

I don't know if you can write a correct & not risky patch that does just the manifest part. In  Bug 903291 we changed the way we manage the DOM objects and the ipc traffic to reduce ipc from the parent to a given child, and make sure that dom objects's state is consistent between windows in a given process. So this is a very big change, and while it looks ok now on master that's not something to take on 2.0 that late in the game.

Comment 12

[[:fabrice] Fabrice Desré]

Also, I'm interested in new timings since bug 972076 landed.

Comment 13

[Kevin Grandon :kgrandon]

Seems like there's a few things we should do here.

1 - Perform timings since bug 972076 landed.

2 - See if there is some manifest caching we can do without the mass refactor.

3 - Reconsider uplifting. We're running out of time, so we need to act fast. Uplifting at this point seems like the most risk-free imo.

Comment 14

[Kevin Grandon :kgrandon]

(In reply to Kevin Grandon :kgrandon from comment #13)
> 3 - Reconsider uplifting. We're running out of time, so we need to act fast.
> Uplifting at this point seems like the most risk-free imo.

We also have a ton of gaia tests around apps now. Since they're all passing as of now we should be pretty confident in our ability to make changes to this code.

Comment 15

[Dave Huseby [:huseby]]

I'm re-running the timing right now.

Comment 16

[Dave Huseby [:huseby]]

I tested the timing by changing the JS to collect and output the time it takes to call mozApps.mgmt.getAll().  I rebooted b2g 15 times to measure the execution time of getAll().  I first tested the before changeset (f483f43) and here's what I found:

312,330,292,290,312,237,256,245,296,284,257,370,483,320,307
median: 306 ms
std dev: 60 ms
time per app: 6.9 ms

Then I tested the after (9feaa11) and found:

282,211,207,235,219,231,210,260,302,327,320,350,336,307,327
media: 274 ms
std dev: 52 ms
time per app: 6.2 ms

The patch in Bug 972076 speeds up the call to getAll() by about 10%.  But I want to point out that making getAll() faster is not what we should ultimately do.  Even if we made getAll() only take 1ms per app, if a phone has 100 apps, that is 100ms.  Currently, with the patch from Bug 972076, it takes 6.2 ms per app.  If a phone has 100 apps, it will take 620 ms for this call to return back with data.

I'm going to run the same test against v2.0 tip and see what the results are.

Comment 17

[Dave Huseby [:huseby]]

I recommend that we uplift the 2.1 patch.

Comment 18

[Dave Huseby [:huseby]]

So I just tested the tip of v2.0 (02126fb) and there is trouble.  Running the same test as in Comment 16 I found this:

474,488,443,473,522,448,433,541,448,439,455,554,437,512,564
median: 482ms
std dev: 45ms
time per app: 11ms

This is much worse than just after the Bug 972076 patch landed.  I want to test v2.1 and then bisect this regression to see what causes things to get much slower between 9feaa11 and 02126fb.

Comment 19

[[:fabrice] Fabrice Desré]

2.1 has bug 903291 and bug 972076 that both provide benefits. 2.0 has none of these so it's not surprising to see a difference between 2.0 and 2.1. I'm even surprised that you don't see more improvement after bug 972076.
In comment 16 you said you changed the JS to collect timinig. Was it on the gaia side or in Webapps.js ?

Comment 20

[Dave Huseby [:huseby]]

It was gaia side:

diff --git a/apps/verticalhome/js/sources/application.js b/apps/verticalhome/js/sources/application.js
index 329ec36..0554f97 100644
--- a/apps/verticalhome/js/sources/application.js
+++ b/apps/verticalhome/js/sources/application.js
@@ -6,7 +6,9 @@
   var appMgr = navigator.mozApps.mgmt;
   var apps = null;
 
+  var start_time = performance.now();
   appMgr.getAll().onsuccess = function onsuccess(event) {
+    console.log("XXXXXX: getAll() " + (performance.now() - start_time));
     apps = event.target.result;
     window.dispatchEvent(new CustomEvent('navigator-mozapps-ready'));
   };

Comment 21

[[:fabrice] Fabrice Desré]

ok. I was measuring chrome side. Could be interesting to know how much the event dispatching itself costs, but it's mostly out of curiosity.

Comment 22

[Dave Huseby [:huseby]]

So what is the verdict on this bug?  How are we going to decide what to do?  If we uplift, we take on risk.  If I try to make things faster with new code, we take on risk.  Is this really something we want to block on?

Comment 23

[[:fabrice] Fabrice Desré]

I would not block <broken_record_mode>, and certainly not uplift the 2.1 patches<broken_record_mode>.

Comment 24

[Dave Huseby [:huseby]]

(In reply to Fabrice Desré [:fabrice] from comment #23)
> I would not block <broken_record_mode>, and certainly not uplift the 2.1
> patches<broken_record_mode>.

Does this mean we're accepting the regression?

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Where is there a regression?

Comment 26

[Kevin Grandon :kgrandon]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #25)
> Where is there a regression?

I'm not sure if there is a regression in the mozApps API or not. We would need to compare to 1.4 and I don't think we have yet. There *is* a regression in the homescreen launch time. This is all being tracked in bug 1037706, and there are more details there. There are a few reasons why we have a regression, and we can do more to fix it - but the safest thing to do is to leverage the patches we already have around in 2.1 instead of trying to re-create these gains with new patches.


(In reply to Dave Huseby [:huseby] from comment #22)
> So what is the verdict on this bug?  How are we going to decide what to do? 

I think at this point if we want to ship the vertical homescreen in 2.0 we have no choice but to uplift these patches. Unless bug 1037706 is no longer deemed a CAF blocker, then we are fine.

Comment 27

[Dave Huseby [:huseby]]

So here's timing data collected with the patch in Comment 20 on master this morning:

281
264
246
263
294
240
294
305
254
270
270
249
255
272
277

average is 269
std dev is 19
avg per app is 6.1ms

Comment 28

[Dave Huseby [:huseby]]

Here's numbers for v1.4:

409
386
332
369
402
381
289
415
403
387
441
434
397
361
454

average is 391
std dev is 42
avg per app is 8.9

Comment 29

[Kevin Grandon :kgrandon]

Thanks Dave. According to your numbers we do have a slight regression in 2.0 of approximately ~90ms. (Based on your latest numbers and findings in comment 18).

I don't really think it's worthwhile to try to narrow down the regression if we already have patches in master that fix this gain and quite a bit more (~200 ms or so based on comment 27 and comment 18).

Comment 30

[bhavana bajaj [:bajaj]]

[Blocking Requested - why for this release]:

Given https://bugzilla.mozilla.org/show_bug.cgi?id=1037706 was good enough for 2.0 and any further improvements would be done in 2.1, moving this nom to 2.1 as well.

Comment 31

[Kevin Grandon :kgrandon]

I don't think we need to block unless we need some specific investigation here still. 2.1 is a lot faster already, so thanks for all of the work and investigation here. Long-term, getAll() is not really a good solution for us and third party app developers, so we should explore some other API solutions for them.