947234 - [1.3] Launch latency of SMS app needs to be improved

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[Mandyam Vikram]

On 1.3, the SMS app launch time needs to meet the Android target number.  The target Android launch latency is 954 ms.The measured launch time on the following build on a 8610 device (dual core)is 1738 ms:
Gecko:
0e3362fb5625eb6d98c7617b1b3019a2cc553d47
Gaia:
f23aebebcd28c4d19347def77c4836c8baebc2ce

STR:
1. Flash the device
2. Load the SMS workload specified earlier
3. Re-boot the device
4. Launch the SMS app and record the launch on the high speed camera
5. Measurement is from the first launch to the SMS messages appearing on the first screen paint
6. Repeat steps 3-5
7. Average the two measurements across the two runs.

As a point of comparison, on v1.2, the SMS app launch latency was measured to be 1713 (almost no improvement on a multi-core device).

Measurements on a slightly older 1.3 build as follows was measured to be 1738 ms (fairly consistent on 1.3):
Gecko:
e5147f9a670e3a315a8aedbf11c3b9672affe8a2
Gaia:
e48c2025b97f26db6e23c16d72095143d731b1fa

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

I find that very harsh and difficult to have such requirements now, when 1.3 will be branched. We know the startup can be improved but this requires massive rearchitecture, that can't be achieved during the stabilization phase.

I'd agree with making this a 1.4 goal, but it's really too late for 1.3.

Comment 2

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Wilfred, Joe, can you please see if we can relax the 1.3 goal for this?

Comment 3

[Michael Vines [:m1] [:evilmachines]]

Parity with Android launch times has always been the requirement.   The good thing is that at the moment the only app that needs significant improvement is SMS.

Comment 4

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

We can probably have some quick wins (stop lazy loading the CSS for example) and see how it goes but I doubt we can go down to 950ms without big changes.

Comment 5

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

A profile and some first steps are in bug 934429.

Comment 6

[Michael Vines [:m1] [:evilmachines]]

Cool, we do have some time here as well, a couple weeks.  Let's try to take a look at the details first and maybe we'll find more wins.

Comment 7

[Preeti Raghunath(:Preeti)]

Gabriele

Any updates on next steps here?

Please provide an ETA for a patch.

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Preeti Raghunath(:Preeti) from comment #7)
> Gabriele
> 
> Any updates on next steps here?

I've started working on it today.

> Please provide an ETA for a patch.

This might take a while, at least a week for doing all the profiling and coming up with a patch and probably another one for reviews and landing as this might contain some fairly significant changes.

Comment 9

[Gabriele Svelto [:gsvelto]]

After analyzing a lot of profiles I've come up with a 3-steps plan to reduce the startup latency:

1) First of all we need to get rid of the lazy-loading on the startup path, I've been experimenting with it and it saves us ~100ms by itself

2) Once we've done that we should actually lazy load files we don't need immediately, this will require some code changes and is suitable only in certain places. I've drawn up a conservative list of the files I want to lazy load; we  could do more but the changes would be too extensive. So the second step will be to modify the code to load the functionality of these files only when needed:

- action_menu.js
- activity_picker.js
- attachment_menu.js
- blacklist.js
- contact_renderer.js
- dialog.js
- gesture_detector.js
- link_action_handler.js
- link_helper.js
- mime_mapper.js
- notification_helper.js
- notify.js
- recipients.js
- settings_url.js
- smil.js
- wbmp.js

I will test as I go by how much time is saved for each file, starting from the easiest ones and moving up to those that require more changes only if it's worth it.

3) getThreads() is started too late, we can gain at least ~200ms by starting it earlier, this will be the last step to be undertaken and I'm not yet sure what would be the best way to address it yet.

Comment 10

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Gabriele Svelto [:gsvelto] from comment #9)
> After analyzing a lot of profiles I've come up with a 3-steps plan to reduce
> the startup latency:
> 
> 1) First of all we need to get rid of the lazy-loading on the startup path,
> I've been experimenting with it and it saves us ~100ms by itself
> 
> 2) Once we've done that we should actually lazy load files we don't need
> immediately, this will require some code changes and is suitable only in
> certain places. I've drawn up a conservative list of the files I want to
> lazy load; we  could do more but the changes would be too extensive. So the
> second step will be to modify the code to load the functionality of these
> files only when needed:
> 
> - action_menu.js
> - activity_picker.js
> - attachment_menu.js
> - blacklist.js
> - contact_renderer.js
> - dialog.js
> - gesture_detector.js
> - link_action_handler.js
> - link_helper.js
> - mime_mapper.js
> - notification_helper.js
> - notify.js
> - recipients.js
> - settings_url.js
> - smil.js
> - wbmp.js
> 
> I will test as I go by how much time is saved for each file, starting from
> the easiest ones and moving up to those that require more changes only if
> it's worth it.
> 
> 3) getThreads() is started too late, we can gain at least ~200ms by starting
> it earlier, this will be the last step to be undertaken and I'm not yet sure
> what would be the best way to address it yet.

