814257 - [email] Move back-end into worker thread

Status: VERIFIED FIXED

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

Description

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

The e-mail app's architecture is intentionally split into 1 or more front-ends talking to 1 back-end over a structured clone bridge.  (We used to support just JSON serialized messaging, but now that we throw Blobs around, we require structured clone.)

We currently run the front-end and the back-end in the same main page context because it was easiest.  While my dream is and was to run the back-end in a background process so that it can persist even when the UI is dead, there is really no call for this in v1 since we have ditched cron-syncing and message sending is not async.

This means we can run in a worker thread, although Gecko's worker threads currently are not as full featured as our main page contexts, so we will need to remote a lot of our API access from the worker to the main page context.  I have proposed that it may be easiest for us to develop or reuse (if they already exist) transparent shims to expose the APIs in our worker without doing anything custom.

The list of shims we currently/imminently will require is:
- mozTCPSocket
- IndexedDB
- device storage (this appears to use XPCOM IDL, so I presume is not exposed in workers)
- navigator.onLine

Things I am assuming we don't need shimmed, but could be wrong:
- Privileged XHR, specifically, mozAnon

The list of shims we may eventually require for functionality postponed to v2:
- mozAlarms 


The place where the front-end gets bolted to the back-end is in same-frame-setup.  This also serves as our root script for gaia-email-opt.js:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/same-frame-setup.js

Besides the very necessary shims, most of this work would entail creating front-end and back-end variants of same-frame-setup that know how to talk to each other.


Note!  Doing this change on its own is not going to do much to improve perceived startup time on a single core device because the UI does not bring itself up until it gets a list of accounts and folders from the back-end.  Other improvements will need to be made in parallel.

Comment 1

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

Partial progress to this.  Andreas provided a patch to convert our mozTCPSocket usage in imap.js to use the node shims we already use for SMTP.

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

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

Comment 2

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

