1006331 - [Tarako][dolphin] Contact cold launch is too long: 2.5 seconds

Status: RESOLVED FIXED

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

Description

[Peipei Cheng (needinfo if you need my action)]

Tarako testing has shows the Contact cold launch from lock to be 2.7 seconds. This needs to be reduced to 1 second.


Please refer to following file for details.
https://bug995303.bugzilla.mozilla.org/attachment.cgi?id=8405480

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I think we also need to examine what we're testing with : https://datazilla.mozilla.org/b2g/?branch=v1.3t&device=tarako&range=30&test=cold_load_time&app_list=contacts,template&app=contacts&gaia_rev=643f3e6676cbb89c&gecko_rev=e757fdd55426&plot=avg

Manually testing I still get the ~ 2.7 seconds 

Gaia      aef737f4eae863949d4b42cd6c17339aec3a5fa0
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/cd0c51278ae5
BuildID   20140506014003
Version   28.1
ro.build.version.incremental=eng.cltbld.20140506.052701
ro.build.date=Tue May  6 05:27:08 EDT 2014
Tarako

Ravi, Geo, is there a criteria that we determined yet?

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

TL;DR: between 1000-1250ms, ideally.

It's a little unclear what the PDF means by cold launch, but assuming that's above-the-fold (all visible content stabilizes) or interaction ready (app takes input) ideally we'd be looking for between 1000ms and 1250ms per https://developer.mozilla.org/en-US/Apps/Build/Performance/Firefox_OS_app_responsiveness_guidelines. That would also put us in parity with Android.

As far as release acceptance goes, the decision made within performance team for Tarako was to look at final numbers/outstanding blockers and make a call from there, similar to what we'd do for functional, rather than publish a series of hard targets at a late date. But the Android numbers are a good guideline for what we should be shooting for.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Oh, one other note, you linked datazilla; onload for Contacts (which is what Datazilla is measuring) plainly fires way before the actual launch is complete. It's almost certainly not valid for comparison with the numbers articulated above. This is a general and well-known issue that we'll be resolving when we can track the events from bug 996038.

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Thanks Geo.  I interpreted cold launch as in reboot the phone, launch + time the app from start of tapping on the icon to the UI load.  I'm getting close to ~3 for user interaction.

Comment 5

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

try to get some profiling

Comment 6

[Mason Chang [Inactive] [:mchang]]

Triage: Please see bug 992120, which according to :bkelly, should increase the cold load time of the contacts app. Joe, will a regression in cold launch time be acceptable for the feature described in bug 992120? Thanks.

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

ni? James for comment 6

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

triage: add this to backlog as we are getting very close to the end, but lets continue the investigation, if there are any low hanging fruits, we may uplift. thanks

Comment 9

[James Zhang (Spreadtrum)]

I don't think bug 992120 cause this issue and please see datazilla. I haven't seen 2.5 seconds launch time.
https://datazilla.mozilla.org/?start=1398936237&stop=1399541037&product=B2G&repository=v1.3t&test=contacts&page=cold_load_time&project=b2g

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to James Zhang from comment #9)
> I don't think bug 992120 cause this issue and please see datazilla. I
> haven't seen 2.5 seconds launch time.
> https://datazilla.mozilla.org/
> ?start=1398936237&stop=1399541037&product=B2G&repository=v1.
> 3t&test=contacts&page=cold_load_time&project=b2g

The Datazilla stat is based on the time before the apploadtime event fires. It's probably not firing at a point representative of what the user would see for cold launch. Naoki and the Taipei team's numbers are observed externally and are likely more accurate.

Bug 992120 wouldn't have caused this bug's issue since it wasn't landed yet. I can see from Datazilla that it also hasn't introduce any further delay before apploadtime after landing. However, it may or may not have caused a further observable performance decrease, depending on whether it added any more processing after the apploadtime event and before user ready.

That seems like an issue to be raised in that bug (or a new one, if it has decreased) though.

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I captured a couple of profiles trying to get the contacts up and profile at the same time; the profile depends on the app running, so it makes it a little bit difficult to capture the startup in it's entirety.

