844770 - [SMS] Optimize starting time with lazy-load

Status: RESOLVED FIXED

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

Comment 1

[Borja Salguero [:borjasalguero]]

After some initial testing, with 'b2gperf --iterations=30 --delay=6 Messages', I have the following

Currently:
Results: {'cold_load_time': [1019, 1040, 1100, 1028, 1056, 1067, 1040, 1086, 1100, 1077, 1107, 1222, 1231, 1200, 1271, 1208, 1278, 1294, 1299, 1352, 1280, 1231, 1266, 1289, 1239, 1254, 1262, 1262, 1308, 1321]}
Average time: 1192

With the first approach of lazy-load:
Results: {'cold_load_time': [879, 859, 911, 983, 854, 900, 897, 1002, 937, 933, 1020, 1123, 1062, 1087, 1113, 1076, 1178, 1105, 1098, 1201, 1097, 1176, 1065, 1071, 1148, 1014, 1021, 1037, 1187, 1074]}
Average time: 1036

It's not a big change, but we are saving 200 ms! I will come back with more info!

Comment 2

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 3

[Borja Salguero [:borjasalguero]]

Here we have a patch for SMS Performance! Here the results executing "b2gperf --iterations=30 --delay=6 Messages" :

- Without the patch (code in Master)
Results: {'cold_load_time': [1045, 1079, 1081, 1072, 1100, 1081, 1111, 1116, 1139, 1109, 1266, 1309, 1286, 1242, 1263, 1225, 1278, 1307, 1282, 1269, 1223, 1308, 1274, 1257, 1280, 1314, 1279, 1360, 1231, 1344]}
Average time: 1217

- With the patch:
Results: {'cold_load_time': [914, 930, 1732, 1007, 885, 878, 982, 929, 955, 902, 984, 1081, 1056, 1072, 1111, 1156, 1114, 1137, 1122, 1090, 1059, 1148, 1074, 1106, 1092, 1119, 1083, 1163, 1156, 1047]}
Average time: 1069

We are saving ~200ms in average ;)

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 718497 [details]
Pull Request

Comments put on the PR.

please push new commits only and not squash :-)

We'll need to test every cases very thoroughly. I can think of the following cases:
- normal launch, can we do everything without a glitch ? send sms, edit mode, delete threads, delete messages
- receive a SMS while the app is closed
- receive a SMS while the app is launched
- receive a SMS while the app is launching (can be tricky ;) )
- click the notification while the app is closed
- click the notification while the app is launched
- click the notification while the app is launching
- send a SMS from Contacts when the SMS app is closed
- send a SMS from Contacts when the SMS app is launched
- send a SMS from Contacts when the SMS app is launching (ok, this use case is probably not important)

Comment 5

[Borja Salguero [:borjasalguero]]

I've added your suggestions. Could you take a look? I will squash it as soon as your review will be ready.

Comment 6

[Julien Wajsberg [:julienw]]

There is a last thing but otherwise this seems nice, small load time, and everything seems to work :)

Comment 7

[Borja Salguero [:borjasalguero]]

Lates suggestions added. Could you take a look? I will squash once it has R+. Thanks!

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 718497 [details]
Pull Request

r=me

please file a bug for moving the l10n library out of the startup critical path (and put the Bug number here :) )

thanks

Comment 9

[Borja Salguero [:borjasalguero]]

Here is the bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=847337

Thanks a lot for your review!

Comment 10

[Borja Salguero [:borjasalguero]]

https://github.com/borjasalguero/gaia/commit/6b15cbc7c4505cd62e3a381a5c96d8712ae67ede
https://github.com/mozilla-b2g/gaia/commit/905cc8802794db12eddb0f94dca9dc5d0d3f4e67

Main step for optimizing startup time in SMS App.

Comment 11

[Borja Salguero [:borjasalguero]]

