Closed Bug 886446 Opened 9 years ago Closed 9 years ago

[email] Partition card loading, cache initial card HTML for fast startup

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jrburke, Assigned: jrburke)

References

Details

(Keywords: perf, Whiteboard: c= ,)

Attachments

(2 files)

At the most recent email mini-work week, the following was worked out for improving startup and code code maintance for the email app:

1) partition card code into discrete modules, with associated HTML
2) Only load specific card JS/HTML when needed, instead of loading an aggregation of cards.
3) Cache the contents of initial cards to use in fast restore.
4) Use a Makefile for the email app that does smarter optimization that just combines the cards used for startup.

With those items: 

1 will reduce the large DOM that is in index.html now, and make it easier to work on modifications related to just one card. Since the HTML is in its own file, syntax highlighting works.

2 will reduce overall

3 results in noticeably faster display of card content, under a second for initial card display. There is still a longer delay before the card has full capabilities, but the user can see the most recent messages and scroll through the message list of most recent 7 messages (based on last sync).

By caching the HTML instead of the last data structures passed from the worker, it also removes some complexity in the data layers around the previous startup optimization of using "fake" data.

4 will allow this more modular structure but still gain the benefits from concatenation.
Duplicate of this bug: 837836
Oops, dropped a comment: 2 will reduce overall amount of HTML/JS needed at a given time for the cards. Before, a few card JS classes were loaded up front for a given card area.
Keywords: perf
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: c= ,
James, are you not set as the assignee for this bug because you're not actively working on it? If so, any idea when this'll be worked on and who'll do it?
Assignee: nobody → jrburke
Thanks, forgot to set assigned. I should have a pull request up by tomorrow.
Status: NEW → ASSIGNED
Comment on attachment 767379 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/222

The GELAM pull request is part of the changes for the front end, gaia pull request will be attached soon:

For mailapi.js: with the HTML cache, the "_fake" data on the MailAPI is no longer needed.

For main-frame-setup.js: Removed a timeout in the wiring up of the worker, since now the HTML cache allows us to do code setup as it makes sense, without having to game startup times.

For copy-to-gaia.js: all the scripts in the email app's index.html are loaded via JS module APIs, so no need to modify the HTML source any more.
Attachment #767379 - Flags: review?(bugmail)
Comment on attachment 767389 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10614

For the Gaia pull request:

1) splits the cards and their associated HTML into separate files. Uses modules and a module loader plugin to associate the HTML to the card JS.

2) the card implementation in mail_common.js now dynamically loads the next card. Cards can implement a `nextCards` array property if the card wants to preload likely next cards. However, the message_list card does not use it to keep up-front load costs smaller.

3) html_cache_restore.js and html_cache.js are used to store a cache of the card HTML that should be used on startup. html_cache_restore.js is the only JS file in the index.html now, to make insertion of the cached content (from cookie storage) as fast as possible. It then dynamically appends the built/mail_app.js file that starts the main JS logic loading.

The HTML cache for a message_list seems to hover around 16KB across 9 cookie segments. With a configured account, it is common for the "time to load" timing to be in the 700-800ms range. Clicking on the UI does not do anything until the back end fully starts up, but the user can scroll through the list of messages while waiting.

4) the email app now has its own Makefile to handle the concatenation of modules and inlining of startup card HTML templates. 

There were concerns about dynamically appended CSS causing reflows, so the Makefile also now combines all the CSS into one built/mail.css file. This approach could be scaled back, and use the css.js loader plugin to dynamically load CSS if need be, but the startup feels very fast now, so it may not be needed.

For mail_app.js:

* the firstAccountLoad flag was used because the message slice's atTop does not seem to be true on the very first sync. Without this change, then the user would see the cache of the New Account screen on first cold restart after configuring an account.

* Activity work is delayed until there is a confirmed push for a new card. Since card loading can be async now, it was possible that activity notification could be triggered before the card it uses to detect a configured account showed up in the DOM.

NOTE:

After merging this change, you will likely need to re-run `make install-git-hook` as it changed, to fix an issue with it not correctly using the excluded files list.
Attachment #767389 - Flags: review?(bugmail)
(In reply to James Burke [:jrburke] from comment #8)
> After merging this change, you will likely need to re-run `make
> install-git-hook` as it changed, to fix an issue with it not correctly using
> the excluded files list.

I filed bug 886971 so this can be landed separately.  I'll put up a patch on that in a second.
(review ongoing)

For text.js, a notable FYI is that xhr.send() can regrettably throw an exception for us when we are a packaged app *if the file does not exist*.  This is a problem for us in autoconfig where we do speculative lookups, but shouldn't be a problem elsewhere.  No change is required, just be aware in case we see breakage.


It looks like l10n.js is a plug-in so that the module won't resolve until mozL10n is ready.  This seems like a good/clever idea because it avoids the modules needing to wait on a promise or something once they load, but I think it merits a comment.  Also, maybe we should fast-path checking readyState ( === 'complete') on navigator.mozL10n.  Specifically, its ready() helper uses setTimeout if it's already complete, and it seems a shame to push out loads for no good reason.


You copy mail-app_test.js to mail_app_test.js but don't delete the original.  Note: I haven't run the unit tests yet.


HTML persistence: The 'search mail' teaser is visible when we load the persisted HTML.  It disappears once the rest of the UI loads.  We probably should just nuke the teaser's DOM node from existence for persistence since it's useless until hooked up and we already know there's a perf issue with manually adjusting that scroll offset.


Desired readme notes.  What we're doing may not be totally obvious, so I think we should call out the following, possibly just by pasting some of the following in (assuming they are correct):

- The js/*_builder.js are only run at build-time; do not worry about their size or that they support various runtimes.

- Standard gaia build optimization: All standard gaia optimization steps are run against the e-mail app, but most of them end up doing nothing or very little.  Because of our cached HTML trick, we only load one JS file at startup, so the concatenation and minification will happen but doesn't do much.  The logic that puts the l10n dictionaries in the file will still run, but our index.html does not contain any application HTML nodes (see next section), so pre-localization does not happen.

- Localization: HTML nodes are localized by mozL10n as they are loaded by the 'tmpl' plugin.  Because our 'nextCards' mechanism allows us to pre-load and pre-localize they are needed, this does not affect our latency tremendously.  However, a future enhancement could be for the build step to pre-localize l10n like webapp-optimize.js does.
For apps/email/Makefile, I assume the intent is to err on the side of doing more work than is required since our Makefile will only be invoked for the $(PROFILE_FOLDER) target in gaia/Makefile and that's a rare thing.

As such, I think the following actions should be taken:

- The default target, "all", should be marked .PHONY by placing ".PHONY: all" on the line before.  This avoids things breaking if someone foolishly creates an "all" file in the directory.  The same should also be done for clean and update_shared.

- The shared JS files should be made into dependencies so that we rebuild built/mail_app.js if the shared files change.  The easiest way to deal with this is probably to change 'update_shared' to also pass the '-p' flag so that it preserves timestamps (to try and avoid spurious rebuilds) and to add a SHARED_JS_SOURCES that looks like JS_SOURCES but for shared.  The net result is that we will always blow away the shared sub-tree every time but might not rebuild mail_app.js.  If we were confident about having rsync available, we could avoid updated_shared being so aggressive, but I don't think that matters.
typo in apps/email/js/cards/settings_account_credentials.js:
  var templateNode = require('tmpl!./setting_account_credentials.html')
s/setting/settings/

I audited the rest and they seem fine.  Also, the UI screens seem to work fine on the device.


Our GAIA_OPTIMIZE=1 zip file has too much stuff in it, but I view that more as a Gaia build system deficiency.  Let's spin-off a bug to improve our situation there to work as a high-priority follow-up.
I got:

ERR: onerror reporting: Error: Load failed: shared/mime_mapper: undefined @ app://email.gaiamobile.org/built/mail_app.js : 843

It appears that mail_app does not trace mime_mapper.js and we don't list it in index.html so it doesn't get loaded.
I think we should clear ignoreAtTopForCache after we persist something once.  Specifically, on my first use, I was able to scroll way down and besides being super-aggressive about saving the cache state repeatedly, when I next loaded the app I was presented with messages a week old.

We should also file 1 or 2 follow-up bugs to address the following:
- Optimize the cache saving logic so that we only trigger the saving logic if the splice actually affected the top 7 messages.  Right now we're going to tend to write to the cookies far more often than is required.

- Have the onchange notification for the slice (currently hooked up to updateMessageDom) trigger the cache saving if the index is in the top N.  We pass the index in to the logic, so this makes it very easy.)
Attachment #767379 - Flags: review?(bugmail) → review+
Comment on attachment 767389 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10614

Review done for now.  The load-time improvement is ridiculously amazing and I love me some refactoring/cleanup.  Major props.

I want to do another review pass once the action items are addressed.  As noted on IRC, we will either need to fix https://github.com/jrburke/r.js/issues/472 (thanks for filing!) or to fix xpcshell's stack problems (as opened up for discussion at https://groups.google.com/d/msg/mozilla.dev.platform/S2dt2xIkSbI/Q0S4656689EJ) and get gaia using a fixed xpcshell version there.
Attachment #767389 - Flags: review?(bugmail)
Oh, and I was unable to get the e-mail unit tests to run, but I am having trouble getting the unit tests to run on normal trunk anyways.  Both "make test-agent-test" and "make test-agent-test APP=email" just sit there for me on gaia/master and your branch.  James Lal, any ideas/can you run the tests?  Also, you should probably take a look at the patches too.
Two follow up commits to address review feedback:

https://github.com/jrburke/gaia/commit/434d46c9d3407337d72f77772dc1cf8d03b0956e

* removed mail-app_test.js to mail_app_test.js but don't delete the original.
* remove search mail from HTML serialization
* .PHONY: all
* update_shared now just does a cp -rp, and there is a SHARED_JS_SOURCES that gathers all the shared resources. I had the old setup because just a plain cp -r did not seem to correctly trigger an update in either the copied shared directory or in the built file, I forget which. But it led to me just doing the rm every time since I did not trust that shared was up to date. Hopefully the cp -rp and the SHARED_JS_SOURCES will address that now, but I am still weak with Make-fu.
* typo for template path in apps/email/js/cards/settings_account_credentials.js
* shared/mime_mapper not included

https://github.com/jrburke/gaia/commit/c8b4ac2901659fe750ca0e765fe597c01e308c0a

* fast path l10n on navigator.mozL10n.readyState
* use index instead of atTop and ignoreAtTopForCache for knowing when to cache
* small refactor to pull message_list's cache logic as a separate function
* fix indenting in html_cache_restore.js
* add some readme notes about the front-end

On the triggering of cache save in message_list: I tried an approach of using onMessageDom and adding a third arg, index, to the function to hopefully get index values from the onchange event, but index never had a value. I suspect it is because onMessageDom is called mostly from onMessagesSplice, which is not passing the index.

I changed onMessagesSplice to use index instead of atTop and the the ignoreAtTopForCache and that seems to work, and seems like it would catch all updates to the slice. If you prefer though, I can update onMessagesSplice to pass the index to onMessageDom and do the cache work in there.

I also updated build/r.js so that it uses Reflect when in xpcshell, so the stack issue with linux during a build should be fixed.

I believe the remaining item is to work out more changes to the gaia build system so the email app could do more cleanup of itself for the final build. You mentioned possibly opening up a follow-on ticket, but happy to do the work now if it makes sense, although for code commit hygiene a separate bug/commit set may make more sense.

What I think I want in the build: a way to tell gaia "this app will create its own directory in the profile prep area" since we can be smarter about what we trim, and can leverage the optimizer more for things like removing files combined in layers, use AST parsing to know what shared files to keep.

In any case, I think it is ready for another review pass, with using index in onMessagesSplice being the biggest thing to scrub. I also expect with some holidays coming up and other priority items in the queue, review may be delayed, so no worries if it takes a while. I may play around with approaches for gaia build system mods in the meantime.
(In reply to James Burke [:jrburke] from comment #17)
> https://github.com/jrburke/gaia/commit/
> 434d46c9d3407337d72f77772dc1cf8d03b0956e
> 
> * update_shared now just does a cp -rp, and there is a SHARED_JS_SOURCES
> that gathers all the shared resources. I had the old setup because just a
> plain cp -r did not seem to correctly trigger an update in either the copied
> shared directory or in the built file, I forget which. But it led to me just
> doing the rm every time since I did not trust that shared was up to date.
> Hopefully the cp -rp and the SHARED_JS_SOURCES will address that now, but I
> am still weak with Make-fu.

This looks good, but I think you want to put the rm -rf back.  It is indeed very good to clean out cruft from previous builds.  The key thing I wanted was adding "-p" to the copy so that after we nuke the tree, we (try and) restore the modification time-stamps so if nothing actually changed, we don't need to rebuild.


Love the README improvements on the other push, thanks!


> On the triggering of cache save in message_list: I tried an approach of
> using onMessageDom and adding a third arg, index, to the function to
> hopefully get index values from the onchange event, but index never had a
> value. I suspect it is because onMessageDom is called mostly from
> onMessagesSplice, which is not passing the index.
> 
> I changed onMessagesSplice to use index instead of atTop and the the
> ignoreAtTopForCache and that seems to work, and seems like it would catch
> all updates to the slice. If you prefer though, I can update
> onMessagesSplice to pass the index to onMessageDom and do the cache work in
> there.

onMessagesSplice is the best granularity, I think, but atTop does still matter.  Specifically, the BridgedViewSlice is some sub-range of the entire list of messages known to the back-end, but we provide no indication of the absolute position of those items in the list (currently; I think this will change) other than atTop.  atTop tells us that the offset of our slice relative to the first message is 0.  If we don't check atTop and the user scrolls down a lot, the 0th message in that list is not the newest message.

For full message correctness, we also want to handle the 'onchange' notifications for the messages.  Because these can be numerous and do not (currently) expose a batching context, I think what we want is a timeout based aggregation strategy.  htmlCache.delayedSaveFromNode happens after too much work has been done; basically, I think we want to wrap _cacheDom with a helper called _dirtyDomQueueCaching or something like that which schedules a timeout if one isn't present.  Since it will add a delay, it might make sense for us to then bypass htmlCache.delayedSaveFromNode's delay or something.

We care about onchange in case of flag changes that happen on the server or when we make local modifications.  The one thing this does not catch is changes in contact names; that would require listening on the MailPeep onchange events.  I think that's much less of a concern since that's going to be a very rare change and we will correct it once we do our actual backend lookup, and then we really will cache the new value.

In any event, I think it would all net out to something like this:

in onMessagesSplice,
===
    // Only cache the default Inbox...
if (this.cacheableFolderId === this.curFolder.id &&
    // Only cache if our slice is slowing the newest messages in the folder
    this.messagesSlice.atTop &&
    // Only cache if we are being told something that affects the data we cache
    index < this._cacheListLimit &&
    // (we may get splice notifications for state changes that neither add nor
    // remove headers)
    (addedItems.length || howMany)) {
  this._dirtyDomQueueCaching()
}
===

in updateMessageDom:
- first, change the signature to add an 'idx' or 'index' argument as the third argument.  We do pass this in currently, it's just not listed.

- next, add some logic that goes something like

===
    // If this is our first time, we are being spliced, and that logic will trigger
if (!firstTime &&
    // Only cache the default Inbox...
    this.cacheableFolderID === this.curFolder.id &&
    // Only cache if our slice is slowing the newest messages in the folder
    this.messagesSlice.atTop &&
    // Only cache if we are something in the cache
    index < this._cacheListLimit) {
  this._dirtyDomQueueCaching()
}
===

> I also updated build/r.js so that it uses Reflect when in xpcshell, so the
> stack issue with linux during a build should be fixed.

This is awesome.  The build step now runs quite happily for me.  It even runs okay under "ulimit -s 256" (on 64-bit linux with the 1 meg JS stack limit), which means we have a lot of head room.  (It did die on a 128k stack, but we can definitely live with that.)
 
> I believe the remaining item is to work out more changes to the gaia build
> system so the email app could do more cleanup of itself for the final build.
> You mentioned possibly opening up a follow-on ticket, but happy to do the
> work now if it makes sense, although for code commit hygiene a separate
> bug/commit set may make more sense.

If you want to get started on this, a different bug/commit I think is the way to go, possibly after sending a mail to dev-gaia to float the issue of avoiding having too much stuff in our packaged app zip files.  I think a lot of people will probably have relevant opinions but might not watch the appropriate bugzilla component for gaia...

I agree with your proposal of letting apps take control of driving the zip process themselves.  Building a fully populated target dir seems to work; another option is creating an explicit zip file mapping like Gecko's chrome jar manifests (https://developer.mozilla.org/en-US/docs/JAR_Manifests) where you list every file that goes in the zip and where it comes from on disk.
New commit to address review feedback:

https://github.com/jrburke/gaia/commit/62cad2a3937f527b58c5eecde4eeee104ca0fd3a

* Makefile, use rm -rf ./shared
* message_list.js: Catches both onMessagesSplice and onchange (via onMessageDom) for possibly cache save. Uses more checks, and a timeout.
* message_list.js: Adds back ignoreAtTopForCache for when first syncing an account, since atTop is never true in that case.

Note that I also pass the index to the onMessageDom call done inside onMessagesSplice.
Good call on centralizing the complicated check!

(In reply to James Burke [:jrburke] from comment #19)
> * message_list.js: Adds back ignoreAtTopForCache for when first syncing an
> account, since atTop is never true in that case.

This is still overcompensating.  Let's just fix gaia-email-libs-and-more.  This does the trick and doesn't change unit tests because they only care about the final value.  (I did run the IMAP tests.)  Feel free to fold it into your patch or what not; a distinct commit is not required:
https://github.com/asutherland/gaia-email-libs-and-more/tree/fix-initial-sync-flags

With this change the timeline looks something like this from the perspective of mailapi.js for the initial sync case:
- upon creation, atTop = false
- when we first hear a status update for 'synchronizing', atTop still = false
(logically, on the back-end the slice flags have now been set, so all future notifications from the back-end for the slice will have atTop = true, and this will continue to be the case until the front-end explicitly shrinks at least one message from the top of the slice)
- we hear about one or more new headers, atTop = true

Sorry for not just going right to this proposal initially; I think I was afraid it might be a more complex problem, but it's not.
- The caching clause you added in updateMessageDom accidentally got put in the "if (firstTime)" clause, which makes it a logical impossibility.  If I move it out, things work great.  What's particularly nice is that if you click on the message reader and thereby cause the message to get marked read and then force-close the app, we will still update the cache, so the message will correctly show being read when you re-open the app!

- The _onDataInserted invocation in onMessagesSplice will may potentially never fire if the Inbox is empty because there is a guard that fast-paths us out.  While I don't hold with organized people and their 'inbox zero' ways, their proactive nature makes them even more likely to complain, so we should probably make sure to invoke that logic.  Based on the semantics of waitForData, it seems like we do want to keep that guard (although its comment is somewhat out of date now), but we additionally want to fire the logic if onStatusChanged gets invoked with 'synchronized'.  I'm conflicted as to whether that little block of code needs a helper or not.  (If only JS had macros and then we could have FIRE_AND_CLEAR_CALLBACK!)

- Also in onMessagesSplice in regards to passing the index to updateMessageDom, I'm conflicted about the indexOffset compensation.  By doing that you're telling it its current index in slice.items, but if we were going to use the index in that function for caching purposes then we would want the post-splice value which means not adding in 'howMany' (are deleted).  I think it's weird either way.  I would argue the safest thing is to explicitly pass null in as the index value in this case and add a jsdoc comment to updateMessageDom that explains that the value is not passed when firstTime is true because the concept of index is ambiguous and not currently needed in that case.
Review feedback commit:

https://github.com/jrburke/gaia/commit/988102a6f10dfbf209f58236fe9e23b000ea0560

* includes asuth's fix in gelam for atTop not being true on first sync
* so ignoreAtTopForCache is now removed
* message_list now has an onMessagesChange bound to the slice's onchange event
* onMessagesChange handles the work of calling updateMessageDom and the cache save
* now updateMessageDom does not need to know about index and avoids the ambiguous passing of index in onMessagesSplice
* onDataInserted work now done in _notifyDataInserted and also triggered in the onStatusChange 'syncfailed' and 'synced' cases.
Comment on attachment 767389 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10614

This is ready to land.  Hooray and excellent work!

I checked the pull against bug 881144 which is the hdpi bug for e-mail on master, and there's only one overlapping CSS file touched and I think git is smart enough to follow the rename.

Travis seems to have not run against your most recent commit, but I think it's okay to squash your changes now, and when you push that, I think Travis should run then.  Although tests pass locally for me and so does the linter, it's best to wait for the travis run.  If Travis is still not cooperating, I think just land.


Things that I think should get spin-off/follow-on bugs (I may be missing some I previously mentioned):
- Gaia build system improvements so we have more control over our optimization
- Play with minifying levels and our concated 'layers' so the e-mail front-end starts up faster.  (Presumably using the previous.)
- Play with minifying/concatenating the back-end better for startup time.
- Consider saving the last-heard click event during the startup process and re-triggering that when load completes.  So if the user clicks on a message, it just takes forever, rather than requiring them to click again.
- (NEW!) Consider re-computing l10n relative times after de-persisting the HTML but before we kick off other expensive stuff.  Alternately, maybe we should persist absolute times to our cached HTML so we don't need to do this at all.  This would be a good thing to discuss with the UX team next week.
Attachment #767389 - Flags: review+
Blocks: 890182
Blocks: 890185
Blocks: 890189
Merged to Gaia Master, merge commit:

https://github.com/mozilla-b2g/gaia/commit/688b8cb555971bce8af095c51cfd8bde6b0f204f

GELAM merge commit:

https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/92fe44e7bf5050746ff1c4e4a7f50adbc6e8329b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Please check for uplifting to v1-train.
Blocks: 893047
You need to log in before you can comment on or make changes to this bug.