From what I read, it looks like doing 3) first would be better ? One action for 200ms :)

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] (PTO until January 6th) from comment #10)
> From what I read, it looks like doing 3) first would be better ? One action
> for 200ms :)

It's a bit complicated to do now with the lazy-loading in place, I could do it as step 2) though once I've got rid of that. Also note that those 200ms are only saved if there's actual threads; considering that's the most common scenario I think it's important but it won't show up when launching the app with an empty message database.

Comment 12

[Dietrich Ayala (:dietrich)]

(In reply to Gabriele Svelto [:gsvelto] from comment #11)
> (In reply to Julien Wajsberg [:julienw] (PTO until January 6th) from comment
> #10)
> > From what I read, it looks like doing 3) first would be better ? One action
> > for 200ms :)
> 
> It's a bit complicated to do now with the lazy-loading in place, I could do
> it as step 2) though once I've got rid of that. Also note that those 200ms
> are only saved if there's actual threads; considering that's the most common
> scenario I think it's important but it won't show up when launching the app
> with an empty message database.

Is that what our test is doing? We should be measuring our performance against the common case, not the uncommon case, no?

Comment 13

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

AFAIK the test is using actual data :)

Comment 14

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] (PTO until January 6th) from comment #13)
> AFAIK the test is using actual data :)

So the database is populated with some threads? Even one thread would be good enough to exercise all the startup code :)

Comment 15

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

I mean, I'm quite sure that the test done in comment 0 is using some threads and messages data (I don't know exactly how many though).

Comment 16

[Preeti Raghunath(:Preeti)]

Gabriele/ Julien,

What are the next steps here?

How do we plan to proceed forward?

Comment 17

[Gabriele Svelto [:gsvelto]]

(In reply to Preeti Raghunath(:Preeti) from comment #16)
> Gabriele/ Julien,
> 
> What are the next steps here?
> 
> How do we plan to proceed forward?

You mid-air collided me while I was attaching the first patch :-) I'm working on the steps I've outlined in comment 9 and I'm pretty confident I should have the patch-set ready by Friday so we can test it thoroughly and land it next week.

Comment 18

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Gabriele is actively working on this.

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Do not lazy load sources in the startup path

This is the first change I had outlined: I'm un-lazy loading all source files and putting them back in the index.html file so that they get loaded right away. My testing shows this change to be worth at least 150-200ms of startup time. That's all the overhead the lazy-loader is adding on top of the loading.

Additionally the startup time variation among different runs becomse quite narrow as a lot of asynchronous has been taken out of the startup path making it much more predictable.

Comment 20

[Rick Waldron [:rwaldron]]

Wow, that's really great. I'll test this for potential regressions, but I can't immediately see anything being problematic.

Comment 21

[Preeti Raghunath(:Preeti)]

Thanks Gabriele.

Comment 22

[Gabriele Svelto [:gsvelto]]

Quick update on my second step here. I've spent most of the afternoon trying to figure out if I could move the getThreads() call earlier to remove the ~200ms delay it introduces while retrieving the cursor from the main process. As it turns out there's no easy way to speed that operation; no matter how early I obtain the cursor the actual DB activity in the main process is always postponed until the SMS app has become quiescent. I'm not sure on which side of the fence the problem lies but I fear it's caused by how the SMS database has been implemented and thus there's no way around this ping-ponging between the two processes in the application code :-(

I'll go on with lazy-loading the sources I've listed in comment 9 in the hope of making up for this.

Comment 23

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8357269 [details] [diff] [review]
[PATCH] Do not lazy load sources in the startup path

OK, I've given this enough testing that I'm comfortable with it and I think it's such an easy win we should put it in so it's time for review.

Comment 24

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8357269 [details] [diff] [review]
[PATCH] Do not lazy load sources in the startup path

Double review to be doubly sure this isn't breaking anything :)

Comment 25

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/2 WIP] Load lazily as many components as possible

And here comes the scary part. In this patch I'm taking out ~1700 lines of JS code and replacing them with placeholders that lazily pull the right files at the right time. I'm doing performance testing right now. I still have another ~800 lines I think I'll be able to take out of the critical path but they require some more work.

