Contact app launch latency regressed in v2.0

RESOLVED DUPLICATE of bug 1112551

Status

Firefox OS
Gaia::Contacts
P1
blocker
RESOLVED DUPLICATE of bug 1112551
3 years ago
3 years ago

People

(Reporter: tkundu, Assigned: arcturus)

Tracking

({perf, regression})

unspecified
ARM
Gonk (Firefox OS)
perf, regression
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [caf priority: p2][c=regression p= s= u=2.0] [CR 694011] [priority])

Attachments

(1 attachment, 1 obsolete attachment)

Contact app launch latency has regressed compared to v1.3.

For v1.3:
gaia: https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gaia/commit/?h=mozilla/v1.3&id=e08beb0297aff472b5c84437e9d7eaf8c0400a29
gecko: https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/commit/?h=mozilla/v1.3&id=e0bfab94fd20e4dcbb2fecf19d7c2dc606189f81




For v2.0:
gaia: https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gaia/commit/?h=mozilla/v2.0&id=ef67af27dff3130d41a9139d6ae7ed640c34d922
gecko: https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/commit/?h=mozilla/v2.0&id=d3eae03cdf4e6944e646d05938a22dc1380a0d95


Test Steps:
1) Flush v1.3 and v2.0 build on two flame device (512MB) device and load 200 contacts on both device. Launch contact app and compare side by side.

2) launch latency = touchdown to displaying settings options
   In v1.3 launch latency is : 944 msecs
   In v2.0 launch latency is : 1377 msecs
blocking-b2g: --- → 2.0?
Whiteboard: [CR 694011]
I've just sent a patch for defining the new performance events: bug 1015388

How are you meassuring this?

Updated

3 years ago
Flags: needinfo?(mlee)
Keywords: perf

Updated

3 years ago
blocking-b2g: 2.0? → 2.0+

Comment 2

3 years ago
David,
Can you have someone on the Contacts app team look into this?
Flags: needinfo?(mlee) → needinfo?(dscravaglieri)
Whiteboard: [CR 694011] → [c=regression p= s= u=2.0] [CR 694011]

Comment 3

3 years ago
Here's the data we've gathered using Eideticker:


----- Forwarded Message -----
From: "William Lachance" <wlachance@mozilla.com>
Subject: Re: Regressions in App startup time in 2.0

Ok did a run, results below. Videos, etc. at

http://people.mozilla.org/~wlachance/eideticker-startup-test/

