[contacts] Search data should only be calculated first time entering in search mode

VERIFIED FIXED

Status

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: alberto.pastor, Assigned: alberto.pastor)

Tracking

unspecified
x86
Mac OS X
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [FFOS_perf])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
When rendering the list, we add a data-search attribute to the element in order to perform searches. We should only calculate those attribute values when entering in search mode instead of doing it every time we render the list
(Assignee)

Comment 1

6 years ago
Created attachment 716494 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8237

Pointer to Github pull-request
(Assignee)

Updated

6 years ago
Attachment #716494 - Flags: review?(francisco.jordano)
Attachment #716494 - Flags: review?(crdlc)
in 10 minutes I am going to test, thanks
Assignee: nobody → alberto.pastor
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
As we don't have the search info until the list is loaded, we need to avoid user searching till that happen.

Ayman, what do you think is the best way for doing this?
Flags: needinfo?(aymanmaat)
(Assignee)

Comment 4

6 years ago
Ok, I talked to Ayman and Rafa offline:

- We allow the user entering in search mode anytime
- In case the list is not fully loaded, we don't hide the "Search spinner" when the user starts searching until we loaded the whole list
- If the user removes all the chars in the search input, we go back to the full list (in search mode).

Regards.
Flags: needinfo?(aymanmaat)
(Assignee)

Comment 5

6 years ago
Created attachment 718958 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8237

Pointer to Github pull-request
(Assignee)

Updated

6 years ago
Attachment #718958 - Attachment is obsolete: true
Comment on attachment 716494 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8237

Tested and working good, thanks Alberto!
Attachment #716494 - Flags: review?(francisco.jordano) → review+
Attachment #716494 - Flags: review?(crdlc) → review+
(Assignee)

Comment 7

6 years ago
https://github.com/mozilla-b2g/gaia/commit/70e5a363282302d4fd6099434f24184793ff9afa
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

6 years ago
Comment on attachment 716494 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8237

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: 
Testing completed: Added unit tests, and manually tested by 3 people. 
Risk to taking this patch (and alternatives if risky): Not a 0 risk patch (taking a fully style patch as 0), but low risk. Just lazyLoading search data waiting for user clicking 'Search'.
String or UUID changes made by this patch:
Attachment #716494 - Flags: approval-gaia-v1?
(Assignee)

Updated

6 years ago
Whiteboard: [FFOS_perf]
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
tracking-b2g18: --- → +
Comment on attachment 716494 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8237

Has been tested and it's still early enough in v1.1 that we can take this uplift to v1-train and get more time to catch any regressions.
Attachment #716494 - Flags: approval-gaia-v1? → approval-gaia-v1+
I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.   If this bug depends on patches that aren't approved for v1-train, 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 70e5a363282302d4fd6099434f24184793ff9afa
  <RESOLVE MERGE CONFLICTS>
  git commit
(Assignee)

Comment 11

6 years ago
This one depends on Bug 835742. SHould cleanly apply after merging it.
v1-train: 63ce739
status-b2g18: affected → fixed
Alberto, is it worth asking for uplift to v1.0.1 ? If yes, could you briefly explain why ?
Flags: needinfo?(alberto.pastor)
Julien this is a performance enhancement.

In order to calculate the search data we expend a precious time for each contact (getting all the fields and especially normalising the output) that may not be used if the user never searches.
Actual numbers with/without could be useful so our drivers can decide whether it's worth it.

For example, does that impact the startup time ?
(In reply to Julien Wajsberg [:julienw] from comment #15)
> Actual numbers with/without could be useful so our drivers can decide
> whether it's worth it.
> 
> For example, does that impact the startup time ?

Right this was part of a pack for perfm solutions, will take again the measures to help with the decision :)

Cheers!
(In reply to Julien Wajsberg [:julienw] from comment #15)
> Actual numbers with/without could be useful so our drivers can decide
> whether it's worth it.
> 
> For example, does that impact the startup time ?

w00t!! Just realised that we don't have the performance tests on v1.0.1 or at least I don't see them in the make file.

Any suggestion to try to take this measures?

Thanks!
(Assignee)

Comment 18

6 years ago
If you only need to test startup time, I would suggest using b2gperf: https://github.com/mozilla/b2gperf
Flags: needinfo?(alberto.pastor)
Or a rough measurement by enabling the display of the load time on the phone could be enough if this is big enough :)
(Assignee)

Comment 20

6 years ago
Using b2gperf:

Before:
{'cold_load_time': {'Contacts': [811, 782, 835, 831, 803, 782, 842, 779, 862, 807, 784, 857, 772, 865, 841, 842, 815, 824, 805, 947, 845, 812, 786, 852, 1110, 828, 947, 841, 893, 823]}}
AVG: 840.7666666666667

After:

{'cold_load_time': {'Contacts': [785, 753, 751, 750, 746, 819, 747, 755, 746, 742, 755, 747, 755, 773, 776, 774, 775, 767, 763, 754, 759, 850, 808, 780, 784, 887, 742, 1014, 868, 836]}}

AVG: 785.3666666666667

Moreover, it should reduce total load time (as we only calculate search values when starting to search), but we don't have an easy way to measure it right now, as we were already only loading search values after the last chunk was rendered.
Thanks alberto, seeing these numbers I feel like it is worth having this in 1.0.1. It landed for 3 weeks in master and v1-train already so I believe this is reasonnably safe.
blocking-b2g: --- → tef?
Summary: Search data should only be calculated first time entering in search mode → [contacts] Search data should only be calculated first time entering in search mode
(tef+ due to the desire to make contacts go fast(er) on v1.0.1 + 3 week soak time)
blocking-b2g: tef? → tef+
status-b2g18-v1.0.1: wontfix → affected
v1.0.1: dd568db972e50828453b24929042e0e471497148
status-b2g18-v1.0.1: affected → fixed

Updated

6 years ago
Flags: in-moztrap-

Comment 24

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

This was a performance enhancement. We won't see anything in the UI, now what we do is perform the operations needed to do the contact search when we click the search the first time, instead of doing it at the beginning of the application (we do this to save time during the app startup).

Cheers,
F.
Blocks: 857574

Comment 26

6 years ago
Alberto Pastor if you can Verify this bug out, it would be much appreciated.
Flags: needinfo?(alberto.pastor)
Keywords: verifyme
(Assignee)

Comment 27

6 years ago
Verified due the numbers posted above
Status: RESOLVED → VERIFIED
Flags: needinfo?(alberto.pastor)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.