So what's scary about this patch? In theory it's all fine and dandy but in practice it makes certain parts of the code which were synchronous asynchronous so it *might* introduce concurrency bugs. I tried to make sure it doesn't but I don't deeply understand all parts of our code.

Comment 26

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/3] Load lazily as many components as possible

Here's an updated version of the second patch; I've taken out some of the most dangerous stuff (like lazy-loading the attachment code, roughly ~250 lines) but started lazy-loading the Facebook code (~800 lines) so it should be a net win. I've profiled this extensively and unfortunately it doesn't seem to make much of a difference vs the all-files-loaded case.

However I'm still keen on taking this because it reduces significantly the amount of memory we take at startup. With an empty message database and this patch applied the app is using 10.88 MiB after startup (10.25 minimized) versus 12.42 MiB (11.34 minimized) for the unmodified app. That's almost a 10% drop and it comes almost entirely from the JS heap / JS runtime. In the light of something like bug 944457 I think this is very relevant.

Comment 27

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 3/3] Recompress all PNG assets

Third and last patch in this series and fortunately the least controversial. This patch includes all our PNG assets duly recompressed. This shaves a few ms worth of load time as well as ~20KiB of memory as the raw assets kept in memory are also shrunk. It's not a big deal but it's essentially a free improvement and it also has the nice effect of shrinking the app's ZIP file by ~7% (48KiB).

Comment 28

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Do not lazy load sources in the startup path

Here's the pull request including all of the patches, the patch from bug 847337 is also applied as this patch-set applies on top of that.

Comment 29

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8359309 [details] [diff] [review]
[PATCH 3/3] Recompress all PNG assets

r=me for that one

yeah, this is a no-brainer.

Comment 30

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8358504 [details] [diff] [review]
[PATCH 2/3] Load lazily as many components as possible

If you see no improvement in startup time, I'd be keen on not taking it for 1.3, and do it again for 1.4 once bug 881469 lands.

Comment 31

[Gabriele Svelto [:gsvelto]]

OK, so to wrap it up I'd say:

- We definitely land the first patch
- Let's wait for bug 881469 (and take it out of the scope of this bug, this will lower the risk too when uplifting to 1.3+)
- We do want the third patch although I could leave it out and do it in bug 959659 instead

Comment 32

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8359309 [details] [diff] [review]
[PATCH 3/3] Recompress all PNG assets

Obsoleting because this Landed as part of bug 959659.

Comment 33

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8358504 [details] [diff] [review]
[PATCH 2/3] Load lazily as many components as possible

Taking this out as we've decided not to take it, we'll open a follow up with this once bug 881469 lands.

Comment 34

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8357269 [details] [diff] [review]
[PATCH] Do not lazy load sources in the startup path

As per the discussion on this bug let's take this one and land it on both master and 1.3 as it's quite safe and a nice win.

Comment 35

[Dylan Oliver [:doliver]]

Julien, this is one of our CS blockers that we're hoping to land by 17 Jan -- can you (or Rick) finish this review tomorrow?

Comment 36

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Yes, this was actually in my todo list for today :)

Comment 37

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 1/3] Do not lazy load sources in the startup path

Un-bitrotted patch, I'll update the PR too.

Comment 38

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8359310 [details] [review]
[PULL REQUEST] Do not lazy load sources in the startup path

Updated the PR to reflect the un-bitrotted patch, I've also removed the patches we decided to drop. Note that this PR includes the patch from bug 847337 as it's needed to apply the patch in this bug.

Comment 39

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8361149 [details] [diff] [review]
[PATCH 1/3] Do not lazy load sources in the startup path

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

r=me

works fine, and I actually really feel the improvement.

Comment 40

[Gabriele Svelto [:gsvelto]]

I did some extra measures using a production gaia build to evaluate the difference there. Total startup time with my patch applied drops by ~130ms there but this doesn't tell the whole story. Removing lazy loading causes us to fire the first request for threads to the main app 500ms in advance. That's probably why I wasn't seeing any improvement by moving the getThreads() call around, it was already starting soon enough with this patch applied.

Comment 41

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

I think I was testing with this configuration, and yes it's really huge.

Maybe you can already prepare a patch for 1.3 too?

Comment 42

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8361149 [details] [diff] [review]
[PATCH 1/3] Do not lazy load sources in the startup path

Clearing the second review flag as per today's IRC discussion, let's ship it!

Comment 43

[Gabriele Svelto [:gsvelto]]

Pushed to gaia/master f0c637b21a568451bb08fc7415de5914ce50ec0b

I'll try to uplift it right away...