https://drive.google.com/file/d/0B_0LdM1CVycINTN0ZWFJWXF6SUU/edit?usp=sharing
https://drive.google.com/file/d/0B_0LdM1CVycIcEo3eWhPWHVlejQ/edit?usp=sharing

Comment 12

[Peipei Cheng (needinfo if you need my action)]

on buri v1.4, the average time is about 2.5s which is similar to dolphin.

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

On Flame, running gaia 1.4 and mc. 500 contacts.

It takes 550 ms. after the DOM has been loaded until the first batch of contacts appears. This is 550 ms. where there is no content in the contact list. Gives a bad user experience.

On Tarako, running contacts app from gaia 1.4 & gecko 28. 200 contacts. It takes 1.2 seconds.

So if we can make sure to render the first batch of contacts as fast as possible, that would shave off quite some time.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

Measured by putting a <script> tag at the bottom of contacts/index.html page, logging current time. And another one when we render the first chunk.

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch for v1.4

Here is a patch that caches the content above the fold in local storage. It improves startup time (for the user) by ca. 1.2 seconds on Tarako. Some remarks:

1. At the bottom of the page vs. loading as first script in head tag with defer attribute, the defered script takes 56 ms. longer to load.
2. Setting first chunk in localstorage almost has no penalty. Takes 3 ms. of blocking code on Tarako
3. Reading first chunk neither. Takes few ms.

The contacts app will be redone for master, so think this is a quick win with low impact for Dolphin 1.4 release.

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Video showing difference

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 8453004 [details] [review]
Patch for v1.4

Looking cool but redirecting to a contact peer :)

Comment 18

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8453004 [details] [review]
Patch for v1.4

Hei Jan,

great to see you helping with the contacts performance!

We tried that approach but product was not happy with the results, specially in case of contacts change.

Anyway I went through the patch and saw the following:

- Use of localStorage is deprecated, if the content is less than 4KB try to use a cookie, that is also blocking but quite fast compared with localStorage. I guess that using indexedDB (asyncStorage) is penalising you with 250 miliseconds just cause of the overhead of the initialisation of indexeddb. Also I've read that localStorage could be deprecated at some point.

- I would try to clean the cache or try to get mark it as dirty, to avoid the flashing that you'll get from updating contacts:
   - Like changing order in settings
   - Contacts being modified by other app
   - ... or other instance of contacts (from the dialer)
   - or just the fb sync updated process
for that you could save and check the revision id to see if it changed and the cache is valid or not.

I'll ping Carrie and Willfred to see if they are happy in 1.4 with this approach.

Thanks again!

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #18)
> - Use of localStorage is deprecated, if the content is less than 4KB try to
> use a cookie, that is also blocking but quite fast compared with
> localStorage. I guess that using indexedDB (asyncStorage) is penalising you
> with 250 miliseconds just cause of the overhead of the initialisation of
> indexeddb. Also I've read that localStorage could be deprecated at some
> point.

Quick comment on this, I'm on PTO, but here is why I used localstorage:

- Data is bigger than 4K (around 5K raw data, but need to serialize for cookie use, will be 7K in base64 or so)
- Startup penalty idb
- Blocking time is 3ms. on Tarako. I know all the mailing list threads, but this is close to zero.

So I think that LS is the best choice here.

Comment 20

[yang.zhao]

Hi,Francisco
  Could you tell me if this issue has any progress? Thank you.

Comment 21

[Jason.Liu]

[Blocking Requested - why for this release]:

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8453004 [details] [review]
Patch for v1.4

Addressed the following things, what do you think:

- Didnt change localstorage, see above
- Add mutation observer on the list to see when we change
- Create HTML fragment of just above the fold items when observer triggers on delete or insert of LI element
- Compare with current value in localstorage and update if above the fold changed

Should take care of the import/export/external apps dealing with data issue.

Comment 23

[Wayne Chang [:wchang]]

Although performance is on par with buri 1.4 according to comment 12, it seems to be worse than tarako as suggested by comment 13. 

Jan, can you elaborate on why 1.4 on dolphin is worse than 1.4 on tarako?

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