Comment on attachment 718497 [details]
Pull Request

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Starting time it's not optimized
Testing completed: 
Risk to taking this patch (and alternatives if risky): Low. Now we have lazy-loading of the files needed, so starting time it's optimized!
String or UUID changes made by this patch:

Comment 12

[Daniel Coloma:dcoloma]

Agreed with partners we should block on this performance improvement.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 718497 [details]
Pull Request

Removing the approval flag since this is tef+ and can be uplifted without it.

Comment 14

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

I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencie swhich are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1,we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 905cc8802794db12eddb0f94dca9dc5d0d3f4e67
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 15

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

Hey, any chance you could take a look at this merge conflict?

See comment 14 for a starting point on resolving the conflict.

#	both modified:      apps/sms/index.html
#	deleted by us:      apps/sms/js/message_manager.js
#	deleted by us:      apps/sms/js/startup.js

Comment 16

[Julien Wajsberg [:julienw]]

John, the cherry-pick works after uplift for Bug 842315 and Bug 843163 are done.

Comment 17

[Julien Wajsberg [:julienw]]

I think the patch will need to be entirely redone for v1.0.1 though, as neither of these bugs will land on this branch...

Borja, do you think you could do it ?

Comment 18

[Julien Wajsberg [:julienw]]

Be careful, we'll need Bug 841251 uplifted before uplifting this.

Comment 19

[Borja Salguero [:borjasalguero]]

Julien, Why dont we could land this patch to V1.0.1? Currently the majority of the bugs has been landed in master & v1-train right?

Comment 20

[Julien Wajsberg [:julienw]]

Borja, uplifts are not magical :)

This patch depends on Bug 842315 and Bug 843163 that won't land on v1.0.1, so it will need to be adapted to land on this branch.

Someone has to do that, and I think this is either you or me, that's why I'm asking for some help ;) Just tell me if you think you'll be able to do it, otherwise I'll take care of this.

Comment 21

[Borja Salguero [:borjasalguero]]

So we should land first 842315 & 843163 right? I will take a look tomorrow morning!

Comment 22

[Julien Wajsberg [:julienw]]

No, these bugs are not tef+ so they won't land to 1.0.1... We "only" need to change this patch for v1.0.1.

Comment 23

[Borja Salguero [:borjasalguero]]

Ok, I thought they were landed in 1.0.1 as well. Tomorrow I will take a look for sure. Thanks for clarifying! Merçi beaucoup!

Comment 24

[Julien Wajsberg [:julienw]]

ok so it seems Bug 842315 and Bug 843163 will land on v1.0.1 after all. Hope this will resolve all the conflicts !

We still need Bug 841251 to be uplifted though, I hope I'll be able to sort it out later today.

Comment 25

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

v1-train: 7c49a7c651e3569c10969b6cdba053579a90916a

Still waiting on the chain of conflicts to be resolved for v1.0.1

Comment 26

[Julien Wajsberg [:julienw]]

John, Bug 841251 is not uplifted on v1-train either, sorry about that... could you please revert this for now ?

Comment 27

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

reverted on v1-train with 2a2d213048a946d5609f0a617c64a6ae72f48064

Comment 28

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

v1-train: 1857c4c6269b279c62a4aca655df2efdfee0f162

This has a lot of conflicts on v1.0.1

Comment 29

[Julien Wajsberg [:julienw]]

Well, Bug 843163 is not on v1.0.1 yet so this is not really surprising. Asked for some help from Borja there.

Comment 30

[Borja Salguero [:borjasalguero]]

I will take a look today.

Comment 31

[Borja Salguero [:borjasalguero]]

Uplifted https://github.com/mozilla-b2g/gaia/commit/701b594c3e320030c538aa19f702764b29a6cc1c

Comment 32

[Borja Salguero [:borjasalguero]]

After checking I've realized that was uplifted to v1-train as well. Fixed.

Comment 33

[amiller]

No need to create a Test Case in Moztrap for this issue.

Comment 34

[amiller]

No way to Black box test. Cannot Verify Fix this issue.