Comment 44

[Gabriele Svelto [:gsvelto]]

Uplifted the patch and pushed it to gaia/v1.3 72ebc8b7b9a3b32b9c557d746f3dc13c75a0a416

Comment 45

[Mandyam Vikram]

Unfortunately, on the following recent build (details below), the startup latency as measured with a high speed camera is about 1765 ms:

Gecko: dbb70aeb55524a3513a3c4d10cd8a8e424589a99
Gaia: 50d6487d4d15efda942c934570e6fdfb91f6fe2e

I'm re-opening this bug to have this re-looked at.

Thanks.

Comment 46

[Gabriele Svelto [:gsvelto]]

(In reply to Mandyam Vikram from comment #45)
> Unfortunately, on the following recent build (details below), the startup
> latency as measured with a high speed camera is about 1765 ms

Could you elaborate on how the test was done? Was the SMS database populated or empty? If it's with the database populated then there's very little chances that we'll be able to do anything in the v1.3 timeframe as it would require risky changes. There might be something we could do for reducing launch-time with an empty database but it's a use-case scenario with very little relevance to an end user and spending time on it would be a waste.

Comment 47

[Mandyam Vikram]

Yes, the test was conducted with some workload (please refer to the following in the original STR):
2. Load the SMS workload specified earlier

I can attach the workload separately if needed (the perf team should have the data).

Comment 48

[Preeti Raghunath(:Preeti)]

Gabriele,

Please see if you need any info from CS.

We need a fix for the same this week.

Comment 49

[Gabriele Svelto [:gsvelto]]

(In reply to Mandyam Vikram from comment #47)
> Yes, the test was conducted with some workload (please refer to the
> following in the original STR):
> 2. Load the SMS workload specified earlier

The comment doesn't mention how many messages should be there but I've always tried with our light workload (37 threads, 200 messages). I hope that should be enough. I'm surprised that you've seen no improvement at all with this bug and its dependencies applied as I can consistently measure a 200ms reduction in my profiles. It's not enough to get us to 1s but it's a measurable improvement.

That being said we've got one final card we can use here though we'll have to evaluate its risk once I've got the patch done. The bulk of the time spent here is due to us waiting for the main process to retrieve the thread list. There's nothing we can do about that in the 1.3 timeframe as improving that would require significant platform changes. One thing :julienw suggested in another patch is to cache locally the first threads so that we don't need to wait for the cursor to display them. I can try that and see if we can do it without introducing significant risks (and if it's worth the risk) but I cannot guarantee anything on such a short deadline.

Comment 50

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Gabriele, you can try with the "medium" or "heavy" workload too.

As I said, I can feel there is an improvement when I use the phone. But maybe it comes from the fact that the splash screen move out later, and as a result the time where we see a blank screen is shorter?

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Vikram, would it be possible for you to provide a movie file for the run described in comment 41?  Thats helped us identify differences between measurements in the past.

Comment 52

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8361149 [details] [diff] [review]
[PATCH 1/3] Do not lazy load sources in the startup path

Updating the patch name to reflect the follow-ups.

Comment 53

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/3] Render the threads incrementally

This patch modifies MessageManager.getThreads() to behave like MessageManager.getMessages() enabling a per-thread callback besides the final callback. In addition to this the startup procedure has been modified to render threads incrementally as they come off the cursor and not to wait for all of them to be available.

The upsides are pretty obvious: rendering the first thread now starts ~250ms from the onload event which is a good ~500ms faster than we did before with the light reference workload. This also makes scaling to large number of threads reasonable. With the x-heavy reference workload we start rendering the first thread after only ~400ms versus the >9s we waited before. Finally tapping on a thread during startup gets you to the thread right away while before the view would stay in the thread list until we had finished drawing all the threads.

While this is all nice and dandy there's a downside too: before we would render the threads starting from the most recent, now we go the other way around as the cursor is serving us threads in the opposite order. This is probably fixable but I need to have a look in the platform implementation.

Not asking for review just yet as I've not updated the unit-tests.

Comment 54

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 3/3] Quiet facebook-related errors that aren't really errors

The facebook contacts code is way too chatty. It's spamming the console with errors that aren't really errors, they just mean that the user hasn't enabled FB-sync which will be IMHO a fairly common scenario hence I propose to quiet those messages. Every console.log() call is fairly cheap but once you have one per thread and 1000+ threads they tend to add up.

Comment 55

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Follow up

Here's the PR with patches 2 and 3 of this series.

Comment 56

[Mandyam Vikram]

Attaching a video of one run of launch of the app captured on a high speed camera (240 fps).

