835791 - [Contacts] Improve opening time performance

Status: RESOLVED FIXED

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

Description

[alberto.pastor]

We need to reduce as much as possible all the content that is initially loaded and is not needed for the initial screen

Comment 1

[Andrew Overholt [:overholt]]

Without a more concrete plan or issue here, we can't block so I'm going to mark is as tracking.

Comment 2

[alberto.pastor]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7853

Pointer to Github pull-request

Comment 3

[Andrew Overholt [:overholt]]

Maybe moving this back to tef? was a mistake?  :)

Comment 4

[Andrew Overholt [:overholt]]

I was informed offline that there is a separate triage session for performance-sensitive issues and that this should probably be a blocker.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking status-b2g18 and status-b2g18-v1.0.0 as affected, please update the status to fixed once this is verified landed on v1-train/mozilla-b2g18 and v1.0.0/mozilla-b2g18_v_1_0_0

Comment 6

[David Scravaglieri [:scravag]]

Any update on this ?

Comment 7

[alberto.pastor]

I started yesterday working on this. I already have a patch for async loading Facebook and settings stuff. It decreases already loading time :)

Even though, it can be decreased even more avoiding .getAll from facebook indexdb and other minor improvements. I'll work on that tomorrow.

Comment 8

[Jose Manuel Cantera]

(In reply to Alberto Pastor from comment #7)
> I started yesterday working on this. I already have a patch for async
> loading Facebook and settings stuff. It decreases already loading time :)
> 
> Even though, it can be decreased even more avoiding .getAll from facebook
> indexdb and other minor improvements. I'll work on that tomorrow.
 
great Alberto. Let me know if you need some help with that and in any case ask me for a review for that part

thanks!

Comment 9

[alberto.pastor]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8038

Comment 10

[alberto.pastor]

WIth the patch applied

=== Contacts average load time: 1280ms

Comment 11

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

Comments made on the code, patch looking good, just waiting for address some changes regarding syncing when fb is completely loaded.

Comment 12

[Jose Manuel Cantera]

the patch needs to be rebased to catch up with the latest Contact settings changes made for SIM Import alignment with new progress BBs

Comment 13

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

Comment on attachment 712386 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8038

Nice work,

I guess since this already needs to be rebased we can wait till bug 836170, hopefully will land tomorrow.

Cheers!

Comment 14

[alberto.pastor]

https://github.com/mozilla-b2g/gaia/commit/e6f1589e29a0b1163a42d14954f1df5f03291450

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

This commit does not apply cleanly to v1-train or v1.0.1.  If this patch depends on another bug, please comment here and I will retry when that bug is approved to land on all branches that this bug needs to land on.  If the merge conflict needs to be resolved by hand, the following commands could be a useful starting point:

cd gaia
git checkout v1-train
git cherry-pick -x -m1 e6f1589e29a0b1163a42d14954f1df5f03291450
<resolve merge conflict>
git checkout v1.0.1
git cherry-pick -x $(git log --pretty=%H -n1 v1-train)

The following files needed merging:
#	both modified:      apps/communications/contacts/index.html
#	both modified:      apps/communications/contacts/js/contacts.js
#	both modified:      apps/communications/contacts/js/contacts_list.js

Comment 17

[Lucas Adamski [:ladamski]]

Alberto, can you take a look at this ASAP please?  If this fix is not uplifted it will not make the release.

Comment 18

[alberto.pastor]

Hi,

It seems it depends on Bug 823025, and apart from that, there were some conflicts recording Performance testing scripts (that I guess don't need to land here).

cd gaia
git checkout v1-train
git pull https://github.com/albertopq/gaia.git merge-contacts-performance-opening

That would merge both bugs without conflicts. I've tested it on a device, and everything works as expected.

Regards,

Comment 19

[alberto.pastor]

BTW, Bug 823025 is not strictly a dependency, just a collision regarding changed lines. If you prefer me creating a patch without that bug, I can do it as well.

Regards

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Alberto Pastor from comment #19)
> BTW, Bug 823025 is not strictly a dependency, just a collision regarding
> changed lines. If you prefer me creating a patch without that bug, I can do
> it as well.
> 
> Regards

Alberto, thanks for that.  I've asked for clarification of bug 823025, which looks like we want it on v1.0.1, but we don't have approval to land there.

Comment 21

[alberto.pastor]

After reviewing it, I think is better land this patch first, and not depend on bug 832025

https://github.com/albertopq/gaia/tree/merge-contacts-list-without-823025

823025 patch apply cleanly in top of this, so I think makes more sense in this way.

Thanks

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

Thanks for the merge, Alberto!

v1-train: f486e806e61e206186c325b3e065e44d59988339
v1.0.1: d9c7ba71c724de86a5e70a7dee5c353da8bf69ca

Comment 23

[amiller]

No test case is needed in Moztrap for this issue.

Comment 24

[amiller]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 25

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

(In reply to amiller from comment #24)
> Can you please provide steps to verify this fix - as we will blackbox test
> from the UI?

Hi,

This was another performance modification, instead of loading ALL js at the beginning of the app we load just the necessary js to display the app and later we do dynamic load of the rest of js.

Cheers,
F.

Comment 26

[Mila Davydova]

Unable to verify performance modifications of the app load.
Adding QARegressExclude to the whiteboard