No clue. I don't have Dolphin.

Comment 25

[Wayne Chang [:wchang]]

Thanks, looks like i misunderstood comment 12 and 13.

Performance on par with buri 1.4 - not blocking.

Comment 26

[Francisco Jordano [:arcturus] [:francisco]]

Hei Jan,

excellent job, just a quick question, could you meassure how much we lose if we use indexeddb for storing the first chunk?

Also, did you measure how long it takes to save the data in localStorage since a dom change happen?

Sorry for being that picky, but if we manage to get ride of the localStorage and we put this behind a pref, so we can choose when to use this technique or not, perhaps it can go to master too.

I'm trying to avoid having to port this patch to any of the updates that tarako could have.

If not, we can just land this for 1.4 and I'll be ok for that.

Thanks again!

Comment 27

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #26)
> Hei Jan,
> 
> excellent job, just a quick question, could you meassure how much we lose if
> we use indexeddb for storing the first chunk?
Hi, on Tarako, it takes 446ms. to load first chunk from IDB (through async_storage). Storing doesn't really matter.

> Also, did you measure how long it takes to save the data in localStorage
> since a dom change happen?

On Tarako, getHTMLAboveTheFold takes 13ms. Updating localStorage takes 11ms.

> Sorry for being that picky, but if we manage to get ride of the localStorage
> and we put this behind a pref, so we can choose when to use this technique
> or not, perhaps it can go to master too.

You have any more info about this, cannot find any info about LS becoming deprecated. It's also used *everywhere* in 3rd party apps.

> I'm trying to avoid having to port this patch to any of the updates that
> tarako could have.
> 
> If not, we can just land this for 1.4 and I'll be ok for that.

Yeah, we need it for Dolphin as well. That's why I took up the issue.

Comment 28

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8453004 [details] [review]
Patch for v1.4

Happy to land this in 1.4, just there.

The comment about the use of localstorage has been done several times in the mailing list. I'm not affraid about 3rd party apps, but this being a certified app will be under different conditions.

Anyway, the improvement is needed for tarako and dolphin, so let's get this in 1.4

Found a couple of things:
- We will need unit test for this patch.

- We are saving the first chunk, which includes any group, even the favourites. If we have favourites we will see how they appear cause of the localstorage data, then dissapear when we start painting again and then will appear back.

I'm thinking loud, it's gonna be hard to avoid the flickering since the way we paint the favourites is as we are finding them.

But the win in performance is huge, specially for devices targeted for 1.4

- Would like to hide this under a pref, not really a strong oppinion in this suggestion since is a patch for 1.4. So Jan if you consider that is too much work, or we miss part of that performance we can leave as it is.

Again, happy to include this, since it's a huuuuge win in performance for those devices, could you add the unit tests?

Thanks!

Comment 29

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #28)
> Comment on attachment 8453004 [details] [review]
> Patch for v1.4
> 
> Happy to land this in 1.4, just there.
> 
> The comment about the use of localstorage has been done several times in the
> mailing list. I'm not affraid about 3rd party apps, but this being a
> certified app will be under different conditions.

Well, then we need something better, as localStorage is the best performing API we have in this scenario. There is more than 4K data in here unfortunately.

> Anyway, the improvement is needed for tarako and dolphin, so let's get this
> in 1.4
> 
> Found a couple of things:
> - We will need unit test for this patch.
OK.
 
> - We are saving the first chunk, which includes any group, even the
> favourites. If we have favourites we will see how they appear cause of the
> localstorage data, then dissapear when we start painting again and then will
> appear back.
> 
> I'm thinking loud, it's gonna be hard to avoid the flickering since the way
> we paint the favourites is as we are finding them.

Yeah, there is content jumping at the moment in master. IMHO the current (master) state would not be acceptable either. Good thing is that we can easily fix it by delaying the favourite rendering until the whole list is there. That way there would be no jump for the user.

> But the win in performance is huge, specially for devices targeted for 1.4
> 
> - Would like to hide this under a pref, not really a strong oppinion in this
> suggestion since is a patch for 1.4. So Jan if you consider that is too much
> work, or we miss part of that performance we can leave as it is.