Comment 57

[Mandyam Vikram]

Attachment: SMS_launch.mp4

Comment 58

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8363804 [details] [diff] [review]
[PATCH 3/3] Quiet facebook-related errors that aren't really errors

r=me for the SMS part

José Manuel, could you please have a quick look for the fb_data_reader part?

Comment 59

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Just to follow-up, I compared 2 Buri phones, one with the current master, one with the master from just before the startup perf patches.

Here are what I found:
* the performance improvement is more visible when the phone just started. The difference looks lower when we start the apps again after killing them.
* as expected, the splash screen stays longer with the patch applied. But the threads start appearing either earlier or at the same time with the patch applied.
* The direct effect of this is that, without the patch, we wait a longer time with an empty screen, while with the patch we have about equal duration splash screen and empty white screen. As a user, the second behavior feels faster, even if it's not always really faster.

Now, I'm looking at patch 2.

Comment 60

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Gabriele Svelto [:gsvelto] from comment #53)

> While this is all nice and dandy there's a downside too: before we would
> render the threads starting from the most recent, now we go the other way
> around as the cursor is serving us threads in the opposite order. This is
> probably fixable but I need to have a look in the platform implementation.

This is a huge issue, the user doesn't really care of the oldest threads. We want the most recent first.

getMessages has a "invert" boolean flag, see [1]. Gene, do you think someone from your team could implement something similar for getThreads ? This would be for 1.3+...

[1] https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl#59

Comment 61

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8363801 [details] [diff] [review]
[PATCH 2/3] Render the threads incrementally

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

yeah, this is huge, thanks !

We need the Gecko change though, hopefully Gene will be able to do it over night ;-)

::: apps/sms/js/startup.js
@@ +82,5 @@
> +      done: null
> +    };
> +
> +    ThreadListUI.prepareRendering();
> +    MessageManager.getThreads(renderingOptions);

just make this the new ThreadListUI.renderThreads. Nobody calls the current ThreadListUI.renderThreads and that's actually what you're doing, right?

Comment 62

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/3 v2] Render the threads incrementally

Updated patch with:

- modified ThreadListUI.renderThreads() as per comment 61

- code properly refactored to minimize changes on the callers' side

- updated tests that accommodate for the new code

- fixed a couple of tests, one was called "Calls renderDrafts" but didn't actually check for that call and the other one would fail if the profile contained some spurious draft messages (which might have resulted from a previous experimentation or something)

BTW I'm going to look into the threads order part myself tonight. I've had a look at that bit of Gecko and it doesn't seem complicated to add an option to flip the threads' order.

Comment 63

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Gabriele Svelto [:gsvelto] from comment #62)
 something)
> 
> BTW I'm going to look into the threads order part myself tonight. I've had a
> look at that bit of Gecko and it doesn't seem complicated to add an option
> to flip the threads' order.

ok but then please update this bug so that Gene knows (and possibly file a new but with your gecko patch).

