837836 - [Gaia::E-Mail] Consider lazy loading DOM elements

Status: RESOLVED DUPLICATE of bug 886446

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Kevin Grandon :kgrandon]

We load a ton of DOM for stuff that isn't used initially (setup, settings, probably some other nodes). We might be able to see a slight improvement if we lazy load DOM nodes which we don't need initially. There are potentially two methods to doing this:

1 - Templates through js files (Similar to what Calendar does)
2 - Commented out sections, similar to what Settings does.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

I don't think we want to change-over to JS templating at this time since we've got an idiom and I'd rather switch once to web components when that gets here rather than twice.

Does the localization pre-population stuff work with the settings app?

And of course, ballpark numbers would be great for this.  Since our HTML is only ~1000 lines and our aforementioned idiom has our template nodes live under display: none nodes, we at least shouldn't be paying layout costs.

Comment 2

[Kevin Grandon :kgrandon]

I will try to get some ballpark numbers today.(In reply to Andrew Sutherland (:asuth) from comment #1)
> I don't think we want to change-over to JS templating at this time since
> we've got an idiom and I'd rather switch once to web components when that
> gets here rather than twice.
> 
> Does the localization pre-population stuff work with the settings app?
> 
> And of course, ballpark numbers would be great for this.  Since our HTML is
> only ~1000 lines and our aforementioned idiom has our template nodes live
> under display: none nodes, we at least shouldn't be paying layout costs.

I will profile and get some performance numbers so we can decide if it's worth switching over earlier. The localization stuff should continue to work if we follow what settings has done.

Comment 3

[Kevin Grandon :kgrandon]

So I commented out most of the elements that were not accessed during startup, and the gains appear to be minimal (20-30ms or so from my testing).  I really dislike what it does to the HTML doc, so I think it's best if we skip that for now. I'm still going to use this bug to track improvements to the initial work that we do to load up the templates.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Thanks for running the numbers; I agree that commenting out swathes of HTML without a huge payoff is undesirable.  It sounds like you're talking about deferring some of our processing of the nodes, which does seem like it could save us some cycles without getting too ugly.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon from comment #3)
> So I commented out most of the elements that were not accessed during
> startup, and the gains appear to be minimal (20-30ms or so from my testing).
> I really dislike what it does to the HTML doc, so I think it's best if we
> skip that for now. I'm still going to use this bug to track improvements to
> the initial work that we do to load up the templates.

That seems weird that it save only 20-30ms. On Settings which was 2000 lines of code at the time where this stuff has been done it was taking 1s for the parser to read it - display: none saves layout but it was still taking forever to parse the doc. So I would have expect a better gain here. Could it be that something else hides the win?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Taking 1sec to parse 2000 lines of HTML is nuts though; did anyone actually use a profiler with valid native backtraces to figure out what was actually slow about that?  (ex: with --disable-elf-hack, although things might have been otherwise broken at that time anyways.)  Or check with Henri Sivonen?

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Andrew Sutherland (:asuth) from comment #6)
> Taking 1sec to parse 2000 lines of HTML is nuts though; did anyone actually
> use a profiler with valid native backtraces to figure out what was actually
> slow about that?  (ex: with --disable-elf-hack, although things might have
> been otherwise broken at that time anyways.)  Or check with Henri Sivonen?

The profiler was barely running at that time and since we were in the rush we did not opened a bug :(

Comment 8

[[:fabrice] Fabrice Desré]

Also, I would not wait on improvements in the HTML parser to do something here.

Comment 9

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

This is a halfway patch which provides an interface so we can lazy-load the DOM nodes if we wish to. There also should be slight performance gains as it will be doing less work at startup. (Not querying for unused elements for example)

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

This looks very reasonable.  My main concern is that the lookup mechanism being dependent on a function call loses us the potential of the shape lookup being cached at each usage location.  It was speculative, but that's why I structured it that way.

Since our implementation is already file-centric based on card family for on-demand JS/CSS loading, and the DOM tree is also sliced in that fashion, it seems like we could accomplish on-demand DOM loading by throwing that in there too.  Even now, our setup cards could require 'dom/sup' which would trigger: "supNodes = processTemplNodes('sup');"

But all that really matters is the startup number to usability (including message list population), so we probably want to add that performance event that was requested in conjunction with the always-refresh patch that should land soon, then we can run with this and check what it does to the number and take it or not.  If the startup aspect improves but the overall impact's not great, we can try on-demand loading with processTemplNodes instead.

Comment 11

[Kevin Grandon :kgrandon]

Attachment: Email profile current

Comment 12

[Kevin Grandon :kgrandon]

Attachment: Email profile lazy loaded DOM

Comment 13

[Kevin Grandon :kgrandon]

I've update the pull request to include full DOM lazy loading as an experiment, and attached two profiles. One with the current state of master, and one after this pull request. Most of the heavy lifting gets shifted over to the left by about ~70 ticks, I'm not sure what this means for the overall performance picture and will continue to look.

Comment 14

[Kevin Grandon :kgrandon]

Gabriele - Here are two profiles I'm currently experimenting with. The latter is the result of some work I'm doing to lazy load the DOM nodes. I'm just trying to decipher this as adding manual timing hooks to the email UI is more complex than calendar, and results widely vary from load to load. I'm wondering:

A - Am I taking the profile correctly?
B - Do you see much improvement in the second profile?

Thanks!

Current state of email: http://people.mozilla.com/~bgirard/cleopatra/#report=994de37dcd88065018dd66b2259b9824b8d9e778

Email with DOM lazy loading: http://people.mozilla.com/~bgirard/cleopatra/#report=86d29e13a47bd7fae38d938cfed01d44cd6aabb0

Comment 15

[Gabriele Svelto [:gsvelto]]

(In reply to Kevin Grandon from comment #14)
> A - Am I taking the profile correctly?

Yes, the profiles look fine. However I would suggest to wait a bit before launching the applicaiton when taking a profile so that the prelaunched process can settle down before starting to work again. This adds a long stretch before the app launches where only the nsAppShell::ProcessNextNativeEvent::Wait() can be seen and makes it easy to spot where the application is starting. In your second profile I have a hard time spotting where the application is starting so I can't measure startup time very precisely.

> B - Do you see much improvement in the second profile?

I think there's a drop but since I can't really tell apart the startup time in the second profile I cannot be sure. Can you take another profile making sure that you wait 10-15 seconds with the phone idle before starting the app?

Additionally I see that there's now 3 distinct sync reflows that are being triggered by JS code during startup:

- once in _hideSearchBoxByScrolling() which we already had discussed in the parent bug

- once again in onMessagesSplice(), looking at the code there's many places where this could be triggered (many lookups to .offsetTop, .clientHeight, etc....)

- the last one is caused by onScroll() being called by onSliceRequestComplete()

Ideally we should have only one reflow during startup when everything is ready on the page however I'm not sure if it's possible to achieve that effect here.

Comment 16

[Kevin Grandon :kgrandon]

Patch no longer applies, and it appears we don't really want to go down this path. Closing for now.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Startup time still matters to us, and we've taken care of most of the other low hanging fruit, so this is still interesting, so no need to wontfix.  James Burke got some numbers recently, available at:

https://gist.github.com/jrburke/70363a7bd77ef92ddeb5

Key point was a 55ms improvement which is meaningful in absolute terms.  James is looking at the performance atom stuff too, so especially once we have that, it should be much easier to see the win on this.

Comment 18

[James Burke [:jrburke]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9258

Pointer to Github pull-request

Comment 19

[James Burke [:jrburke]]

Comment on attachment 738913 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9258

Picks up @KevinGrandon's original lazy DOM approach, but updated for latest master. Also removes dynamic loader used for back end code since that non-worker code is now just a single file. Combining these changes for expediency and ease of uplift.

Related GELAM pull for the GELAM code: mozilla-b2g/gaia-email-libs-and-more#184

Comment 20

[James Burke [:jrburke]]

Comment on attachment 738913 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9258

This patch is obsolete now, some of it landed, will need to redo the main part of the patch that does the actual lazy DOM loading bit.

Comment 21

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

This is a rebased version of my original one against latest master.

Comment 22

[Kevin Grandon :kgrandon]

Running performance test against today's master build, I'm seeing similar gains.

With lazy dom nodes:    "mozPerfDurationsAverage": 930.6
Without lazy dom nodes: "mozPerfDurationsAverage": 982.4

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

We're planning to wait on the lazy loading until we've got our datazilla numbers in the tree for actual time-to-messages.  Currently leaning towards jrburke's alternate implementation that hosts the lazy HTML in separate files rather than commented out HTML (but which can get inlined into the JS as strings for performance reasons).  We'll evaluate the various approaches at that time, which should be in the next few days.

Comment 24

[Kevin Grandon :kgrandon]

That sounds like a good approach to consider. We actually do something similar in Calendar where we lazy load templates when needed. I would suggest that it might be worthwhile to look at abstracting the calendar code we have and move it to shared/.

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 744895 [details]
Github pull request pointer

We're going with :jrburke's more aggressive re-factoring of the front-end to load HTML from separate files and persist the HTML structure to cookies (rather than persisting database state).

Comment 26

[Kevin Grandon :kgrandon]

Sounds good, looking forward to that approach!

I'm going to close this out as I assume you have a bug for that?

Comment 27

[Mike Lee [:mlee]]

(In reply to Kevin Grandon :kgrandon from comment #26)
> I'm going to close this out as I assume you have a bug for that?

Kevin,
Let's make sure we have a bug for James' work before closing this.

James and Andrew,
Please dup this bug against the one being used to track James' work.

Comment 28

[James Burke [:jrburke]]

Right, thanks for the reminder, just created a bug that summarized the results from the email mini-work week in 886446.