After profiling, some improvements can be done in order to reduce the number of DOM accesses when painting the list.

Pointer to Github pull-request

Gregor can you take a look at this from a benefit/risk standpoint and determine whether its something we should take for 1.0?

Is this still needed after the fix for 836423?

Yes, the bigger problem is when loading the contacts list for the first time. oncontactschange has nothing to do with that. We are avoiding unneeded querySelectors with this patch. (In reply to Gregor Wagner [:gwagner] from comment #3) > Is this still needed after the fix for 836423?

We'll track this for now, not block, while awaiting more information on the code change options and the risks of taking uplift for those changes.

(In reply to Lukas Blakk [:lsblakk] from comment #5) > We'll track this for now, not block, while awaiting more information on the > code change options and the risks of taking uplift for those changes. Lukas, what is the kind of information you need on the code change options to reconsider the blocking status?

Hi Dani we are still trying different solutions. The PR sent is not a final version but a way of sharing the changes with other devs to check what else we can do. We aggreed on revisit this one we find the solution.

I tried the current version from https://github.com/albertopq/gaia/tree/dummy-contacts and it looks much better! nice work!

Daniel, it wasn't clear if the PR was the final proposed solution so to make a risk assessment we wanted to know what exactly the final landing would be. Based on comment 8 and the push for perf wins right now, we'll go ahead with blocking.

Alberto, any update on this ?

Now that Bug 836423 I will be able to submit the patch I have. It will be ready tomorrow.

Guys, I arrived a point where I don't know how to find more (visible) performance winnings using the profiler. Hopefully Vivien will give me a hand with that. I think what we need is land Bug 836519. That should save us 1.6sec for 500 contacts... At the other hand, applying changes needed in gaia for use Bug 836423 is too risky right now... I wouldn't suggest that for v1. I will create another bug for tracking that.

Comment on attachment 707610 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7848 This improves the time when we start showing the contacts (once we get all the contacts). Despite of not being the final solution, we are still waiting for the cursor soution (bug 836519), that will modify heavily part of this patch, it adds some improvement to the current solution and adds some modifications that will stay. Unit test working so I'm a happy person. Thanks Alberto, this kind of bugs are difficult to deal with.

(In reply to Alberto Pastor from comment #12) > Guys, I arrived a point where I don't know how to find more (visible) > performance winnings using the profiler. I spent some times looking at contacts_list.js - not sure how reliable are the numbers I got but: - renderContact seems to take between 3-8ms for each contacts, even if they have no photo, no organization, no social networks stuff, ... If you have 500 contacts this is means it will takes between 1500 - 4000ms just to render those. I would suggest to rewrite this code to contains only: var contactContainer = document.createElement('li'); contactContainer.className = 'contact-item'; var link = document.createElement('a'); link.href = '#'; var name = document.createElement('p'); name.innerHTML = contact.givenName; (or something better) link.appendChild(name); contactContainer.appendChild(link); Even this part could maybe be rewritten to generate a big string that you add to the dom with innerHTML. All the other parts can be added lazily and you can maintain an object in memory for some of the contacts informations that are not needed as soon you display the list. - do not do appendChild all the time or use some tricks. I know you tried a fragment and it seems good. You may also want to have a display: none before adding to the dom and to restore it right after.

Alberto, what is the status on this ?

We are working on landing 836170 and 835791. As soon as those land, I will be able to keep working on this.

In partner triage we decided that this does not need to block since when there is a patch ready it can be nominated for uplift to v1.0.1 along with a risk/reward assessment and we can decide based on that whether the results gained here will be worth taking.

Review (In reply to Alberto Pastor from comment #18) > Created attachment 714430 [details] > Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8134 Refactor hazard: https://github.com/mozilla-b2g/gaia/pull/8134/files#r3031987 The TL;DR: firstChild, lastChild and childNodes are not limited to nodes of type 1, which means if the markup is changed, you may end up with a whitespace node (likely) or comment node (less likely, but still possible).

There is already examples of non-element nodes in the markup created by this code: https://github.com/albertopq/gaia/blob/0479f3376a3d861a5fc5244f2671dcc458412fe7/apps/communications/contacts/js/contacts_list.js#L200-L202

Thanks Rick, As I said on the PR, I'll take care of it. Here a video comparing master and this branch. http://dl.dropbox.com/u/27553575/render-perfomance.m4v

Sorry for the noisy echo, I posted this immediately following my post on the PR... I'm still not sure where reviews are supposed to happen. Master is left and branch is right? The right side definitely has a smoother experience. What is the policy on "loading indicators"?

Comment on attachment 714430 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8134 r+ once fixed the unit test. Great work!

Filed Bug 842983 as a follow-up because I believe the tests time out in Travis.

Perf must be nominated for tef+

Sorry tef?

We'll take a shot at uplift, marking tef+ accordingly, based on the agreed upon premise that if this shows regressions it will be backed out promptly.

Commit d57cca775b5f2c522d8f0eafd7559f3874f6c7b5 does not apply to v1-train or v1.0.1. This means that there are merge conflicts which need to be resolved. If there are dependencies that are not approved for branch landing, or have yet to land on master, please let me know If a manual merge is required, a good place to start might be: cd gaia git checkout v1-train git cherry-pick -x -m1 d57cca775b5f2c522d8f0eafd7559f3874f6c7b5 <RESOLVE MERGE CONFLICTS> git checkout v1.0.1 git cherry-pick -x $(git log -n1 v1-train)

Conflicts solved in https://github.com/albertopq/gaia/commit/7a92984b2fc291d68a42ba79bbecad4def200d84

v1-train: 9a0fb34 v1.0.1: 6953618

Test case not needed in Moztrap for this issue

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

Alberto Pastor if you can Verify this bug out, it would be much appreciated.

Verified. Check out video above