Comment 64

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #63)
> (In reply to Gabriele Svelto [:gsvelto] from comment #62)
> ok but then please update this bug so that Gene knows (and possibly file a
> new but with your gecko patch).

Sure! I think I'll have to file another bug anyway because we probably need Fabrice's thumbs up to uplift the Gecko change to 1.3 anyway.

Comment 65

[Preeti Raghunath(:Preeti)]

Julien, Jmcf

Ni for pending patch review.

Comment 66

[Gene Lian (I already quit Mozilla)]

(In reply to Julien Wajsberg [:julienw] from comment #60)
> This is a huge issue, the user doesn't really care of the oldest threads. We
> want the most recent first.
> 
> getMessages has a "invert" boolean flag, see [1]. Gene, do you think someone
> from your team could implement something similar for getThreads ? This would
> be for 1.3+...

To be honest, I don't really like to rush to change API to meet features. As you know, all the changes made on the Web APIs are supposed to be pushed to the public standard and that sould be fully discussed and well designed.

Adding a simple "invert" boolean sounds reasonable to me. Please include me in that bug so that I can take a look/review and thanks Gabriele very much for taking that as well. Please let me know if you encounter any problem during the implementation.

> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/
> nsIDOMMobileMessageManager.idl#59

Comment 67

[Gabriele Svelto [:gsvelto]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #66)
> To be honest, I don't really like to rush to change API to meet features. As
> you know, all the changes made on the Web APIs are supposed to be pushed to
> the public standard and that sould be fully discussed and well designed.

OK, does the API specify in which order the threads are delivered or not? If it doesn't then I suppose we can just invert the order instead of providing a boolean to choose which order is preferred.

> Adding a simple "invert" boolean sounds reasonable to me. Please include me
> in that bug so that I can take a look/review and thanks Gabriele very much
> for taking that as well. Please let me know if you encounter any problem
> during the implementation.

Sure, I'm having a look right now and I'll get back at you as soon as I'm done.

Comment 68

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Gabriele Svelto [:gsvelto] from comment #67)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #66)
> > To be honest, I don't really like to rush to change API to meet features. As
> > you know, all the changes made on the Web APIs are supposed to be pushed to
> > the public standard and that sould be fully discussed and well designed.
> 
> OK, does the API specify in which order the threads are delivered or not? If
> it doesn't then I suppose we can just invert the order instead of providing
> a boolean to choose which order is preferred.

I really think the boolean is preferred, for consistency with getMessages.

Comment 69

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #68)
> I really think the boolean is preferred, for consistency with getMessages.

I'll try that. Thing is, just reverting the order is a very low risk one-line fix so I'll post patches for both changes and we'll pick the more complex one only if it's not too risky.

BTW travis for my PR is as green as the grass in the Netherlands during spring:

https://travis-ci.org/mozilla-b2g/gaia/builds/17496847

Comment 70

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8364504 [details] [diff] [review]
[PATCH 2/3 v2] Render the threads incrementally

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

I'd like some tests in message_manager_test.js for getThreads, we don't have any yet...

Also, I have difficulties to test on a device right now so I'm sending the review without testing this patch yet.

::: apps/sms/js/message_manager.js
@@ +386,2 @@
>      var cursor = this._mozMobileMessage.getThreads(),
>          threads = [];

This "threads" accumulator is not used anymore, you can throw it.

@@ +387,5 @@
>          threads = [];
>  
> +    var each = options.each;
> +    var end = options.end;
> +    var endArgs = options.endArgs;

you can remove this, this is not used anywhere, and if we need to use it we'll use "bind" anyway

::: apps/sms/js/thread_list_ui.js
@@ +380,3 @@
>      // TODO: https://bugzilla.mozilla.org/show_bug.cgi?id=854417
>      // Refactor the rendering method: do not empty the entire
>      // list on every render.

you can remove this comment, bug 854417 was fixed.

@@ +380,4 @@
>      // TODO: https://bugzilla.mozilla.org/show_bug.cgi?id=854417
>      // Refactor the rendering method: do not empty the entire
>      // list on every render.
>      ThreadListUI.container.innerHTML = '';

nit: use "this" instead of "ThreadListUI" (same for all the following code)

@@ -396,5 @@
>  
> -    if (threads.length) {
> -      thlui_renderThreads.abort = function thlui_renderThreads_abort() {
> -        abort = true;
> -      };

I was trying to think whether we needed to keep this but now we call renderThreads only once at the start so I think it's actually unnecessary.

@@ +389,5 @@
>  
> +    FixedHeader.init('#threads-container',
> +                     '#threads-header-container',
> +                     'header');
> +    // Edit mode available

remove this comment as well, it's been forgotten in a previous patch.

@@ +395,4 @@
>  
> +  finalizeRendering: function thlui_finalizeRendering(empty) {
> +    if (empty) {
> +      ThreadListUI.setEmpty(true);

not sure this is right: what happens if you have some drafts? This is a race condition: the draft handling code needs to finish after we come here.

Not sure the current code is right either, though, so maybe this is for another bug.

@@ +402,3 @@
>  
> +    if (!empty) {
> +      // Boot update of headers

nit: remove this unnecessary comment, we're not booting anything (some old code was actually doing it)

@@ +408,3 @@
>  
> +  renderThreads: function thlui_renderThreads(done) {
> +    var empty = true;

nit: I don't like variables with a negative meaning; this makes double negatives and it's awkward to read.

How about using a "hasThreads" instead ?

@@ +425,1 @@
>      }

I think it's more readable to use "this" instead of "ThreadListUI" even if that involves calling "bind".

@@ +426,5 @@
> +
> +    var renderingOptions = {
> +      each: onRenderThread,
> +      end: onThreadsRendered,
> +      endArgs: null,

nit: you don't need endArgs

@@ +427,5 @@
> +    var renderingOptions = {
> +      each: onRenderThread,
> +      end: onThreadsRendered,
> +      endArgs: null,
> +      done: (done ? done : null)

nit: you don't need the parens

::: apps/sms/test/unit/thread_list_ui_test.js
@@ +757,5 @@
> +    test('Rendering an empty screen', function(done) {
> +      ThreadListUI.renderThreads(function() {
> +        assert.ok(FixedHeader.refresh.called);
> +        assert.ok(ThreadListUI.setEmpty.called);
> +        assert.isTrue(ThreadListUI.setEmpty.args[0][0]);

sinon.assert.called(FixedHeader.refresh)
sinon.assert.calledWith(FixedHeader.setEmpty, true);

Comment 71

[Gabriele Svelto [:gsvelto]]

Thanks for the review! Updated patch coming soon, just a couple of comments:

(In reply to Julien Wajsberg [:julienw] from comment #70)
> Also, I have difficulties to test on a device right now so I'm sending the
> review without testing this patch yet.

OK, if you've got your own master build you could also try applying attachment 8364937 [details] [diff] [review] to Gecko and flash the device with that for younger-to-older thread iteration. I'll see if I can cook up some interesting unit tests.

> not sure this is right: what happens if you have some drafts? This is a race
> condition: the draft handling code needs to finish after we come here.

The renderDrafts() method un-sets the emptiness of the thread list if there are only thread-less drafts so I think the behavior is not really changing here. This might or might not be an issue but I can try making it more robust just in case.

For all other comments I'm putting those changes directly into the new version of this patch.

Comment 72

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Gabriele Svelto [:gsvelto] from comment #71)
> 
> The renderDrafts() method un-sets the emptiness of the thread list if there
> are only thread-less drafts so I think the behavior is not really changing
> here. This might or might not be an issue but I can try making it more
> robust just in case.

I meant, imagine this:
* the renderDrafts returns first, and unset the emptiness of the thread list
* _then_ the renderThreads returns, and set the emptiness
=> tada

Comment 73

[Jose Manuel Cantera]

Comment on attachment 8363804 [details] [diff] [review]
[PATCH 3/3] Quiet facebook-related errors that aren't really errors

thanks Gabriele

Comment 74

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/3 v3] Render the threads incrementally

Updated patch with (almost) all review comments addressed, there's two things still missing I'll address today: race between drafts & threads rendering with an empty list and unit tests for ThreadListUI.renderThreads().

Comment 75

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

I'd say, The draft&thread rendering issue is already there in the current code, so please don't pass too much time here, especially since it's an edge case.

Comment 76

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/3 v4] Render the threads incrementally

Updated patch: in render threads we now check if there are some drafts present so if the drafts code runs first we won't accidentally override the emptiness of the view (if drafts rendering code runs afterwards the behavior is unchanged). I've also added an extra unit test for rendering with a populated message DB and moved the asynchronous checks within the done() functions to provide proper stack traces in case of error.

Bug 963475 is about to land so we'll be able to close this bug soon enough.

Comment 77

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/3 v5] Render the threads incrementally

Squashed the linter error. Strangely enough the pre-commit hook didn't catch it; I was doing a rebase so I amended the patch, maybe the hook doesn't work when amending?

Comment 78

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Yeah, the commit hook doesn't run while rebasing, it runs only when running "git commit" something. Maybe we can have another hook (or the same?) to apply during rebasing.

Comment 79

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8366176 [details] [diff] [review]
[PATCH 2/3 v4] Render the threads incrementally

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

(posting a review for the v4 patch since I already wrote most of it ;) )

we're getting there

mostly nits expect for the test comment.

::: apps/sms/js/message_manager.js
@@ +408,4 @@
>      };
>  
>      cursor.onerror = function onerror() {
> +      console.log('Reading the database. Error: ' + this.error.name);

nit: use console.error

::: apps/sms/js/thread_list_ui.js
@@ +420,2 @@
>  
> +      ThreadListUI.appendThread(thread);

nit: this.startRendering and this.appendThread

@@ +423,5 @@
>  
> +    function onThreadsRendered() {
> +      /* We set the view as empty only if there's no threads and no drafts,
> +       * this is done to prevent races between renering threads and drafts. */
> +      ThreadListUI.finalizeRendering(!(hasThreads || Drafts.size));

nit: this.finalizeRendering

@@ +429,5 @@
>  
> +    var renderingOptions = {
> +      each: onRenderThread.bind(this),
> +      end: onThreadsRendered.bind(this),
> +      done: (done ? done : null)

wondering if you can't just put "done" instead of this ternary expression?

::: apps/sms/test/unit/sms_test.js
@@ +222,5 @@
> +
> +        var options = {
> +          each:    each,
> +          end:     null,
> +          done:    null

nit: do we need to be such explicit here? Just don't pass end/done, I guess ?

::: apps/sms/test/unit/thread_list_ui_test.js
@@ +24,5 @@
>  requireApp('sms/test/unit/mock_messages.js');
>  requireApp('sms/test/unit/mock_utils.js');
>  requireApp('sms/test/unit/mock_waiting_screen.js');
> +requireApp('sms/test/unit/thread_list_mockup.js');
> +requireApp('sms/test/unit/utils_mockup.js');

nit: you can use "require('/test/unit/...')", requireApp is useless when loading files from the tested app, and doesn't work in B2G desktop when loading files from other apps.

Please don't change the other lines as this would make the uplift more difficult.

@@ +766,5 @@
> +      ThreadListUI.renderThreads(function() {
> +        done(function checks() {
> +          assert.ok(FixedHeader.refresh.called);
> +          assert.ok(ThreadListUI.setEmpty.called);
> +          assert.isTrue(ThreadListUI.setEmpty.args[0][0]);

you don't need these 3 asserts now, right? ;)

@@ +797,5 @@
> +          sinon.assert.called(ThreadListUI.renderDrafts);
> +          sinon.assert.called(ThreadListUI.prepareRendering);
> +          sinon.assert.called(ThreadListUI.startRendering);
> +          sinon.assert.called(ThreadListUI.appendThread);
> +          sinon.assert.calledWith(ThreadListUI.finalizeRendering, false);

I wonder if we really need this. This is all internal stuff, right ?

I think we need to test the external effect: that the threads are actually displayed.

Also, I think we need to check that one thread is already displayed after the first each.

Comment 80

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/3 v6] Render the threads incrementally

Here we go again. I should have addressed all comments and the unit-tests are now checking the contents of the DOM for the inserted threads to ensure that ThreadListUI.renderThreads() does its job. Just a note, in renderThreads() I had to move the function declarations within the declaration of the renderingOptions object. Outside of it using |this| within the functions triggered linter errors related to potentially not meeting 'strict' requirements.

Comment 81

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Gabriele Svelto [:gsvelto] from comment #80)

>  Outside of it using |this|
> within the functions triggered linter errors related to potentially not
> meeting 'strict' requirements.

You can just add this at the start of the function :
/*jshint validthis:true */

This means "hey, jshint, I know what I do, ok?" :)

Comment 82

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8366669 [details] [diff] [review]
[PATCH 2/3 v6] Render the threads incrementally

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

r=me

please revert the 2 function moves, fix the nit in the test, rebase and wait for a green travis.

Thanks a lot!

::: apps/sms/js/thread_list_ui.js
@@ +421,3 @@
>  
> +        this.appendThread(thread);
> +      }).bind(this),

yeah, please revert this and use /* jshint validthis: true */ instead. See [1] for an example.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_list_ui.js#L264

@@ +425,5 @@
> +      end: (function onThreadsRendered() {
> +        /* We set the view as empty only if there's no threads and no drafts,
> +         * this is done to prevent races between renering threads and drafts. */
> +        this.finalizeRendering(!(hasThreads || Drafts.size));
> +      }).bind(this),

same for this

::: apps/sms/test/unit/thread_list_ui_test.js
@@ +809,5 @@
> +        done(function checks() {
> +          sinon.assert.calledWith(ThreadListUI.finalizeRendering, false);
> +          assert.ok(ThreadListUI.noMessages.classList.contains('hide'));
> +          assert.ok(!ThreadListUI.container.classList.contains('hide'));
> +          assert.ok(!ThreadListUI.editIcon.classList.contains('disabled'));

nit: use assert.isFalse(

Comment 83

[Gabriele Svelto [:gsvelto]]

Merged to gaia/master 7c033555f5f79bca85d2a89a9d78fa3dff7dfead

Travis run: https://travis-ci.org/mozilla-b2g/gaia/builds/17827916

Comment 84

[Gabriele Svelto [:gsvelto]]

Fixed on master, waiting for the v1.3 travis run to uplift this.

Comment 85

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/3 v7] Render the threads incrementally r=julienw

This is the final version of the patch, carrying over the r+ flag.

Comment 86

[Gabriele Svelto [:gsvelto]]

Two tests failed in the Travis run but they're both completely unrelated (calendar):

https://travis-ci.org/mozilla-b2g/gaia/builds/17851165

The patches required some rework but nothing major, basically just removing all Draft-related functionality. They were merged to gaia/v1.3 98b07a5e1452e1ec31eb47aa5489fb84c059b4cb

Comment 87

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Gabriele, it seems like this regress the facebook datastore thing. I'm investigating this a little more now...

Comment 88

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

No, false alert, it's not this patch.

Comment 89

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case has been added here in Moztrap: https://moztrap.mozilla.org/manage/case/13751/