(click through to the index page, then click on the bars for videos and an
option to dig into the detail of what we're measuring... e.g.

http://people.mozilla.org/~wlachance/eideticker-startup-test/14-results/contacts-startup-14/#/timetostableframe
->
http://people.mozilla.org/~wlachance/eideticker-startup-test/14-results/contacts-startup-14/detail.html?id=90d314340b8f11e4b65210ddb19eacac#/framediffsums)

Remember this is a comparison between 1.4 and 2.0, so not the same as
what Qualcomm was measuring.

Still:

1. We see a small regression with contacts
2. An *improvement* in settings
3. An *improvement* in camera.

So not what they were seeing.

Hope this helps and let me know if you need more help interpreting this
data,

Will

----

v2.0 aurora (build on 14 July):

=== Results on b2g-contacts-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [1.8833333333333333, 1.95, 1.9, 1.9833333333333334, 1.8666666666666667]

=== Results on b2g-messages-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [4.683333333333334, 4.466666666666667, 4.633333333333334,
4.466666666666667, 4.55]

=== Results on b2g-settings-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [2.316666666666667, 2.1333333333333333, 2.2666666666666666, 2.1,
2.283333333333333]

=== Results on b2g-camera-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [2.6333333333333333, 2.2333333333333334, 2.95, 2.966666666666667, 2.2]

v1.4 (build on 14 July):

=== Results on b2g-contacts-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [1.8, 1.65, 1.65, 1.8166666666666667, 1.5666666666666667]

=== Results on b2g-messages-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [4.016666666666667, 4.0, 4.133333333333334, 4.133333333333334,
3.9833333333333334]

=== Results on b2g-settings-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [2.6166666666666667, 2.5, 2.8666666666666667, 2.1333333333333333, 2.6]

=== Results on b2g-camera-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [3.3, 2.933333333333333, 3.0, 2.8833333333333333, 2.816666666666667]
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #1)
> I've just sent a patch for defining the new performance events: bug 1015388
> 
> How are you meassuring this?

We measured using high speed camera. Could you please follow the steps which I mentioned in Comment 0 ?
Flags: needinfo?(francisco)
Hi,

so we are talking about the latency on displaying the settings.

I'll need more info, since we are doing more operations, is the list completely loaded, or are you launching settings while the list is still being loaeded, not even displaying a contact, etc.
Flags: needinfo?(tkundu)
Flags: needinfo?(francisco)
Flags: needinfo?(dscravaglieri)
Hi folks we are taking a look to this and we have some issues understanding the problems:

- Are we taking about a regression from 1.3 to 2.0 or 1.4 to 2.0 (which makes sense to me)
- When talking about startup time, util we see the settings icon isnt?

Thanks a lot folks!
Hi again,

went through codeb base for 1.4 and 2.0 and the comparisson is the following:

- In 2.0 we got the extra stuff:
  - New css file for the edit mode (this is a feature in 2.0): /shared/style/edit_mode.css
  - New l10n refactor (this happened for all apps)
  - Also a global change in all apps font_size_utils.js added to calculate font header
  - /shared/js/mime_mapper.js this one we can remove it from the critical path and lazyload, is a 4kb file.
  - /shared/locales/import_contacts.ini another locale file, but during build process should not affect.
  - New feature: nfc so we have to load: contacts/js/nfc.js

Perhaps that could explain the small regression between 1.4 and 2.0

When we compare 1.3 with 2.0, a part from the previous changes, we also find the following in the critical path:
-We landed 1.4 features dsds so we got the following extra stuff from 1.3 to 2.0:
  - shared/style/sim_picker.css
  - locales for sim picker (as they are shared), but shouldn't affect after the build process.

I don't have a speed camera but I've been hacking homescreen app and contacts app for doing the following:
- Output Date.now() just before we execute app.launch in the homescreen
- Output Date.now() when we remove the 'hidden' class from the body of the app.

Of course it's not meassuring the time gecko needs to repaint the whole app but the times that I got are the following:
1.4 ~ 780 ms
2.0 ~ 880ms
2.1 ~ 870ms

All of that meassured with 500 contacts. Couldn't get 1.3 data, since we don't have builds for 1.3 and don't think that makes sense to compare gecko 1.4 with gaia 1.3.

The data that I got, kind of match the eideticket data.

Tapas could we have another meassure?
:francisco, since you are helping with investigation here, I'll set the assignee to you as we do not want to have unassigned blockers. Feel free to reassign as needed.
Assignee: nobody → francisco
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #6)
> Hi folks we are taking a look to this and we have some issues understanding
> the problems:
> 
> - Are we taking about a regression from 1.3 to 2.0 or 1.4 to 2.0 (which
> makes sense to me)
> - When talking about startup time, util we see the settings icon isnt?
> 
> Thanks a lot folks!

We compared contact app launch for same 200 contacts loaded in both v1.3 and v2.0 512MB device. 
here launch latency means 'touch down to displaying full page of contacts'

My understanding is that 'end user' will feel bad if they see big delay after moving from v1.3 to v2.0 (see comment 0 for exact numbers) .

I can share a side by side contact app launch comparison video for v1.3 and v2.0. Please let me know if it will help you here.
Flags: needinfo?(tkundu) → needinfo?(francisco)
Hi Tapas!

Yest that will be helpful!

Thanks a lot,
Francisco
Flags: needinfo?(francisco) → needinfo?(tkundu)
Created attachment 8458069 [details]
2.0patch

This is a patch that lazyload all the new features, it goes down around ~200ms.

Sergi could you provide feedback?
Attachment #8458069 - Flags: feedback?(sergi.mansilla)
Comment on attachment 8458069 [details]
2.0patch

I think that you might want to keep the check for NFC. Something like that should work:

LazyLoader.load(['/shared/style/sim_picker.css',
    '/shared/style/action_menu.css'], function() {
  var files = [simPickerNode,
    '/dialer/js/telephony_helper.js',
    '/shared/js/contacts/utilities/templates.js'
  ];

  if ('mozNfc' in navigator) {
    files.push('/contacts/js/nfc.js');
  }

  LazyLoader.load(, function() {
    navigator.mozL10n.translate(simPickerNode);
    detailsReady = true;
    contactsDetails = contacts.Details;
    contactsDetails.init();
    callback();
  });
});

Everything else looks good.
Attachment #8458069 - Flags: feedback?(sergi.mansilla) → feedback+
I forgot to add the `files` array to `LazyLoader.load` in the example in my previous comment.

Updated

3 years ago
Status: NEW → ASSIGNED
Hi,

A branch with Francisco's patch that lazy-loads all new features is here: https://github.com/sergi/gaia/tree/bug_1038179
Tapas, is it easy to run your test against this branch and see the difference? If that's a problem, the PR will eventually get into the 2.0 codebase and you can retest v2.0.
Hi Mike,

do you know if Tapas can try this branch to check the improvements we did?
Flags: needinfo?(mlee)

Comment 17

3 years ago
He should be able to.

Tapas, please re-test with the branch mentioned in comment 14.
Flags: needinfo?(mlee)
Hi,

Just a reminder that this is a blocker issue and we would really appreciate timely feedback about whether the fix improves things in the speed camera test. This is Contact's team current main concern and we are looking forward to closing this issue :)

Thanks!

Updated

3 years ago
Blocks: 1041241

Updated

3 years ago
No longer blocks: 1011657

Comment 19

3 years ago
AS mentioned in Mlee comment # 17. 
Tapas, please re-test with branch mentioned in comment 14.
(In reply to Jean Gong from comment #19)
> AS mentioned in Mlee comment # 17. 
> Tapas, please re-test with branch mentioned in comment 14.

Sorry for delayed reply. Thanks a lot for your help. It seems to me that I just need to cherry-pick following patch[1] into FFOS 2.0 gaia branch [2]

[1] https://github.com/sergi/gaia/commit/c43184f85a3d13c081591011e27342bbbea2435f
[2] https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gaia/log/?h=mozilla/v2.0

I am testing it now and I will update you soon.
Thanks!
(In reply to Mike Lee [:mlee] Paris: 2014.07.21 - 25 from comment #17)
> He should be able to.
> 
> Tapas, please re-test with the branch mentioned in comment 14.

I shared some profiling data with you by email. I am still seeing regression in launch latency even with this patch.
Flags: needinfo?(tkundu)
(In reply to Tapas Kumar Kundu from comment #22)
> (In reply to Mike Lee [:mlee] Paris: 2014.07.21 - 25 from comment #17)
> > He should be able to.
> > 
> > Tapas, please re-test with the branch mentioned in comment 14.
> 
> I shared some profiling data with you by email. I am still seeing regression
> in launch latency even with this patch.

Can Sergi and myself have access to that profiling information?

Thanks in advance!
See Also: → bug 1038176

Updated

3 years ago
Whiteboard: [c=regression p= s= u=2.0] [CR 694011] → [caf priority: p2][c=regression p= s= u=2.0] [CR 694011]

Updated

3 years ago
Keywords: regression

Updated

3 years ago
Blocks: 1011657

Comment 24

3 years ago
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #23)
> 
> Can Sergi and myself have access to that profiling information?
> 

Forwarded you the email with attachment from Tapas |July 22, 2014 5:34 PM| Paris time. Per Tapas' request please don't attach the video to this bug.

Updated

3 years ago
No longer blocks: 1011657
Hi,

Thanks for the video. We are a bit puzzled though because the speed-camera measurements you provided before were very precise and the last ones you sent in the email look rough, since they are made by hand (is there anything else than the video?).

Since we know that the Homescreen is adding part of the delay, we can't take this video as a proof that contacts is still slow. Another speed-camera measurement would help a lot in determining where the bottleneck is.

Thanks!
Sounds like we're blocked on a response from Tapas?
Flags: needinfo?(tkundu)
(In reply to Sergi Mansilla (Telenor) from comment #25)
> Hi,
> 
> Thanks for the video. We are a bit puzzled though because the speed-camera
> measurements you provided before were very precise and the last ones you
> sent in the email look rough, since they are made by hand (is there anything
> else than the video?).
> 
The video which I shared by via last email has low resolution ( I compressed it to decrease video file size) . But it clearly shows delay between contact app launch latency in v2.0 and v1.3. 

> Since we know that the Homescreen is adding part of the delay, we can't take
> this video as a proof that contacts is still slow.

I didn't understand how homescreen is adding part of contact app launch delay. Could you please help me to understand it :) ?
Flags: needinfo?(tkundu) → needinfo?(sergi.mansilla)
Hi Tapas,

we are asking for numbers, as we got in comment 0, cause with this patch we hope that we reduced the time around 100ms, that mixed with the fact that the homescreen is adding an additional delay, perhaps the combination of both is causing this regression that you are commenting.

We got this info from you:
In v2.0 launch latency is : 1377 msecs

Can you tell us how much latency you get with the patch applied?

Thanks a lot!
Flags: needinfo?(tkundu)
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #28)
> Hi Tapas,
> 
> we are asking for numbers, as we got in comment 0, cause with this patch we
> hope that we reduced the time around 100ms, that mixed with the fact that
> the homescreen is adding an additional delay, perhaps the combination of
> both is causing this regression that you are commenting.
> 
> We got this info from you:
> In v2.0 launch latency is : 1377 msecs
> 
> Can you tell us how much latency you get with the patch applied?
> 
> Thanks a lot!

I will confirm you soon. Thanks for the clarification :) .
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #28)
> Hi Tapas,
> 
> we are asking for numbers, as we got in comment 0, cause with this patch we
> hope that we reduced the time around 100ms, 

Could you please rebase this patch for v2.0 tip again ?
Flags: needinfo?(tkundu) → needinfo?(francisco)
Created attachment 8466127 [details] [diff] [review]
patchv20.diff

Here is the rebased version, thanks a lot!!
Attachment #8458069 - Attachment is obsolete: true
Flags: needinfo?(francisco) → needinfo?(tkundu)
Flags: needinfo?(sergi.mansilla)
Hi Bhavana,

We are wating for feedback to see if the patch given eases the problem.
Flags: needinfo?(bbajaj)
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #32)
> Hi Bhavana,
> 
> We are wating for feedback to see if the patch given eases the problem.

Sorry for the delayed response here. We tried to test it last week but we missed last high fps camera launch latency measurement cycle because patch was not re-based for v2.0 (See Comment 30)

I am following up with our internal launch latency measurement team for this patch and we will definitely update you asap. Please expect response from us by Friday, 8th Aug.
Hi all,

I'll be on PTO starting tomorrow till 14th August, meanwhile Jose or Sergi could help with this.

I just checked that patch applies perfectly to 2.0 branch, so hopefully that won't be a problem.

Thanks
(In reply to Francisco Jordano [:arcturus] [:francisco] (on PTO till 14th August) from comment #31)
> Created attachment 8466127 [details] [diff] [review]
> patchv20.diff
> 
> Here is the rebased version, thanks a lot!!

build[1] Contact app launch latency with this patch : 1453ms 

build[1] Contact app launch latency without this patch : 1411ms 

So we are not seeing any improvement with this patch (attachment 8466127 [details] [diff] [review]) if we compare this number to Comment 0. 

BTW, from Comment 0, we are seeing 1411-944=467 ms regression in FFOS v2.0 compared to v1.3
But in Android KK, Contact app is showing launch latency as 1342ms .

So if mozilla does not want to improve launch latency regression between v2.0 and v1.3 then we can consider it as completed. It is not blocking from us anymore. Sorry for late confirmation. 

[1] https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gaia/commit/?h=mozilla/v2.0&id=9e5907995c9327f14cb5d182cee5ff16b1743ed4
    https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/commit/?h=mozilla/v2.0&id=ed87f0a54baf646eb0b20b4debc090c8016d2104

Device : msm8610 512MB
Flags: needinfo?(tkundu) → needinfo?(sergi.mansilla)
Clearing my NI as we've got feedback from CAF here.

Franciso, given comment #35 I think we should be ok to close this issue if the latency is acceptable to CAF. 

But, looks like the launch latency gets worse with the patch applied, so we may not want to land it on master. Franciso, :geo should be able to help us with instructions on set-up so you are able to test this locally.
Flags: needinfo?(bbajaj) → needinfo?(francisco)

Updated

3 years ago
Flags: needinfo?(gmealer)
Yes, make test-perf should be up to doing pre/post-patch tests locally at this point, but I'm doublechecking accuracy and pulling instructions together. Will follow up later today.

Comment 38

3 years ago
Based on comment 35 and #36, removing the dependency.
No longer blocks: 1041241
Flags: in-moztrap?(bzumwalt)
New test case needs to be written
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
I'll handle the make test-perf comparison on this one, re: comment 36. I should be able to turn numbers around tomorrow.

Re: comment 39, we probably need to handle performance bugs a little differently than functional blockers. jsmith will follow up offline, but in this case I can say that we already have tests for contact startup defined in automation and a moztrap case likely isn't necessary.
Test case added in moztrap:

https://moztrap.mozilla.org/manage/case/14276/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(bzumwalt)
Flags: in-moztrap+
(In reply to bhavana bajaj [:bajaj] from comment #36)
> Clearing my NI as we've got feedback from CAF here.
> 
> Franciso, given comment #35 I think we should be ok to close this issue if
> the latency is acceptable to CAF. 
> 
> But, looks like the launch latency gets worse with the patch applied, so we
> may not want to land it on master. Franciso, :geo should be able to help us
> with instructions on set-up so you are able to test this locally.

I would prefer to leave this open, but remove the 2.0+ since it's not a blocker anymore. And I really want to fix this.

Regarding the latency increasing with this patch ... it's really suprising, we have been doing tests and we see around 100ms of improvement.

Wesley, should we remove the 2.0+ then based on comments 35 and 36?
Flags: needinfo?(francisco) → needinfo?(whuang)

Updated

3 years ago
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][2.0-signoff-need+]
blocking-b2g: 2.0+ → backlog
Flags: needinfo?(whuang)
Whiteboard: [caf priority: p2][c=regression p= s= u=2.0] [CR 694011] → [caf priority: p2][c=regression p= s= u=2.0] [CR 694011] [priority]

Updated

3 years ago
QA Whiteboard: [QAnalyst-Triage+][2.0-signoff-need+] → [QAnalyst-Triage+]
(In reply to KTucker (:KTucker) from comment #41)
> Test case added in moztrap:
> 
> https://moztrap.mozilla.org/manage/case/14276/

We probably should be relying on automation here, rather than having a manual test case here, given that need reliability of consistent measurements to determine the performance here. Can we disable this test case?
Flags: needinfo?(ktucker)
Disabled the test case in moztrap.
Flags: needinfo?(ktucker)
Flags: in-moztrap-
Flags: in-moztrap+

Updated

3 years ago
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(sergi.mansilla)
Flags: needinfo?(gmealer)
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
I think that after landing bug 1112551 that will make it to 2.2, we can close this bug since the launch numbers that we get are below the expected ones.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1112551
You need to log in before you can comment on or make changes to this bug.