843768 - [email] display something as soon as possible at startup

Status: VERIFIED FIXED

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

Comment 1

[Julien Wajsberg [:julienw]]

Right now, during email startup, a long time is spent to load lots of things without displaying anything to the user.

This bug is to display something as soon as possible: something that looks like the email app + a spinner, OR the setup form.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: tentative patch v1

This applies on top of vivien's email.fast branch. In addition to the following changes, the db version is set to 17 because that's the current in master and I had to do that to correctly test the different code paths.

I tested :
- old email app with an account, then new email app: slow to display the first card
- new email app without account: slow the first time, fast after
- new email app with account: fast if it was already launched once
- delete account, restart the app : fast

Other changes :
- store the number of accounts in localStorage as this is the fastest at startup
- have a migration path for existing users
- create a very minimal startup script that needs only some other js files
- body's background-color is white now
---
 apps/email/index.html                      |   19 ++---
 apps/email/js/account-number-helper.js     |   53 ++++++++++++++
 apps/email/js/ext/mailapi/maildb-worker.js |  105 ++++++++++++++++++++++++++++
 apps/email/js/ext/mailapi/maildb.js        |  105 ----------------------------
 apps/email/js/ext/mailapi/mailuniverse.js  |    2 +-
 apps/email/js/mail-app.js                  |   64 +++++++++++++----
 apps/email/js/mail-common.js               |   11 +--
 apps/email/js/mail-startup.js              |   70 +++++++++++++++++++
 apps/email/js/mailapi-worker-parent.js     |   22 ++++--
 apps/email/js/mailapi-worker.js            |    2 +-
 apps/email/js/message-cards.js             |   13 +++-
 apps/email/style/mail.css                  |    2 +-
 12 files changed, 323 insertions(+), 145 deletions(-)
 create mode 100644 apps/email/js/account-number-helper.js
 create mode 100644 apps/email/js/ext/mailapi/maildb-worker.js
 delete mode 100644 apps/email/js/ext/mailapi/maildb.js
 create mode 100644 apps/email/js/mail-startup.js

Comment 3

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

Thanks for providing the link to Vivien's email.fast repo; I wasn't aware that it existed before now.  I've updated bug 814257 to be assigned to Vivien.

(Proposed enhancement) bug 814267 is also relevant to the changes here; Kevin Grandon had also done some work along these lines, experimenting with using localStorage to latch the information from the back-end.


In terms of use of localStorage, is it already spun-up and primed in our pre-loaded process, or is it something that gets initialized on demand?  I know the perf team was doing work to pre-load localStorage for pages so the sync API issue went away, but I don't know if that landed or is on b2g18 or what not.  Specifically, if we don't get that for free, I was thinking using it to prime our IndexedDB would be better.


Otherwise, and this may be a question for bug 814257, what's the game-plan for issuing a pull request against the repo's where the code actually lives?  For reference, vivien's email.fast branch and these changes touch code in apps/email/js/ext/* that are sourced from the following repo's.

https://github.com/mozilla-b2g/gaia-email-libs-and-more
https://github.com/asutherland/bleach.js
https://github.com/mozsquib/jsas
https://github.com/mozsquib/jswbxml

Those repos have their own unit tests that need to pass; very extensive ones in the case of gaia-email-libs-and-more.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: tentative patch v2

fixes tests

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Andrew Sutherland (:asuth) from comment #3)
> (Proposed enhancement) bug 814267 is also relevant to the changes here;
> Kevin Grandon had also done some work along these lines, experimenting with
> using localStorage to latch the information from the back-end.
> 
> 
> In terms of use of localStorage, is it already spun-up and primed in our
> pre-loaded process, or is it something that gets initialized on demand?  I
> know the perf team was doing work to pre-load localStorage for pages so the
> sync API issue went away, but I don't know if that landed or is on b2g18 or
> what not.  Specifically, if we don't get that for free, I was thinking using
> it to prime our IndexedDB would be better.

I tried various things: directly querying the existing IDB, using asyncStorage (so another DB or objectstore), and using localStorage. localStorage was by far the fastest. I didn't take time to find why and if IDB could be improved.


For the other questions I'll let :21 answer.

Comment 6

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

(In reply to Julien Wajsberg [:julienw] from comment #5)
> I tried various things: directly querying the existing IDB, using
> asyncStorage (so another DB or objectstore), and using localStorage.
> localStorage was by far the fastest. I didn't take time to find why and if
> IDB could be improved.

Excellent! Do you have any numbers in terms of time-to-load among the various options or profiler runs that you can upload to cleopatra?  I really want to try hard to make sure we're not pushing out the showing-messages part of things, or at least understand what, if anything is getting pushed out, and the profiler run should show what else was happening while the localStorage spin was (or was not, if we're lucky ;) happening.  If you don't but you could easily do a run or two (especially if you have disabled elfhack in your build), I'd really appreciate it!

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: tentative patch v3

