Closed Bug 907907 Opened 6 years ago Closed 6 years ago

[Contacts] Implement a lazily loaded DOM, based on WebComponents

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 C2(Oct11)
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

(Keywords: perf, Whiteboard: [c= p=3 s= u=1.2])

Attachments

(1 file)

No description provided.
Depends on: 907860
Blocks: 871823
Summary: Implement a lazily loaded DOM in contacts, based on WebComponents → [Contacts] Implement a lazily loaded DOM, based on WebComponents
Keywords: perf
Whiteboard: [c= p= s= u=]
Status: NEW → ASSIGNED
Blocks: 908365
No longer blocks: 908365
Whiteboard: [c= p= s= u=] → [c= p=3 s= u=]
Comment on attachment 793685 [details]
Github pull request pointer

Hi Francisco - When you get a chance please take a look and add any other people that you think should take a look. Thanks!
Attachment #793685 - Flags: review?(francisco.jordano)
Comment on attachment 793685 [details]
Github pull request pointer

Great job!

I'm sure the datazilla will be super happy.

Just left two issues on github, that are basically:

- The search dom must be loaded when we are about to export to any source, actually is not export, but use the list in select mode.
- Now that we are loading the overlay dom asynchronously we will need to transform the rest of the code calls to asynchronous, since before we were returning the dom node to allow the app to update the overlay.

A part from that, just have to thank you for the fantastic job, and sorry for the delay ;)

Cheers,
F.
Attachment #793685 - Flags: review?(francisco.jordano) → review-
Comment on attachment 793685 [details]
Github pull request pointer

Thanks for the awesome review Francisco! Update the PR with some issues addressed.
Attachment #793685 - Flags: review- → review?(francisco.jordano)
Comment on attachment 793685 [details]
Github pull request pointer

Looking perfect for me.

Adding Jose as a reviewer in case we want to double check the FB part.

Thanks a lot Kevin for the hard work, definitely will make a difference.
Attachment #793685 - Flags: review?(jmcf)
Attachment #793685 - Flags: review?(francisco.jordano)
Attachment #793685 - Flags: review+
Depends on: 914832
Depends on: 914835
Depends on: 915018
According to Jose, this patch is currently too big. I'm going to open a few dependent bugs for the smaller pieces, and leave the big one as-is for now. Patches to come.

Current diff shows: 36 changed files with 748 additions and 638 deletions.
Depends on: 915055
With some refactoring of work, I was able to open multiple pull requests. The main pull request is now mainly HTML movement. The commit stats are as follows:

Old: 36 changed files with 748 additions and 638 deletions.
New: 13 changed files with 549 additions and 512 deletions. 