It appears Vivien has been hard at work on this since Saturday (discovered in https://bugzilla.mozilla.org/show_bug.cgi?id=843768#c2):
https://github.com/vingtetun/gaia/tree/email.fast


It looks like this is still very much a work-in-progress.  The major issue that will need to be addressed is it is exclusively developed against the gaia repo and ignores the delineation between the  gaia front-end and the gaia-email-libs-and-more back-end and that the back-end in turn depends on modules like wbxml that have been factored out to be reusable.

Instead of using transparent shims, chunks of functionality have been migrated to the front-end code repo with an RPC style idiom hooking them up.  While I personally still favor transparent shims over RPC remoting of our modules since the problem is Gecko's very limited workers, the style of implementation in the branch could be leveraged to provide drop-in proxies when building for gaia.


A complicating factor is that the branch has been transformed so that the closure compiler does not choke on it.  This includes not only the expected changes to avoid the syntactic JS1.7-isms used by the activesync and some accountcommon.js code such as destructuring assignment, but also more extreme things like 'const'.  Semantic js1.7-isms like Iterator() have not been changed, so it's not clear we'll actually run any better on other JS engines.  This is probably going to result in some unpleasant bit-rot and conflicts for all involved.  Ideally that transformation could have landed on gelam/master in one go with all the other work only then layered on top of that...

Comment 3

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

(In reply to Andrew Sutherland (:asuth) from comment #2)
> It appears Vivien has been hard at work on this since Saturday (discovered
> in https://bugzilla.mozilla.org/show_bug.cgi?id=843768#c2):
> https://github.com/vingtetun/gaia/tree/email.fast
> 
> 
> It looks like this is still very much a work-in-progress.  The major issue
> that will need to be addressed is it is exclusively developed against the
> gaia repo and ignores the delineation between the  gaia front-end and the
> gaia-email-libs-and-more back-end and that the back-end in turn depends on
> modules like wbxml that have been factored out to be reusable.
> 

I feel like it will tooks a lot of time for me to upstream things on multiple repo while it will be much faster for you to integrate those changes in your remote repository. So I'm in favor of doing a PR against Gaia and let you upstream the changes on the correct branches - that would save time for everybody and make this feature happens sooner.

> Instead of using transparent shims, chunks of functionality have been
> migrated to the front-end code repo with an RPC style idiom hooking them up.
> While I personally still favor transparent shims over RPC remoting of our
> modules since the problem is Gecko's very limited workers, the style of
> implementation in the branch could be leveraged to provide drop-in proxies
> when building for gaia.
> 

Transparent shims is probably the way to go (while I would really be happier to have some of those APIs accessible directly on the worker :( ). But there is no reason to block on this thing - this can be done in a followup if there is a real need to do so. And I believe such a work will be living in shared/ since other apps may want to use them in the future.

> 
> A complicating factor is that the branch has been transformed so that the
> closure compiler does not choke on it.  This includes not only the expected
> changes to avoid the syntactic JS1.7-isms used by the activesync and some
> accountcommon.js code such as destructuring assignment, but also more
> extreme things like 'const'.  Semantic js1.7-isms like Iterator() have not
> been changed, so it's not clear we'll actually run any better on other JS
> engines.  This is probably going to result in some unpleasant bit-rot and
> conflicts for all involved.  Ideally that transformation could have landed
> on gelam/master in one go with all the other work only then layered on top
> of that...

The goal is to make this app fast and those features does not bring much as they are currently used and prevent some to run simple minification on the code. Also it has been told since the beginning of Gaia that those non standard features should have been avoided, so ideally they should not have landed at first. Iterator does not make the closure compiler angry because it thinks this is a regular function. I still have some issues with yield as it is used in one of the file but changing this function will affects the logic and I don't want to do that here.

So to resume Email is the last app that tooks a while to start and the goal is to fix that Today (this is already Friday here).

Comment 4

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #3)
> I feel like it will tooks a lot of time for me to upstream things on
> multiple repo while it will be much faster for you to integrate those
> changes in your remote repository. So I'm in favor of doing a PR against
> Gaia and let you upstream the changes on the correct branches - that would
> save time for everybody and make this feature happens sooner.

If it was your plan to throw the code over the wall to me, it would have been very helpful if you consulted with me before implementing things or checked out this bug and my tentative strategy for this.  I feel like you have saved you a lot of time, but I am concerned it might take me as much time to port your changes across as it would for me to just implement them from scratch in the first place.

To be clear, I really appreciate you pitching in to help speed up the e-mail app, but I feel like this was gone about the wrong way.  It's frustrating for me to be in a position where I end up looking like the guy standing in the way of making the e-mail app fast because I'm also one of the main people who has to support this stuff without going insane.

> The goal is to make this app fast and those features does not bring much as
> they are currently used and prevent some to run simple minification on the
> code. Also it has been told since the beginning of Gaia that those non
> standard features should have been avoided, so ideally they should not have
> landed at first. Iterator does not make the closure compiler angry because
> it thinks this is a regular function. I still have some issues with yield as
> it is used in one of the file but changing this function will affects the
> logic and I don't want to do that here.

I have no problem with eliminating Mozilla-specific JS.  It's just far preferable to break syntactic changes like this out into a separate pull request because of the high risk of conflicts.

Apart from const, I've been trying to keep things as ES5 and JS1.5 as possible, but I also didn't demand rewrites of things that tried to use the JS ergonomics afforded to us given that we already were fairly Mozilla specific and the ES5 normalization wouldn't be that hard.

> So to resume Email is the last app that tooks a while to start and the goal
> is to fix that Today (this is already Friday here).

There is categorically no way this patch can land as-is.

Comment 5

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

(In reply to Andrew Sutherland (:asuth) from comment #4)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #3)
> > I feel like it will tooks a lot of time for me to upstream things on
> > multiple repo while it will be much faster for you to integrate those
> > changes in your remote repository. So I'm in favor of doing a PR against
> > Gaia and let you upstream the changes on the correct branches - that would
> > save time for everybody and make this feature happens sooner.
> 
> If it was your plan to throw the code over the wall to me, it would have
> been very helpful if you consulted with me before implementing things or
> checked out this bug and my tentative strategy for this. 

Sorry I'm not sure what means 'throw the code over the wall' but if this is something like: 'comes with a bunch of code in front of someone and say: here it is' that was not the plan - the plan is to have a fast email application asap by using all the skills possible of everybody. And you know much better than me where to put those changes!

I don't want to feel aggressive with this patch or with my comments, but email is the slowest app at this point and making patches to make it fast is much more efficient than speaking for me.

(And also I never saw this bug before you pointed it on the other bug opened by Julien)

> I feel like you
> have saved you a lot of time, but I am concerned it might take me as much
> time to port your changes across as it would for me to just implement them
> from scratch in the first place.

How long do you expect it would take you to port this patch? And how long do you think it would have taken you to do it? I started on monday the 'worker' patch so it tooks me 4 days -which is far from saving my time. If you feel it would have took you 4 days (and I expect less from you since you know the app while I don't) what has prevented you to did it?

The bridge - mailapi communication was ready for workers

Your feeling is wrong since I'm pretty new to the architecture of the email app and I had to dig a lot inside it while doing this patch so I feel like most of my time would have been saved if I didn't have to jump on that actually :)

Does porting it require something else than copying the file to the right repo?

> 
> To be clear, I really appreciate you pitching in to help speed up the e-mail
> app, but I feel like this was gone about the wrong way. 

Could you elaborate? What would have be the *right way* ?

> It's frustrating
> for me to be in a position where I end up looking like the guy standing in
> the way of making the e-mail app fast because I'm also one of the main
> people who has to support this stuff without going insane.

Sorry I don't want to make you feel this way - As long as you explain with good reasons why the current shape of the code is not 'good enough' to land your position makes sense. Leveraging the current implementation is not my immediate goal here so that's not an argument for me. And if you really feel this will be useful for other apps you're welcome to help moving them to shared/ once this bug is resolved. I would appreciate actually if you want to make a patch against my branch to draw the line better between the front-end and the back-end tonight so I will be able to continue my work on it tomorrow.
 
> > So to resume Email is the last app that tooks a while to start and the goal
> > is to fix that Today (this is already Friday here).
> 
> There is categorically no way this patch can land as-is.

And that's why I'm cleaning it right now - It's not going to be perfect since as I said your architecture is still pretty new to me and you may have some strong opinions about things I does not suspect.

Comment 6

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #5)
> Sorry I'm not sure what means 'throw the code over the wall' but if this is
> something like: 'comes with a bunch of code in front of someone and say:
> here it is' that was not the plan

Sorry, yeah, it's an idiomatic expression, but you guessed right.

> (And also I never saw this bug before you pointed it on the other bug opened
> by Julien)

This is mainly my frustration.  There is one bug in the Gaia::E-mail component with "worker" in the subject, and it's this bug so it could have been found with limited searching.  Or if you had filed a new bug about what you were planning to do and assigned it to yourself, I could have pointed you at this bug and/or we could have discussed things on that bug.  Or an e-mail also would have worked.


Your effort is fantastic and appreciated, I'm mainly just complaining that the lack of coordination led to shortcuts in how you developed (in gaia rather than gaia-email-libs-and-more) which led to design choices that are going to complicate propagating your changes back into gaia-email-libs-and-more for whoever does it.  Transparent shims that could disappear when gecko's workers improve would have required significantly fewer changes in gaia-email-libs-and-more.

I think we've now moved on to getting your changes in (via IRC), and I deeply appreciate your support in this and in generally improving the performance of the e-mail app.

Comment 7

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

Andrew I tried to use the gaia-email-lib-and-more repo to launch the tests - sadly they don't run with an unchanged repository. There seems to be a lot of import errors with modules...

Comment 8

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

Attachment: Patch v1

This is not the final patch. I would like to clean a little bit the communication from the worker to the main thread and have a generic method somewhere. The listeners for those calls could also be moved in the whole back-end but I have no strong opinion about that and the only things that should really matter is the fact that they live in ext/ and do not depends on the front-end.

Comment 9

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

Comment on attachment 717095 [details] [diff] [review]
Patch v1

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

Oh and I need to clean this setTimeout/process.nextTick/setZeroTimeout thing. It was not working as expected on the worker likely because message were going to the worker -> main thread and the structured cloning algorithm was not a fan of functions.
>

Comment 10

[Masatoshi Kimura [:emk]]

Will the e-mail app need to send a mail encoded in non-UTF-8? Currently TextEncoder allows only UTF encodings.

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #10)
> Will the e-mail app need to send a mail encoded in non-UTF-8? Currently
> TextEncoder allows only UTF encodings.

(This is not specific to workers,)

Comment 12

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

(In reply to Masatoshi Kimura [:emk] from comment #11)
> (In reply to Masatoshi Kimura [:emk] from comment #10)
> > Will the e-mail app need to send a mail encoded in non-UTF-8? Currently
> > TextEncoder allows only UTF encodings.
> 
> (This is not specific to workers,)

A better place to ask would be the dev-gaia mailing list or as a separate bug.  The short answer is: only if there's a _really_ good reason for it.  Please file a new bug in the Gaia::E-mail component if you have such a reason and we can discuss it there.

Comment 13

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

I filed bug 844426 to track doing just the ES5-izations.  I put a pull request up on there that has most (all?) of the ES5-only changes ported over to gaia-email-libs-and-more and its sub-deps.  Unfortunately, it looks like I regressed something about autodiscovery in the process, probably in the more tricky bit where I was taking stuff from where the logic had migrated to another file to run in the page context rather than the worker context.  I think I may have failed to properly clean the build byproducts when I was verifying the jsas changes, because I did get a green test run.

I'm super tired so I need to crash now, but I should be able to finish that up tomorrow if someone else doesn't fix it first.  That pull request is using a branch in mozilla-b2g/gaia-email-libs-and-more rather than my own, so peeps should be able to push to it.

Comment 14

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

Attachment: gaia-email-lib.patch (still need some love to clean up message passing)

Comment 15

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

Attachment: activesync.patch

Comment 16

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

Attachment: bleach.patch (still need some love for HTML parsing / link stashing)

Comment 17

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

Attachment: TextEncoder-b2g18-part1.patch

Comment 18

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

Attachment: TextEncoder-b2g18-part2.patch

Comment 19

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

Attachment: Patch

Comment 20

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

Attachment: Email app - front-end patch

Comment 21

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

Attachment: Main thread side of the APIs.

This zip contains the file that were living into apps/email/js/ext/apis/ in my build. That the main thread side of the backend db, socket, etc...

I'm not sure where you want to put them in the backend.

Comment 22

[Masatoshi Kimura [:emk]]

Comment on attachment 722934 [details] [diff] [review]
TextEncoder-b2g18-part2.patch

>diff --git a/dom/webidl/TextDecoder.webidl b/dom/webidl/TextDecoder.webidl
>--- a/dom/webidl/TextDecoder.webidl
>+++ b/dom/webidl/TextDecoder.webidl
>@@ -4,17 +4,17 @@
>-[Constructor(optional DOMString encoding = "utf-8",
>+[Constructor(DOMString encoding,

Why did you make it non-optional? |new TextDecoder()| shouldn't throw.

>diff --git a/dom/webidl/TextEncoder.webidl b/dom/webidl/TextEncoder.webidl
>--- a/dom/webidl/TextEncoder.webidl
>+++ b/dom/webidl/TextEncoder.webidl
>@@ -4,17 +4,17 @@
>-[Constructor(optional DOMString encoding)]
>+[Constructor(DOMString encoding)]

Same here.

Comment 23

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

(In reply to Masatoshi Kimura [:emk] from comment #22)
> Comment on attachment 722934 [details] [diff] [review]
> TextEncoder-b2g18-part2.patch
> 
> >diff --git a/dom/webidl/TextDecoder.webidl b/dom/webidl/TextDecoder.webidl
> >--- a/dom/webidl/TextDecoder.webidl
> >+++ b/dom/webidl/TextDecoder.webidl
> >@@ -4,17 +4,17 @@
> >-[Constructor(optional DOMString encoding = "utf-8",
> >+[Constructor(DOMString encoding,
> 
> Why did you make it non-optional? |new TextDecoder()| shouldn't throw.
> 
> >diff --git a/dom/webidl/TextEncoder.webidl b/dom/webidl/TextEncoder.webidl
> >--- a/dom/webidl/TextEncoder.webidl
> >+++ b/dom/webidl/TextEncoder.webidl
> >@@ -4,17 +4,17 @@
> >-[Constructor(optional DOMString encoding)]
> >+[Constructor(DOMString encoding)]
> 
> Same here.

I think that was because that was a different bug than the initial one targetting the signature to match the spec and I didn't backported it but there is no strong reasons for it otherwise.

Comment 24

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

Those patches are targetting the gaia-email-lib-repo as well as some of its dependencies. Also there is a zip file containing the main thread side of the backend apis living in the worker. Not sure where they should belong in the backend.

There is also a small front-end patch.

There is also the patches I used on b2g-18 for TextEncoder/TextDecoder.

The combination of those patches still have some issues, here the list:
 - small regression with device storage in jobmixins - it was working but I likely broke something in my last merge.
 
 - Need to clean up the messaging code in the backend. A lot of boilerplate is duplicated in the files that were used to use APIs not available anymore in the worker. At the moment each files send message to the main thread and they listen for a specific keyword in order to filter the data they are looking for. This is probably not that good and a big listener that dispatch to the right target would be much better (maybe similar to what i did on the main thread side).
 
 - bleach.js. The new HTML Parser does not work with all html emails and is probably very easy to broke it right now. Also I didn't backport yet prune / links stashing but this part should be easy.


I feel like the hardest part will be to make sure bleach.js works as expected with HTML emails.

Those patches should make panning smoother while updating the thread list but I noticed that the candy progress bar consume a lot of CPU as well. It forces a redraw every 30ms and additionaly there is a reflow when progress.value is updated. Removing those also helps a lot panning the main email list.

This also probably needs a lot of testing.

Comment 25

[Masatoshi Kimura [:emk]]

(In reply to Vivien Nicolas (:vingtetun) (:21) (away until march 25th) from comment #23)
> I think that was because that was a different bug than the initial one
> targetting the signature to match the spec and I didn't backported it but
> there is no strong reasons for it otherwise.

I rechecked bug 764234, 801487, 825195, 795542 and hg history.
The first parameter was optional from the start and has never been non-optional. YOU changed the signature, not me.

Comment 26

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

(In reply to Masatoshi Kimura [:emk] from comment #25)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (away until march 25th) from
> comment #23)
> > I think that was because that was a different bug than the initial one
> > targetting the signature to match the spec and I didn't backported it but
> > there is no strong reasons for it otherwise.
> 
> I rechecked bug 764234, 801487, 825195, 795542 and hg history.
> The first parameter was optional from the start and has never been
> non-optional. YOU changed the signature, not me.

I did not put those 2 backend patches to land but more for Andrew to be able to develop on top of it. If those should be optional that sounds fine.

Comment 27

[Masatoshi Kimura [:emk]]

Just replace |DOMString encoding| with |optional DOMString encoding = "utf-8"|. WebIDL binding codegen will do the job.

Comment 28

[Josh Carpenter [:jcarpenter]]

Flagging this as ux-most-wanted in the hopes that this addresses scrolling choppiness, which in current builds is a higher priority than sheer FPS.

(In reply to Vivien Nicolas (:vingtetun) (:21) (away until march 25th) from comment #24)
> Those patches should make panning smoother while updating the thread list
> but I noticed that the candy progress bar consume a lot of CPU as well. It
> forces a redraw every 30ms and additionaly there is a reflow when
> progress.value is updated. Removing those also helps a lot panning the main
> email list.
> 
> This also probably needs a lot of testing.

We can look at finding more performant progress indicators if need be.

Comment 29

[Julien Wajsberg [:julienw]]

I uplifted the landing done in Comment 1 because uplift for Bug 814273 would have had it anyway:
v1-train: b0061bb19f9d752b0f83092816d98a311dc77575

Comment 30

[Julien Wajsberg [:julienw]]

same for v1.0.1: 412970be4d4d413b95c085824787521238adeb18

Comment 31

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

My understanding is that for performance reasons this is *strongly* desired for v1.0.1, so I think we should formalize the tef+ so we can get bug 795542 uplifted sooner rather than later.  It's going to be harder for people to test the worker thread patch for gaia if they also have to patch gecko.

Comment 32

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

(In reply to Masatoshi Kimura [:emk] from comment #27)
> Just replace |DOMString encoding| with |optional DOMString encoding =
> "utf-8"|. WebIDL binding codegen will do the job.

I formally request help with an uplift of TextEncoder/TextDecoder on workers from the platform team; cc'ing jonas because he reviewed many of these patches and dougt seems to command legions of loyal platform hackers...


I found out why :vingtetun had made this change; without it, his patch gets compile errors involving optional arguments.  mozilla-b2g18 seems to be fairly behind the times in terms of StringEncoding API and appears to be missing commits that bug 795542 depends on.

===
hg log on dom/encoding/TextDecoder.h in mozilla-b2g18 gets me:

changeset:   114724:2eb1ca5c87ea
user:        Masatoshi Kimura <[e-mail]>
date:        Fri Sep 28 11:19:18 2012 +0100
summary:     Bug 764234 - Implement StringEncoding API. r=dougt,smontagu


===
git log --reverse on dom/encoding/TextDecoder.h in the mozilla-central git mirror (mq blew up my hg repo) shows me that the following 3 landings happened after the above and before the worker fix in bug 795542:

commit 73ca0ef1beb96708f08842facf7528eb5aa39e5b
Author: Masatoshi Kimura <[e-mail]>
Date:   Tue Nov 6 18:23:14 2012 -0500

    Bug 801487 - Remove encoding detection using BOM. r=sicking

commit e37c38e5b97e530c57ab5257d1ee30760404ab59
Author: Masatoshi Kimura <[e-mail]>
Date:   Wed Nov 7 18:04:22 2012 -0500

    Bug 801402 - Make EncodingUtil methods case sensitive. r=hsivonen

commit 242ce4f3e40dcc00a6233edea9eb389e67254a4a
Author: Masatoshi Kimura <[e-mail]>
Date:   Mon Nov 26 20:38:20 2012 -0500

    Bug 782412 - Part 5: Remove a workaround from dom/encoding. r=hsivonen

===
Particularly concerning are that bug 801487 (Update StringEncoding API per the latest spec and fix some bugs) involved landing 7 patches.  There are also some concerning bugs after bug 795542 landed involving nsISupports on worker threads: bug 816088 and bug 841479 (the latter of which may moot the former?)

Comment 33

[Masatoshi Kimura [:emk]]

Attachment: Make TextDecoder and TextEncoder's constructor paramter optional again

I was able to build latest mozilla-b2g18 branch + attachment 722933 [details] [diff] [review] + attachment 722934 [details] [diff] [review] + this patch on Windows without any problems. I don't understand what is so difficult.

Comment 34

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

(In reply to Masatoshi Kimura [:emk] from comment #33)
> Created attachment 728595 [details] [diff] [review]
> Make TextDecoder and TextEncoder's constructor paramter optional again
> 
> I was able to build latest mozilla-b2g18 branch + attachment 722933 [details] [diff] [review]
> [details] [diff] [review] + attachment 722934 [details] [diff] [review] +
> this patch on Windows without any problems. I don't understand what is so
> difficult.

Thanks very much for the sanity check.  I suspect I must not have fully cleaned my build directory from a prior build and there must have been some leftovers the Makefiles didn't correct.  (Or I did something else foolish.)  Building from clean with Vivien's patches and your fix is working properly for me now.  *Thank you*.

If you can land the changes on mozilla-b2g18, that'd be great, otherwise I'll do it in the morning.  I'm not going to forget my lesson about not doing things without sufficient sleep so quickly...

Comment 35

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/08e95442bc65
https://hg.mozilla.org/releases/mozilla-b2g18/rev/34a84093e26a

Comment 36

[Masatoshi Kimura [:emk]]

I have to backout changes because of test failures:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/563c58c449c6
I have no time to hunt down the cause of the failure, sorry

Comment 37

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

Thanks very much for trying to land the patches!

Ah, yeah, the gaia email unit tests turn out to be failing using a "utf-16be" TextDecoder with the patches.  (We use that to decode modified-utf-7 for IMAP folder names.)  Maybe we do need some of those fixes after all?

Looking at:
https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=34a84093e26a

I see at least:
- crasher: Mochitest 2
- utf-8 round-trip bug (didn't get to utf-16le or utf-16be) on xpcshell
- TextDecoder is defaulting to utf-16 instead of utf-8 from JSM space: mochitest "oth"

If anyone can provide assistance in figuring out what is needed to uplift/prepare a non-regressing patch, and potentially then help assemble that patch and push a try run, that would be super helpful.  I will post on this bug if I start to do that to make sure there is no overlap of effort.

Comment 38

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Create Text(En|De)coderBase

- Folded the optional parameter patch.
- Removed the unrelated netwerk/protocol/app/AppProtocolHandler.js change.
- TextDecoder had duplicate members which hid members in the base class.

Comment 39

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Implement StringEncoding API objects in Workers

Comment 40

[Masatoshi Kimura [:emk]]

These patches worked locally, but I couldn't test it on the tryserver because the server stop to respond on searching changes. How can I test a b2g patch on try?

Comment 41

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

(In reply to Masatoshi Kimura [:emk] from comment #40)
> These patches worked locally, but I couldn't test it on the tryserver
> because the server stop to respond on searching changes. How can I test a
> b2g patch on try?

It was my understanding from fabrice that the normal try server should work.  Maybe no one has pushed a b2g18 commit to the repo in some time so there are just a lot of patches to push?  Does your connection time out and fail to push?  I guess #developers or #b2g might have better advice.

If try is broken for b2g18, I think the importance of the patch is such that it may be worth landing on b2g18 anyways and backing it out if it fails again.  Copying :akeybl.

Comment 42

[Masatoshi Kimura [:emk]]

try hangs at least 12 hrs here:
> $ hg push -f try
> pushing to ssh://hg.mozilla.org/try
> searching for changes

I'll reland on b2g18 directly again.

Comment 43

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/44ccc86fa232
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e94004b134a3

Comment 44

[Joe Cheng [:jcheng] (please needinfo)]

how are things with landing here? thanks

Comment 45

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

Sorry for the lack of visibility here; we've been working very hard.  Our daily stand-up notes are here:
https://etherpad.mozilla.org/email-perf

IMAP unit tests are green, Jim Porter's fixing the ActiveSync tests, and James Burke and I are doing testing on device.  IMAP is visibly working on the device too, but there is some weirdness with snippest we are investigating.  We currently believe with high probability that we should be able to land it on Wednesday.

The only work item that has popped us since providing the estimate is that for security reasons we need to use a real CSS parser rather than the minimal .split() implementation Vivien had stubbed out.  However, since there are a number of viable options for this, this is not expected to be much of a problem, although it is likely CSS parsing will initially be somewhat slower than the DOM approach because I think we are going to go with the most spec-compliant option available until we have massively improved our security test case coverages.

Comment 46

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

The problem with snippets on the worker thread was that the extend() in imap.js taken from jQuery broke so the body argument wasn't getting propagated and our body snippet fetches never actually fetched any body.

This is now resolved on the worker-thread branch in conjunction with the suppression fixes so we won't repeatedly try and grab snippets for a message whose snippet is the fallback of ''.

Responsiveness/smoothness is markedly improved on IMAP with the worker, but this does not appear well represented in the FPS indicator, probably because we are unable to hit 60 fps under any circumstances and the FPS indicator by default only has a memory of at most 250ms from 16 frame sample points.

To try and get a better idea of the impact, I modified the FPS counter to instead generate histograms of the time between each call to add a frame to the FPS/draw the FPS.  For the data below I created a new account, scrolling around to cause lazy snippet fetching and periodically telling it to fetch more messages.  Frame durations were binned at a 2ms granularity into 64 bins (saturating to the highest bin).  So the first bin is ~0-2ms, the second ~2-4ms, etc.  The histogram data was dumped every 120 frames to adb.  I then also ran this through a little sparkline generator *with scaling disabled by saturating at a value of 7 for each bin).  If the following looks messed up, you must have bad fonts.

No worker:

▁▁▁▁▄▂▃███▅▄▃▁▁▂▁▁▁▂▂▁▁▁▂▂▁▁▁▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▂▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▄
▁▁▁▁▁▁▂▁██▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▃███▂▄▂▂▄▂▁▄▃▃▃▁▁▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▄███▇██▆█▅▆▆▄▂▄▄▂▃▁▂▂▁▂▁▁▂▂▂▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▂▁█▃▇▆▆█▅▆███▄█▃▇▄▁▄▄▁▁▃▁▂▁▂▃▁▁▁▁▂▁▁▁▂▁▁▁▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▁
▁▁▁▁▁▂▃███▇▆▅▅▅▃▂▄▄▄▆▂▂▃▅▃▃▂▂▂▂▁▁▁▁▁▁▂▁▁▂▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▅█▃▆██▆███▆▃▄▆▃▂▄▂▅▄▁▁▁▂▁▂▁▂▂▁▂▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▁█▃▂▃▅▃▇▄████▇▃▅▄▃▃▁▂▂▄▁▃▂▂▃▁▁▂▂▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂
▁▁▁▁▁▂▃███▆██▆█▆▄▄▆▁▃▂▁▂▂▂▁▂▁▁▁▁▃▂▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▆███▂█▃▅▁▄▃▁▃▃▁▄▂▂▁▁▁▂▂▂▂▁▂▁▁▂▁▂▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▃▂▃▇██▂▁▂▁▁▂▁▁▁▂▁▁▂▁▁▂▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▂▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃
▁▁▁▁▁▁▂█████▇▅▆▂█▆▅▃▁▅▁▂▃▂▂▁▁▃▂▁▁▁▁▁▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▂▁▂▁▁▁▁▁▁▁▁▁▃
▁▁▁▁▂▁▃███▆▅█▄█▃▇▇▄▆█▃▁▂▄▂▁▁▄▂▃▁▃▁▁▂▁▁▂▂▂▁▁▁▂▁▁▁▁▁▁▁▁▁▁▂▂▂▁▁▁▁▁▂
▁▁▁▁▁▁▁█▂███▇▅▅▅▇██▄▂▃▁▄▃▂▇▂▁▂▁▃▂▃▁▂▁▁▁▁▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂
▁▁▁▁▂▄▁█▅███▆▅▄▄▆▂▁▂▁▁▁▂▃▃▁▂▁▂▁▁▂▂▂▁▁▂▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂
▁▁▁▁▁▂████▁▅▁▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▁▂

Worker:

▁▁▁▁▁▁▁███▃▁▁█▆▁▂▄▂▁▁▁▁▂▂▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▂▁▁▁▁▁▁▁▁▁▁▁▁▅
▁▁▁▁▂▁▁▁██▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▂▃▂███▄▁▁▂▄▅▁▃▄▁▂▁▁▁▁▁▂▂▃▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▂█▃▄▄▅▄▂▄▅▇█▆▆▄████▄▆▄▂▄▄▃▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▂█▄████▇▄█▂▄▆▁▅█▂▂▂▂▁▂▂▂▁▂▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▁█▄███▂████▅█▅▇▁▆▃▁▃▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▁█▂██████▁▅█▂▃▃▃▃▃▂▃▂▄▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂
▁▁▁▁▁▄▁███▃▃▂▁▂▃▃▄▁▁▄▂▁▁▂▁▁▁▁▁▁▁▁▁▂▃▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▁▆
▁▁▁▁▁▁▄█████▆▃▅▄▅▆▇▄▅▅▃▂▁▂▁▁▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▃█▅█▂▄▄▅███▇▅█▃▃▂▆▂▁▃▁▁▁▁▁▂▂▂▁▁▂▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃
▁▁▁▁▁▁▁▃▇██████▆█▃▆▆▃▃▄▁▁▁▁▂▃▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂
▁▁▁▁▄▇▄█████▄▇▄▂▃▄▁▃▁▁▂▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂
▁▁▁▁▁▂▄███▃▂█▃▄▄▂▄▃▃▁▂▂▃▂▂▂▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▂▁▁▁▁▁▁▇
▁▁▁▁▁▁▂█▂███████▇██▃▅▂▁▄▁▂▂▁▁▁▁▂▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▁▇▂████▄▂██▅▅▄▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
▁▁▁▁▁▁▁█▂████▂▇▆▄▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁


The mozilla-b2g18 patch to log the data, the data from the above, and the simple processing script can be found here, noting that I did trim the adb output in a text editor first:
https://github.com/asutherland/gelam-tools


It's worth noting that the data is unable to distinguish between us not wanting to paint at 60fps and us being unable to paint at 60fps.  The message list usually does seem to want to paint as fast as possible, compared to a more idle screen like the account setup screen, but I make no guarantees.

One could argue based on the above that the worker is generally improving things by eliminating a lot of the longer frame times, but it is unable to eliminate occasional scary (but ambiguous) points all the way on the right.

Comment 47

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

Attachment: fps sparklines as attachment for fixed width purposes

Comment 48

[James Burke [:jrburke]]

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

Pointer to Github pull-request

Comment 49

[James Burke [:jrburke]]

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

Change for moving backend to worker. Includes all the GELAM changes.

Comment 50

[Julien Wajsberg [:julienw]]

master : 0649e5e9d2f71c96517bca1b582c706e665bca6a

Comment 51

[Tony Chung [:tchung]]

hi Andrew,  nice work to you and the team.   Can you give QA some pointers on what testing should we be looking out for?   Perf and responsiveness jump out; so i'm looking for where regressions would occur most.

Comment 52

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

(In reply to Tony Chung [:tchung] from comment #51)
> hi Andrew,  nice work to you and the team.   Can you give QA some pointers
> on what testing should we be looking out for?   Perf and responsiveness jump
> out; so i'm looking for where regressions would occur most.

As noted in my e-mail to dev-gaia, regressions in HTML sanitization are most likely because the HTML sanitizer was completely redone, but we also weren't perfect there before, so you might end up noticing new issues (which is still fine).  Otherwise, anything and everything could have broken.

Comment 53

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 0649e5e9d2f71c96517bca1b582c706e665bca6a
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 54

[Julien Wajsberg [:julienw]]

This uplifts cleanly on both on v1.0.1 and v1-train once Bug 854292 is uplifted. tef+ was asked there to uplift it.

Comment 55

[Julien Wajsberg [:julienw]]

v1-train: b973e66f328d8e09e364654e17ddab9f2c938bc4

Comment 56

[Julien Wajsberg [:julienw]]

v1.0.1: 8cf8dbd430af5e72a2ad542824421debe6de29d4

Comment 57

[Tracy Walker [:tracy]]

Verified with the latest 1.0.1 build