works on top of current vivien's email.fast + reuse the message card if it's already loaded.

will now try to work on a simpler patch on top of current master

Comment 8

[Julien Wajsberg [:julienw]]

Attachment: master patch v1

- add defer and lazy loading
- store the number of accounts in localStorage as this is the fastest at startup
- have a migration path for existing users
- create a very minimal startup script that needs only some other js files
- body's background-color is white now
---
 apps/email/index.html                         |   84 +++------------
 apps/email/js/account-number-helper.js        |   53 +++++++++
 apps/email/js/ext/mailapi/same-frame-setup.js |    5 +-
 apps/email/js/mail-app.js                     |  104 ++++++++++++------
 apps/email/js/mail-common.js                  |   11 +-
 apps/email/js/mail-startup.js                 |  144 +++++++++++++++++++++++++
 apps/email/js/message-cards.js                |   13 ++-
 apps/email/style/mail.css                     |    2 +-
 8 files changed, 306 insertions(+), 110 deletions(-)
 create mode 100644 apps/email/js/account-number-helper.js
 create mode 100644 apps/email/js/mail-startup.js

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Andrew Sutherland (:asuth) from comment #6)

> Excellent! Do you have any numbers in terms of time-to-load among the
> various options or profiler runs that you can upload to cleopatra?  I really
> want to try hard to make sure we're not pushing out the showing-messages
> part of things, or at least understand what, if anything is getting pushed
> out, and the profiler run should show what else was happening while the
> localStorage spin was (or was not, if we're lucky ;) happening.  If you
> don't but you could easily do a run or two (especially if you have disabled
> elfhack in your build), I'd really appreciate it!

It was with debug console logs with Date.now().. my current patch has such logs still so you might want to change some code and test by yourself.

I think the use of the main indexed db was about 600ms and asyncstorage 300 ms more than the local storage solution (just remembering numbers so the real numbers might be different, but this is the order of magnitude).

Comment 10

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

I just asked on #perf about the localStorage situation with taras and friends.

The information seems to be:
- LocalStorage is remoted back up to the parent.  This could be one of the reasons why things load faster for you since one localStorage is initialized in the parent it will stay initialized.  So even though e-mail might pay the price on its first load, you won't pay the price on the subsequent load if you kill it.
- "18 has some really bad LS issues" "if one tab is writing stuff to LS" "a flush is scheduled" "and all readers block"
- Cookies may be a viable alternative: "cookies are preloaded" "on b2g" "jduel did some hacks there" "and there are generally not too bad after the db has been opened"

The general opinion seemed to also be that using localStorage is a very bad idea because it is blocking and has all those interaction issues if any other apps use it too.  There are also the cargo culting issues to think about.

James Burke was taking a look at how to integrate the excellent strategy of this bug with the phased loading from bug 814271, and so I've asked him to take a quick look at using a cookie instead.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Gaia patches that include localstorage get automatic r- ;).

Comment 12

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

James Burke's quick tests via IRC indicated that the cookie solution was 105ms faster (995ms versus 1100ms) and 100% less evil than the LocalStorage solution.  Statistical sample size not huge at this point, but it looks good.

Key patch bits:
https://github.com/jrburke/gaia-email-libs-and-more/compare/noaccount
https://github.com/jrburke/gaia/compare/noaccount#diff-56

Only real glitch right now is the cookie is being set from the back-end when it will need to be set from the front-end.


If we combine that patch with Julien's work here to also cause us to display the message card before we really load anything, I think we could have a very nice perceptual win.  With the phased offline/online stuff, we can then get the messages in next.  And once we get Vivien's worker work ported over, the main thread can be much more responsive while we do the sync in the background.

Comment 13