Well, reading the pref would require a call to settings DB which would make stuff slower :p Or we should not include the file at build time or something, but I don't think we should make any significant changes there.

Comment 30

[Jan Jongboom [:janjongboom] (inactive)]

Updated PR with preserving favorites and unit test coverage is on the githubz

Comment 31

[Francisco Jordano [:arcturus] [:francisco]]

Hi Jan,

did a review of the new patch, impressive work here mate.

I just left one comment, related to try to listen to groups that are above the fold, to avoid making pretty slow the import features.

A part from that, the code looks awesome, and unit tests are passing, it looks pretty cool so I think if we fix that we will be ready to go.

Does it sound reasonable for you?

Comment 32

[renfeng.mei]

Comment on attachment 8453004 [details] [review]
Patch for v1.4

good job for me.

Comment 33

[Rachelle Yang [:ryang][[e-mail]]]

[Blocking Requested - why for this release]:

Comment 34

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #31)
> Hi Jan,
> 
> did a review of the new patch, impressive work here mate.
> 
> I just left one comment, related to try to listen to groups that are above
> the fold, to avoid making pretty slow the import features.
> 
> A part from that, the code looks awesome, and unit tests are passing, it
> looks pretty cool so I think if we fix that we will be ready to go.
> 
> Does it sound reasonable for you?

See Github

Comment 35

[Wayne Chang [:wchang]]

From comment 28 - sounds like there is a tradeoff and we'll be seeing flickering if we land this.

Triage decides not to land this as it will introduce other problems being reported by partner. However, thanks for the great patch and improvement in performance. 

Partners are welcome to pick this up on 1.4 but do note the tradeoff involved.

Comment 36

[Jan Jongboom [:janjongboom] (inactive)]

[Blocking Requested - why for this release]:

(In reply to Wayne Chang [:wchang] from comment #35)
> From comment 28 - sounds like there is a tradeoff and we'll be seeing
> flickering if we land this.
> 
> Triage decides not to land this as it will introduce other problems being
> reported by partner. However, thanks for the great patch and improvement in
> performance. 
> 
> Partners are welcome to pick this up on 1.4 but do note the tradeoff
> involved.

There is no flickering involved. See comment 30.

Comment 37

[Wayne Chang [:wchang]]

Ah great!

Apologies for overlooking. Thanks - taking 1.4 since no additional concerns.

Comment 38

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #34)
> 
> See Github

Great point, like the idea of stoping and resuming the observer when we have a massive update on the list (it can be import and delete as well).

Left in github another idea on how to do it.

Thanks for the great work Jan!

Comment 39

[Jan Jongboom [:janjongboom] (inactive)]

Here you go, stops DOM updates during batch import: https://github.com/comoyo/gaia/commit/07cb1e41405ce60c8a115ba37db018b595fa6b3b

Comment 40

[Francisco Jordano [:arcturus] [:francisco]]

Jan, amazing job in here!

I think this is ready for making it to 1.4.

I will be on holidays from tomorrow, but I already see the code and everything looks perfect to me, could you squash, I would like to delegate the final r? in case I don't have time to do it to Sergi.

You both are close and the code as I said is looking pretty good.

Thanks again for the fantastic job!

Comment 41

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8453004 [details] [review]
Patch for v1.4

Thanks! Forwarded the r? to Sergi.

Comment 42

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8453004 [details] [review]
Patch for v1.4

I left some comments in the PR with small nits to be corrected. It looks good to me, nice improvement!

Comment 43

[Jan Jongboom [:janjongboom] (inactive)]

Fixed nits & rebased. Try running @ https://tbpl.mozilla.org/?rev=b91e164df8900fcf0e49818b55f4679a1eb0d2bd&tree=Gaia-Try

Comment 44

[Jan Jongboom [:janjongboom] (inactive)]

checkin-needed on 1.4

Comment 45

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/66b4b164536caf96529f702291b196773c34ad06

Marking 2.0/2.1 as wontfix based on discussion with Jan on IRC. Sounds like a follow-up bug should be filed for any work to be done porting this fix to them.