Note, almost the entirety of the new pull request is moving HTML around.
I will be reviewing this patches in a low priority thread, as they are not blocking v1.2 which is currently our priority.
(In reply to Jose M. Cantera from comment #8)
> I will be reviewing this patches in a low priority thread, as they are not
> blocking v1.2 which is currently our priority.

I think we need to work together to find a better solution than that. Typically the priority should be reviews over anything else - this allows other people to move much faster. Overall the entire gaia team can move much faster if we knock out reviews quicker.
(In reply to Jose M. Cantera from comment #8)
> I will be reviewing this patches in a low priority thread, as they are not
> blocking v1.2 which is currently our priority.

I suppose the proper thing to do is to get this bug properly prioritized. Because this bug blocks a 1.2 bug for us (871823), I think it makes sense to have this one block as well.
blocking-b2g: --- → koi?
blocking-b2g: koi? → koi+
Whiteboard: [c= p=3 s= u=] → [c= p=3 s= u=][perf-reviewed]
Priority: -- → P1
Whiteboard: [c= p=3 s= u=][perf-reviewed] → [c= p=3 s= u=]
As I said to Dietrich in a private e-mail 

"Do you have any estimation about the perf win that these patches provide?
In my opinion it can be risky to land this code in 1.2 as now the Contacts
App is very stable and IMHO the regression risk is high with all that
code."
Flags: needinfo?(kgrandon)
(In reply to Jose M. Cantera from comment #11)
> "Do you have any estimation about the perf win that these patches provide?
> In my opinion it can be risky to land this code in 1.2 as now the Contacts
> App is very stable and IMHO the regression risk is high with all that
> code."

My back of the envelop estimation would be on the order of saving ~100ms once all of these patches land. I'll try to do some more profiling to get exact numbers, but they should be significant.

I was under the impression that contacts app performance was blocking 1.2. If it's not, then there's no problem moving these to 1.3.
Flags: needinfo?(kgrandon)
(In reply to Kevin Grandon :kgrandon from comment #12)
> ...
> I was under the impression that contacts app performance was blocking 1.2.
> If it's not, then there's no problem moving these to 1.3.

It *is* blocking 1.2, see bug 915073 for the regression since 1.1.
Blocks: 915073
This patch is only saving us 100ms and the amount of new code and regression risk is high. As according to bug 915073 we need to save 1 sec I believe this is not the performance win we need. It seems to me that this is a cosmetic change that is not going to make a difference.
(In reply to Kevin Grandon :kgrandon from comment #9)
> (In reply to Jose M. Cantera from comment #8)
> > I will be reviewing this patches in a low priority thread, as they are not
> > blocking v1.2 which is currently our priority.
> 
> I think we need to work together to find a better solution than that.
> Typically the priority should be reviews over anything else - this allows
> other people to move much faster. Overall the entire gaia team can move much
> faster if we knock out reviews quicker.

yes, but reviewers time is precious and they shouldn't be spending time in reviewing changes that do not make a real difference and that only introduce regression risks.
(In reply to Jose M. Cantera from comment #14)
> This patch is only saving us 100ms and the amount of new code and regression
> risk is high. As according to bug 915073 we need to save 1 sec I believe
> this is not the performance win we need. It seems to me that this is a
> cosmetic change that is not going to make a difference.

100ms is absolutely a huge win. If we have 10 wins at 100ms each, we will meet our goal of 1 sec performance improvements. You're not going to find a one-liner at this point which will shave off 1s of load time. It's going to be a long hard fight, and we're trying to help with that.
100ms is *huge*. Jose, please review the UX guidelines for performance and responsiveness at https://mozilla.app.box.com/s/aww17rx74k7fjds5vada.
(In reply to Jose M. Cantera from comment #15)
> (In reply to Kevin Grandon :kgrandon from comment #9)
> > (In reply to Jose M. Cantera from comment #8)
> > > I will be reviewing this patches in a low priority thread, as they are not
> > > blocking v1.2 which is currently our priority.
> > 
> > I think we need to work together to find a better solution than that.
> > Typically the priority should be reviews over anything else - this allows
> > other people to move much faster. Overall the entire gaia team can move much
> > faster if we knock out reviews quicker.
> 
> yes, but reviewers time is precious and they shouldn't be spending time in
> reviewing changes that do not make a real difference and that only introduce
> regression risks.

You don't need to do a full review since Francisco has already done one. I think he asked for an additional review mostly to check if nothing will broke for the facebook code path.
As a data point, I added class=hide to the offscreen panel <section> elements.  This seems to improve load time by 200ms to 300ms.  I think landing this will be a big win.

We may still want to consider using hide class when a panel is dismissed and goes offscreen, though.
Video of v1.2 with these patches applied.  Launch time is about 1.9s which seems to be about 100ms improvement as Kevin originally predicted.  My times in comment 19 were measured from JS, so did not capture the entire picture.

  https://www.dropbox.com/s/g11ykbfu8ga66w5/20130923_lazy_html.mov
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #18)
> (In reply to Jose M. Cantera from comment #15)
> > (In reply to Kevin Grandon :kgrandon from comment #9)
> > > (In reply to Jose M. Cantera from comment #8)
> > > > I will be reviewing this patches in a low priority thread, as they are not
> > > > blocking v1.2 which is currently our priority.
> > > 
> > > I think we need to work together to find a better solution than that.
> > > Typically the priority should be reviews over anything else - this allows
> > > other people to move much faster. Overall the entire gaia team can move much
> > > faster if we knock out reviews quicker.
> > 
> > yes, but reviewers time is precious and they shouldn't be spending time in
> > reviewing changes that do not make a real difference and that only introduce
> > regression risks.
> 
> You don't need to do a full review since Francisco has already done one. I
> think he asked for an additional review mostly to check if nothing will
> broke for the facebook code path.

And because the patch is huge and as we say in Spain, "four eyes can see more than two", and in my case as I wear glasses they are six :)
Kevin,

I see a major regression with this patch. The path is:

Enter into 'Search Mode' by putting the cursor on the search box in the Contacts list. Then, exit search mode by clicking on 'Cancel' an ugly and spureous white screen will appear. If you do the same with current master, you will see that the transition between the search mode and list mode is quite smooth. 

Please could you review that issue? It is the *only* issue I can see in the patch

thanks
Flags: needinfo?(kgrandon)
Attachment #793685 - Flags: review?(jmcf) → review-
(In reply to Jose M. Cantera from comment #22)
> Kevin,
> 
> I see a major regression with this patch. The path is:
> 
> Enter into 'Search Mode' by putting the cursor on the search box in the
> Contacts list. Then, exit search mode by clicking on 'Cancel' an ugly and
> spureous white screen will appear. If you do the same with current master,
> you will see that the transition between the search mode and list mode is
> quite smooth. 
> 
> Please could you review that issue? It is the *only* issue I can see in the
> patch
> 

Another major regression. Go to Contacts/Import Gmail. Import some contacts and then the status message with the number of imported contacts will not appear. In the console you will find

E/GeckoConsole( 4245): [JavaScript Error: "TypeError: statusMsg.querySelector(...) is null" {file: "app://communications.gaiamobile.org/contacts/js/utilities/status.js" line: 10}]


I have set r-. Please react on this problems and ask for a new review. 

> thanks
I distinctly remember testing these issues ~1 month ago when the patch was submitted and they were working so perhaps there is some bit-rot. I will investigate and add tests where possible. Thanks for testing!
Flags: needinfo?(kgrandon)
Comment on attachment 793685 [details]
Github pull request pointer

Hi Jose,

I think that the fact that the applying of the patch for this one was a bit confusing due to the multi-patch was causing some issues.

Now that the dependent bugs have landed, I have rebased this against master and it should be easier to apply. With the simplier patch I've checked that the white flash screen is not there, and that the gmail import shows the counter properly. Please verify and let me know if you have any issues. Thanks!
Attachment #793685 - Flags: review- → review?
@Jose - Actually I may have mus-interpreted your comment, but I did go through and find the statusMsg error you were talking about. The PR has been updated to reflect the necessary change. Thanks.
Comment on attachment 793685 [details]
Github pull request pointer

now the reported issues seem to have gone.
Attachment #793685 - Flags: review? → review+
https://github.com/mozilla-b2g/gaia/commit/f1b488cec4f1757c99fe3e2eb8a16b44ed2d287d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [c= p=3 s= u=] → [c= p=3 s= u=1.2]
Uplifted f1b488cec4f1757c99fe3e2eb8a16b44ed2d287d to:
v1.2: 33eabf186cd1d6980872502dfe7fa4104b5645b4
Target Milestone: --- → 1.2 C2(Oct11)
You need to log in before you can comment on or make changes to this bug.