[James Burke [:jrburke]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/129

Pointer to Github pull-request

Comment 14

[James Burke [:jrburke]]

Comment on attachment 717432 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/129

This pull request for gaia-email-libs-and-more creates a mock mailAPI for the front end and then dynamically loads the same-frame-setup layer in the background if no accounts have been configured. Uses a cookie to indicate if the backend has accounts. r=asuth in IRC review session.

Comment 15

[James Burke [:jrburke]]

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

Pointer to Github pull-request

Comment 16

[James Burke [:jrburke]]

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

Email: If no account configured, give UI a fake mailAPI and dynamically load same-frame-setup.

same-frame-setup is now a rollup layer for faster dynamic loading. r=asuth in IRC review session.

Comment 17

[James Burke [:jrburke]]

There is still more to do beyond the patch above -- namely to port over Julien's insertion of a card even when there is an account, and still dynamically load the back end in that case. However, I was having trouble sorting out the activityCallback interaction in that case, since it could have work to do. Once that part is ported, repeated use after account setup should be even better.

Comment 18

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

Comment on attachment 717200 [details] [diff] [review]
master patch v1

Review of attachment 717200 [details] [diff] [review]:
-----------------------------------------------------------------

I think the lazy loading aspects of this patch are mooted by James' patch that we landed, but I think we definitely still want the show-the-message-list logic.  I've limited my comments to those bits.  The general cleanup stuff like adding 'use strict' and wrapping us inside standard scope guards are of course fine and can stay.

::: apps/email/js/mail-app.js
@@ +10,5 @@
> +
> +(function(window) {
> +
> +var DEBUG = true;
> +function debug() {

I think this would want to go in mail-common instead of mail-app.

@@ +119,5 @@
>              dieOnFatalError('We have an account without an inbox!',
>                  foldersSlice.items);
>  
> +          // remove all existing Cards
> +          Cards.removeCardAndSuccessors(null);

this should be called with a second argument of 'none'.

@@ +132,5 @@
>                curFolder: inboxFolder
>              });
> +
> +          try {
> +            Cards.tellCard(

It seems like this is going to either reliably throw or silently fail since you called Cards.removeCardAndSuccessors above.  All that would exist is the folder-picker you just added.

You should be able to not remove the existing message-list card and then just pass 'left' as an additional argument and it should insert this card to that card's left, at least conceptually.  You might need to then call moveToCard with a showMethod of 'immediate', but I think you can probably get away without doing that because of the DOM's inherent default z-ordering.

::: apps/email/js/mail-common.js
@@ +610,5 @@
>        // adjusted the z-index (and we are definitively clearing all cards).
>        this._zIndex = 0;
> +
> +      // forcing showMethod to make the next code happy
> +      showMethod = 'none';

As noted at your call-site, the caller should pass this.  (I do agree that in the case of the value null, 'none' is probably the only sane value, but a more convincing comment is required in that case ;)

::: apps/email/js/message-cards.js
@@ +171,2 @@
>      this.showFolder(args.folder);
> +  } else {

I think general mozilla convention is not to brace single-line if's and else-if's, although maybe our JS coding guidelines is changing this? (or maybe you had debug stuff in there that got removed?)

@@ +324,5 @@
>     * we did nothing because we were already in the folder.
>     */
>    showFolder: function(folder, forceNewSlice) {
> +    if (folder === null) {
> +      this.onStatusChange('syncblocked');

This is a pretty good hack!  A comment is in order, however.  Like:
// Null is used when starting up the app in order to provide some UI while the back-end loads.

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 717200 [details] [diff] [review]
master patch v1

Review of attachment 717200 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/email/js/mail-app.js
@@ +119,5 @@
>              dieOnFatalError('We have an account without an inbox!',
>                  foldersSlice.items);
>  
> +          // remove all existing Cards
> +          Cards.removeCardAndSuccessors(null);

I will remove this call anyway (see below).

@@ +132,5 @@
>                curFolder: inboxFolder
>              });
> +
> +          try {
> +            Cards.tellCard(

> It seems like this is going to either reliably throw or silently fail since you called Cards.removeCardAndSuccessors above.
> All that would exist is the folder-picker you just added.

Oops right, using |removeCardAndSuccessors| was my first try and I forgot to remove it afterwards.

::: apps/email/js/mail-common.js
@@ +610,5 @@
>        // adjusted the z-index (and we are definitively clearing all cards).
>        this._zIndex = 0;
> +
> +      // forcing showMethod to make the next code happy
> +      showMethod = 'none';

Acually, I believe this should be "none" when either removing all cards, or removing the last card. Otherwise the code breaks afterwards.

::: apps/email/js/message-cards.js
@@ +171,2 @@
>      this.showFolder(args.folder);
> +  } else {

Well, I actually had debug stuff, and I like braces everywhere (which makes it easy to add debug stuff BTW).

Still I agree this is a bad practice to change code merely to change the style so I'll revert this change.

@@ +324,5 @@
>     * we did nothing because we were already in the folder.
>     */
>    showFolder: function(folder, forceNewSlice) {
> +    if (folder === null) {
> +      this.onStatusChange('syncblocked');

ok

Comment 20

[James Burke [:jrburke]]

Julien, since the patch that landed may have made it troublesome to port this last part of your patch for this ticket, I am willing to do the rest of the work to port the change. However, if you already have the patch in-progress or want to finish it out, I will leave it to you.

Comment 21

[Julien Wajsberg [:julienw]]

I didn't have the time to work on this today, please go ahead if you have the time today, otherwise I definitely planned to do it tomorrow.

Comment 22

[Julien Wajsberg [:julienw]]

Please tell here at the end of your day if you started to work on this after all. If I see no comment tomorrow then I'll work on this :)

Comment 23

[James Burke [:jrburke]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/132

Pointer to Github pull-request

Comment 24

[James Burke [:jrburke]]

Comment on attachment 718251 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/132

gaia-email-libs-and-more change for "show blank card when account is configured but not loaded yet". Gaia pull request that includes this change is coming next.

Comment 25

[James Burke [:jrburke]]

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

Pointer to Github pull-request

Comment 26

[James Burke [:jrburke]]

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

Gaia patch for "show blank card when account is configured but not loaded yet", finishes out the port of Julien's patch to the fake account bootstrapping. Includes the changes done in already-attached patch for gaia-email-libs-and-more.

Comment 27

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

Comment on attachment 718251 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/132

r=asuth, needs gaia/master landing approval from vingtetun

Comment 28

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

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

r=asuth, needs gaia/master landing approval from vingtetun

All gaia-email-libs-and-more unit tests maintain green-ness with these patches.

Start time for already possessing an account per time-to-load UI is ~1150 ms.  Sometimes I see the blank card 'assemble' as different parts of the header show up, etc.  This suggests there is additional profiling to be done; we might be taxing the device heavily enough during startup that image loading is getting pushed out, etc.

I verified that nothing terribly bad happens if we hit the 'folder list' button before we have created the card.  We fail to find the card and throw, which is desirable since the alternative is that we trigger "event eating" and the app becomes unresponsive forever.

Comment 29

[Julien Wajsberg [:julienw]]

(In reply to Andrew Sutherland (:asuth) from comment #28)
> 
> Start time for already possessing an account per time-to-load UI is ~1150
> ms.  Sometimes I see the blank card 'assemble' as different parts of the
> header show up, etc.  This suggests there is additional profiling to be
> done; we might be taxing the device heavily enough during startup that image
> loading is getting pushed out, etc.

In my previous patch I took great care to the ordering of the js files so that only the minimum was loaded to show the first screens. That's also why I added a startup.js file.

Maybe this could be done with the current code in a future bug ?

Comment 30

[Julien Wajsberg [:julienw]]

For example end.js could be put way before in index.html, afaik it only needs: l10n.js, mail-common.js, message-cards.js, and maybe lazy-loader.js (?).

also you should definitely put defer on all these scripts ! I didn't see that you didn't do that in the previous patch... That would definitely help a lot !!

Comment 31

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

We can even do that on this bug!

It looks like the startup.js involving patch loaded:
- mail-common.js
- message-cards.js
- account-number-help.js (mooted by the stuff in end.js for early load)
- shared/l10n.js
- mail-startup.js

With this patch, we are currently at:
- shared/l10n.js (needed, same as above)
- shared/l10n_date.js (can postpone)
- gesture_detector.js (can postpone)
- input_areas.js (newly added, needed for the account setup card, so it needs to be a lazy load dep if we do account setup, but can otherwise be deferred)
- console-hook (we can remove this since we also want to take out all our console.log stuff, sad as it is)
- mail-common.js (needed, same as above)
- message-cards.js (needed, same as above)
- folder-cards.js (can postpone)
- ext/alameda.js (it does seem like this can be postponed; I think it can detect an existing require.config maybe to handle letting end.js provide some logic to fire when that does happen?)
- ext/end.js (needed)
- lazy_loader.js (we certainly don't want another loader like startup.js added, but I think this might be able to be slightly postponed?)
- mail-app.js (can be slightly postponed?)


Julien, can you elaborate exactly on what the effects of adding 'deferred' are on the JS in terms of when we paint, when we load those deferred libraries?  If you have a cleopatra profile that helps make the side-effects more clear, that would be fantastic

Comment 32

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

Also, maybe to reduce overhead/interaction complexity we might just want to use the alameda loader for our dynamic lazy loading stuff that the DOM itself isn't right to handle.  At least if it can load our un-AMD-shimmed files (I think it can since we generate load events in geceko?) so we're not trying to coordinate the interaction of two lazy loaders.

Comment 33

[Julien Wajsberg [:julienw]]

(In reply to Andrew Sutherland (:asuth) from comment #31)
> We can even do that on this bug!
> 
> It looks like the startup.js involving patch loaded:
> - mail-common.js
> - message-cards.js
> - account-number-help.js (mooted by the stuff in end.js for early load)
> - shared/l10n.js
> - mail-startup.js
> 
> With this patch, we are currently at:
> - shared/l10n.js (needed, same as above)
> - shared/l10n_date.js (can postpone)
> - gesture_detector.js (can postpone)
> - input_areas.js (newly added, needed for the account setup card, so it
> needs to be a lazy load dep if we do account setup, but can otherwise be
> deferred)

Would there be a JS error if this is loaded like 1 or 2 seconds after the setup is displayed ?

> - console-hook (we can remove this since we also want to take out all our
> console.log stuff, sad as it is)
> - mail-common.js (needed, same as above)
> - message-cards.js (needed, same as above)
> - folder-cards.js (can postpone)
> - ext/alameda.js (it does seem like this can be postponed; I think it can
> detect an existing require.config maybe to handle letting end.js provide
> some logic to fire when that does happen?)
> - ext/end.js (needed)
> - lazy_loader.js (we certainly don't want another loader like startup.js
> added, but I think this might be able to be slightly postponed?)

Initially I added one in startup for 2 reason :
1 - I had to specify that we want javascript 1.8 ;-) lazy_loader doesn't do that afaik.
2 - I specified "async = false" so that the execution were keeping the order (as the defer attribute does, see below). I don't know if lazy_loader does that.

> - mail-app.js (can be slightly postponed?)
> 
> 
> Julien, can you elaborate exactly on what the effects of adding 'deferred'
> are on the JS in terms of when we paint, when we load those deferred
> libraries?  If you have a cleopatra profile that helps make the side-effects
> more clear, that would be fantastic

I have no cleopatra profile but you can find a nice schema at http://peter.sh/experiments/asynchronous-and-deferred-javascript-execution-explained/

Scripts without defer are particularly bad: the browser stops everything until the script is loaded and executed. This is synchronous.

Scripts with defer is quite nice: the browser donwloads it as soon as it sees the script tag, but execute it only when DOMContentLoaded fire, _keeping the defined order_ ! It stills delays the load event though (that's why lazy loading though JS can be useful).
So this is basically all win with no impact. The only case it can have an impact is if the script has to do something synchronous at the place it is inserted. This is not our case.

Scripts with async is not easy: the browser downloads it as soon as it sees the script tag, and executes it as soon as the download is finished, interrupting whatever he's doing at that moment. The order is not kept. IIRC the DOMContentLoaded event is delayed (and thus the load event too).

Inserting a script using javascript behaves like the async attribute, unless we explicitely set "async = false" on the script dom node (as I was doing in startup.js); in that case this behaves like defer and it keeps the order. Setting "async = false" doesn't make the insert synchronous though :-)

Another thing we do in Gaia: we automatically optimize the webapps. This means we take all scripts defined in index.html and we concatenate/minify them in 2 files: 1 for non-defered files, 1 for defered files. For now the build script can't have more than these 2 groups, that's why a JS lazy loading can be useful: to not have a big JS file wih everything in it. This also means that for now all the JS we're lazy loading will _not_ be concatenated/minified, but we can hope that if everyone is using the shared lazy_loader we might have soon an automatic support in the build system (or not ?).

I also remember that in my patch I decided to not display the first card in setTimeout. Using setTimeout made the "load" event fire in 800ms (instead of 1200ms iirc) but when not using the setTimeout the first card was still displaying in about 800ms and the feeling was better; but the load event fired in 1200ms :-) So maybe that's also part of your problem here, and as soon as we feel that the first card happens before the ~1150ms that's ok.

(In reply to Andrew Sutherland (:asuth) from comment #32)
> Also, maybe to reduce overhead/interaction complexity we might just want to
> use the alameda loader for our dynamic lazy loading stuff that the DOM
> itself isn't right to handle.  At least if it can load our un-AMD-shimmed
> files (I think it can since we generate load events in geceko?) so we're not
> trying to coordinate the interaction of two lazy loaders.

I don't know much about that so do as you want. But I think I'd prefer having 4 lines of JS to bootstrap alameda (which is 50kB unminified..) so that we can show something earlier.

Comment 34

[James Burke [:jrburke]]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> Scripts without defer are particularly bad: the browser stops everything
> until the script is loaded and executed. This is synchronous.

Without defer, multiple scripts should still be fetched concurrently, but then executed in order. The script tags prevent further processing of the page though.

> Scripts with defer is quite nice: the browser donwloads it as soon as it
> sees the script tag, but execute it only when DOMContentLoaded fire,
> _keeping the defined order_ ! It stills delays the load event though (that's
> why lazy loading though JS can be useful).
> So this is basically all win with no impact. The only case it can have an
> impact is if the script has to do something synchronous at the place it is
> inserted. This is not our case.
> 
> Scripts with async is not easy: the browser downloads it as soon as it sees
> the script tag, and executes it as soon as the download is finished,
> interrupting whatever he's doing at that moment. The order is not kept. IIRC
> the DOMContentLoaded event is delayed (and thus the load event too).

With async tags the DOMContentLoaded event is not delayed.

> Another thing we do in Gaia: we automatically optimize the webapps. This
> means we take all scripts defined in index.html and we concatenate/minify
> them in 2 files: 1 for non-defered files, 1 for defered files. For now the
> build script can't have more than these 2 groups, that's why a JS lazy
> loading can be useful: to not have a big JS file wih everything in it. This
> also means that for now all the JS we're lazy loading will _not_ be
> concatenated/minified, but we can hope that if everyone is using the shared
> lazy_loader we might have soon an automatic support in the build system (or
> not ?).

AMD tools get this minification & concatenation capabilities for free now. The most recent r.js optimizer runs in xpcshell too. Dependency tracing gives much finer grained capabilities for doing targeted, layered builds. It was used to speed up the email back end, by doing a set of layered builds with exclusions.

> (In reply to Andrew Sutherland (:asuth) from comment #32)
> > Also, maybe to reduce overhead/interaction complexity we might just want to
> > use the alameda loader for our dynamic lazy loading stuff that the DOM
> > itself isn't right to handle.  At least if it can load our un-AMD-shimmed
> > files (I think it can since we generate load events in geceko?) so we're not
> > trying to coordinate the interaction of two lazy loaders.
> 
> I don't know much about that so do as you want. But I think I'd prefer
> having 4 lines of JS to bootstrap alameda (which is 50kB unminified..) so
> that we can show something earlier.

Since the rendering depends on JavaScript, I would prefer to see your other bug land for triggering an "actually usable" event and doing measurements and profiling on changes once that is in.

It would be good to look again once that is available, but I do not believe the defer attributes will help that number because all the scripts still needs to load to show the UI, and it seems like defer will just game the load event. This is further blurred if the scripts will be concatenated. I would not expect to see a difference between the use of defer or async in that concatenated case, particularly with the CSS coming before the script tag(s). 

It would be interesting to compare the numbers for "concatenated but with script tag at the end of the body tag" case too.

I also prefer to see measurements and data to prove what impact using alameda and AMD actually has, particularly since it gives better code organization, finer grained build capabilities (alameda is much smaller once minified/no comments), and puts the code in a much better place to upgrade to ES modules later vs. the ad hoc approach being used now.

For discussion of the current pull request for this bug though, using the data tool that is most readily available:

I recorded the "time to load" green number in the upper left corner of the unagi device 5 times, using the current state of the patch with no "defer" attributes, and then 5 time with the "defer" attributes on all the script tags:

With one IMAP account configured:

Without defer:
1100
1134
1091
1130
1101

With defer:
1065
1052
1079
1071
1048

Average 1111 without, 1063 with defer.

With no accounts configured:

Without defer:
1048
1020
1007
1089
1019

With defer:
986
998
1069
1057
1039

Average 1036 without, 1029 with defer.

Sorry, I think there is a better statistical tool to use than average, but I do not know it off the top of my head.

Interesting: when I look in profile/webapps/email.gaiamobile.org/application.zip, it has the script tags separate (no concatenation apparently) and rewrites the defer tags as defer="".

Comment 35

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

(In reply to Julien Wajsberg [:julienw] from comment #33)
> > - input_areas.js (newly added, needed for the account setup card, so it
> > needs to be a lazy load dep if we do account setup, but can otherwise be
> > deferred)
> 
> Would there be a JS error if this is loaded like 1 or 2 seconds after the
> setup is displayed ?

As structured right now, mail-common can't actually push a card without dying without it.  But it's something we could check the presence of, skip if not present, and just have it do a fix-up when it gets loaded.  (Scoped to just the currently alive cards; we want to keep it out of our template nodes and/or remove those.)


It does sound like the lack of defer could be causing a 'pipeline bubble' during our (probably not too long) SSD I/O, but it was my understanding that our HTML5 parser ran async on a background thread unless we do something dumb like calling "document.write", so I do wonder how much of a stall it is.

In any event, I'll try and get profiler runs with marker augmentation built-in over the next few days/before next week's perf work week and see what's happening.  I had started on a simple cleopatra plugin a few weeks ago that should help reduce the need to eyeball some of this stuff, too.

Comment 36

[Julien Wajsberg [:julienw]]

(In reply to James Burke [:jrburke] from comment #34)
> (In reply to Julien Wajsberg [:julienw] from comment #33)
> > Scripts without defer are particularly bad: the browser stops everything
> > until the script is loaded and executed. This is synchronous.
> 
> Without defer, multiple scripts should still be fetched concurrently, but
> then executed in order.

What you say is actually true since a very recent time. I couldn't find since when it's true so are you sure this is true for our b2g18 branch ?

However without defer they still block the parsing of the page.

I believe that if we use a dependency loader, async could be used to load the bootstrap script. But we're maybe not there yet, and this could be for another bug.

> 
> > Scripts with defer is quite nice: the browser donwloads it as soon as it
> > sees the script tag, but execute it only when DOMContentLoaded fire,
> > _keeping the defined order_ ! It stills delays the load event though (that's
> > why lazy loading though JS can be useful).
> > So this is basically all win with no impact. The only case it can have an
> > impact is if the script has to do something synchronous at the place it is
> > inserted. This is not our case.
> > 
> > Scripts with async is not easy: the browser downloads it as soon as it sees
> > the script tag, and executes it as soon as the download is finished,
> > interrupting whatever he's doing at that moment. The order is not kept. IIRC
> > the DOMContentLoaded event is delayed (and thus the load event too).
> 
> With async tags the DOMContentLoaded event is not delayed.

Maybe I mixed with defer tags for this one but I remember I read that one of them delays this event.

> > Another thing we do in Gaia: we automatically optimize the webapps. This
> > means we take all scripts defined in index.html and we concatenate/minify
> > them in 2 files: 1 for non-defered files, 1 for defered files. For now the
> > build script can't have more than these 2 groups, that's why a JS lazy
> > loading can be useful: to not have a big JS file wih everything in it. This
> > also means that for now all the JS we're lazy loading will _not_ be
> > concatenated/minified, but we can hope that if everyone is using the shared
> > lazy_loader we might have soon an automatic support in the build system (or
> > not ?).
> 
> AMD tools get this minification & concatenation capabilities for free now.
> The most recent r.js optimizer runs in xpcshell too. Dependency tracing
> gives much finer grained capabilities for doing targeted, layered builds. It
> was used to speed up the email back end, by doing a set of layered builds
> with exclusions.

I don't mind what we use, I'm just saying what happens now.

> 
> > (In reply to Andrew Sutherland (:asuth) from comment #32)
> > > Also, maybe to reduce overhead/interaction complexity we might just want to
> > > use the alameda loader for our dynamic lazy loading stuff that the DOM
> > > itself isn't right to handle.  At least if it can load our un-AMD-shimmed
> > > files (I think it can since we generate load events in geceko?) so we're not
> > > trying to coordinate the interaction of two lazy loaders.
> > 
> > I don't know much about that so do as you want. But I think I'd prefer
> > having 4 lines of JS to bootstrap alameda (which is 50kB unminified..) so
> > that we can show something earlier.
> 
> Since the rendering depends on JavaScript, I would prefer to see your other
> bug land for triggering an "actually usable" event and doing measurements
> and profiling on changes once that is in.
> 
> It would be good to look again once that is available, but I do not believe
> the defer attributes will help that number because all the scripts still
> needs to load to show the UI, and it seems like defer will just game the
> load event. This is further blurred if the scripts will be concatenated. I
> would not expect to see a difference between the use of defer or async in
> that concatenated case, particularly with the CSS coming before the script
> tag(s). 
> 
> It would be interesting to compare the numbers for "concatenated but with
> script tag at the end of the body tag" case too.
> 
> I also prefer to see measurements and data to prove what impact using
> alameda and AMD actually has, particularly since it gives better code
> organization, finer grained build capabilities (alameda is much smaller once
> minified/no comments), and puts the code in a much better place to upgrade
> to ES modules later vs. the ad hoc approach being used now.


Really I don't want to enter this controversy again here. I was just saying what happens now, and so, what performance gain we can have now in a short timeframe.

 
> For discussion of the current pull request for this bug though, using the
> data tool that is most readily available:
> 
> I recorded the "time to load" green number in the upper left corner of the
> unagi device 5 times, using the current state of the patch with no "defer"
> attributes, and then 5 time with the "defer" attributes on all the script
> tags:
> 
> With one IMAP account configured:
> 
> Without defer:
> 1100
> 1134
> 1091
> 1130
> 1101
> 
> With defer:
> 1065
> 1052
> 1079
> 1071
> 1048
> 
> Average 1111 without, 1063 with defer.
> 
> With no accounts configured:
> 
> Without defer:
> 1048
> 1020
> 1007
> 1089
> 1019
> 
> With defer:
> 986
> 998
> 1069
> 1057
> 1039
> 
> Average 1036 without, 1029 with defer.

so... this is faster, this is easy, what are we waiting for ? a 5% gain (for the account case) is still good :)
Also, the gain will probably be bigger after the order reorganization.

> 
> Sorry, I think there is a better statistical tool to use than average, but I
> do not know it off the top of my head.

average is fine here (5 runs, no strange values). :)

> 
> Interesting: when I look in
> profile/webapps/email.gaiamobile.org/application.zip, it has the script tags
> separate (no concatenation apparently) and rewrites the defer tags as
> defer="".

You have to run "GAIA_OPTIMIZE=1 make install-gaia" to trigger the concatenation. This is done in our user builds.

I'd suggest only putting the defer attributes in this PR (easy, faster, no risk) and keep the other potential loading improvements for another time and another bug.

Comment 37

[Julien Wajsberg [:julienw]]

(In reply to Andrew Sutherland (:asuth) from comment #35)
> (In reply to Julien Wajsberg [:julienw] from comment #33)
> > > - input_areas.js (newly added, needed for the account setup card, so it
> > > needs to be a lazy load dep if we do account setup, but can otherwise be
> > > deferred)
> > 
> > Would there be a JS error if this is loaded like 1 or 2 seconds after the
> > setup is displayed ?
> 
> As structured right now, mail-common can't actually push a card without
> dying without it.  But it's something we could check the presence of, skip
> if not present, and just have it do a fix-up when it gets loaded.  (Scoped
> to just the currently alive cards; we want to keep it out of our template
> nodes and/or remove those.)

Well it's quite small anyway, let's keep it here.
 
> It does sound like the lack of defer could be causing a 'pipeline bubble'
> during our (probably not too long) SSD I/O, but it was my understanding that
> our HTML5 parser ran async on a background thread unless we do something
> dumb like calling "document.write", so I do wonder how much of a stall it is.

I admit I haven't kept up with recent ameliorations in how we render pages with script tags. However we saw real gains in using defer for other apps.

-- 
Julien

Comment 38

[James Burke [:jrburke]]

Added defer attributes to email app's index.html and rebased the pull request. I also updated the gaia-email-libs-and-more-jrburke pull request's copy script to write out the tags with defer attributes.

Tested and email app works as before, including clicking on the menu item on blank card, and was fine.

Also tried with GAIA_OPTIMIZE=1 as that is what the datazilla graphs use. The numbers for without defer and with defer:


New Account:
-----

Without defer: avg 947.4

968
906
984
883
996


With defer: avg 922

993
867
945
942
863

With one IMAP account configured:
-----

Without defer: avg 972

990
966
969
962
974


With defer: avg 947.8

981
928
925
965
940

Comment 39

[James Burke [:jrburke]]

(In reply to Julien Wajsberg [:julienw] from comment #36)
> (In reply to James Burke [:jrburke] from comment #34)
> > (In reply to Julien Wajsberg [:julienw] from comment #33)
> > > Scripts without defer are particularly bad: the browser stops everything
> > > until the script is loaded and executed. This is synchronous.
> > 
> > Without defer, multiple scripts should still be fetched concurrently, but
> > then executed in order.
> 
> What you say is actually true since a very recent time. I couldn't find
> since when it's true so are you sure this is true for our b2g18 branch ?

This blog post indicates firefox 3.5+ can do parallel script loading (I'm looking at the "|| Script Script" column:

http://www.stevesouders.com/blog/2010/02/07/browser-script-loading-roundup/

However, IIRC, standardization of that behavior was done some time after that blog post though.

Comment 40

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

Comment on attachment 718251 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/132

Vivien indicated his sign-off is not required.

landed on gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/132

Comment 41

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

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

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/8317

Comment 42

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

Marking this fixed because the recent patches have landed and there seems to be agreement all around that we should defer additional lazy loading to a subsequent patch.

Thanks very much to Julien and James!  At this rate, e-mail will start up in negative time soon!

Comment 43

[James Burke [:jrburke]]

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

Pointer to Github pull-request

Comment 44

[James Burke [:jrburke]]

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

Fast startup additions in 843768 invalidated the state used in the web activity. Changed the web activity path to use the MailAPI state instead. Also holds on to the web activity to complete if the user needs to first set up email.

Comment 45

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

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

r=asuth

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/8390

This also fixes bug 821494 about us discarding the activity if no account is yet created. There was a lot of overlap and testing synergy which is why I requested this complementary fix while discussing the bug via IRC.

Comment 46

[Julien Wajsberg [:julienw]]

mail perf improvements => tef? and tracking?

Andrew, could you please summarize in one comment all commits that finally landed on gaia master ?

Comment 47

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

(In reply to Julien Wajsberg [:julienw] from comment #46)
> Andrew, could you please summarize in one comment all commits that finally
> landed on gaia master ?

I've proposed in dev-gaia that we just land the contents of gaia/master for e-mail on the branches that want a more performant e-mail app.  I think that's easiest/best.  If we don't do that, I don't have any special list or tooling to tell me what landed other than using 'git log' on apps/email/.

Comment 48

[Julien Wajsberg [:julienw]]

Ah yes right I saw that. I quite agree with you but I'm afraid this won't be possible for v1.0.1...

Comment 49

[Lukas Blakk [:lsblakk] use ?needinfo]

If this can be safely uplifted into v1-train, go for it. Will leave the tef? up so partners can decide how to proceed for v1.0.1 though comment 48 doesn't bode well.

Comment 50

[Michael Vines [:m1] [:evilmachines]]

(donno about v1.0.1, does this do anything more than just display "something" sooner?  ie, any actual improvement to the time it takes for the email app to become usable after launch?)

Comment 51

[Julien Wajsberg [:julienw]]

Michael> In the "first launch" case, we can actually use the displayed form when it's displayed sooner. For the other cases, it does not help the "usable" time, but it still "feels" faster because the user sees something happening.

Comment 52

[Michael Vines [:m1] [:evilmachines]]

(tef+ because I suspect landing this patch will very much help us land real improvements to email app start time over the coming days, please lmk if this assumption is not quite right)

Comment 53

[Julien Wajsberg [:julienw]]

Andrew, it would help a lot for uplift if you could summarize in one comment all the commit hashes that landed in this patch.

Comment 54

[Julien Wajsberg [:julienw]]

This landed to v1.0.1 as part of Bug 851124 uplift and will land to v1.1 tomorrow at the latest.

Comment 55

[Julien Wajsberg [:julienw]]

Landed in v1-train as part of Bug 851124 big uplift.

Comment 56

[mdaniloff]

Verified fixed on V.1.0.1 and V.1.1 on Unagi
Email responds/loads quickly
Unagi Build ID: 20130329070203
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/56c922308fd1
Gaia: 0a9f78bffafda93a159c1f502e8b110c2f49a500

Comment 57

[mdaniloff]

Correction: V1 train
also updating tracking flags to: verified