849924 - [email] process/render the email body once the card transition started

Status: RESOLVED FIXED

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

Description

[Etienne Segonzac (:etienne)]

Related to bug 846373.

It's fairly easy to have an email body that will take ~1s (or more) to be processed/rendered.

In order to mitigate this we can start the card transition before we start linkifying/appending to the dom.

The proposal is to split the dom work into header/body in order to
- fill the header section
- start the (OMT) transition
- start the body work

Comment 1

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Small patch, easily upliftable.

And it feels a lot better.

Comment 2

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

Comment on attachment 723576 [details] [diff] [review]
Patch proposal

Doesn't this mean the body won't be there when we animate the card in?

Most of the body process/render time appears to really be because of our slow/dumb regexps for linkification on bug 846373, so I'd prefer to address that first.

Comment 3

[Etienne Segonzac (:etienne)]

(In reply to Andrew Sutherland (:asuth) from comment #2)
> Comment on attachment 723576 [details] [diff] [review]
> Patch proposal
> 
> Doesn't this mean the body won't be there when we animate the card in?

Yes it does.
But even with the linkification de-activated long emails cause too long of a gap between the tap on a message and the card animation.

We can do as much optimization as we want we still need to ensure an instant feedback to the user (that's what we do in other apps).

I guess it's more of a UX decision.

Comment 4

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

Yeah, it's ultimately a UX decision, although it's preferable to have numbers.


Do you have a number on how long it takes with the linkification de-activated?  And are you sure you disabled it correctly?  If you have a branch with the disabling with timestamps or a patch, that's an easy way for me to help validate.

In the example on bug 846373, it only took 10ms to insert the body of an HTML message with linkification even naively disabled, but it took 460ms with linkification still enabled.  Similar numbers for text/plain.


Having said that, if we do bug 808716 we will need to show the message reader card without the body.  But in that case we'll also need UI to express that we are currently fetching the body.

Comment 5

[Etienne Segonzac (:etienne)]

Attachment: DEBUG - no linkification + logs

Comment 6

[Etienne Segonzac (:etienne)]

Some numbers, testing with an email newsletter:

== current version of the email app ==
I/GeckoDump(12277): LOG: +++ 1363113848425 the user did something 
I/GeckoDump(12277): LOG: +++ 1363113849725 the user see something
-> 1300ms gap

== email app with the linkification de-activated ==
I/GeckoDump(12025): LOG: +++ 1363113499469 the user did something 
I/GeckoDump(12025): LOG: +++ 1363113500398 the user see something 
-> 929ms gap

== Data with the proposed patch ==
I/GeckoDump(12296): LOG: +++ 1363114036868 the user did something 
I/GeckoDump(12296): LOG: +++ 1363114036884 the user see something 
-> 16ms gap

Comment 7

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

Yup, that's well disabled.

And those are terrifying numbers!

Could you forward me a copy of this e-mail newsletter as an attachment?  It seems like a really good test case for me to have around, and I'd also like to take a look at it to better understand what is so horrible about it that we are preforming that badly on it.

Comment 8

[Etienne Segonzac (:etienne)]

(In reply to Andrew Sutherland (:asuth) from comment #7)
> Could you forward me a copy of this e-mail newsletter as an attachment? 

done

> It
> seems like a really good test case for me to have around, and I'd also like
> to take a look at it to better understand what is so horrible about it that
> we are preforming that badly on it.

And what about the patch?

I think we could probably find 5-10 simple tricks like this one to make the _on device_ email app experience better in the short term, but are the email peers open to the idea?

Comment 9

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

(In reply to Etienne Segonzac (:etienne) from comment #8)
> I think we could probably find 5-10 simple tricks like this one to make the
> _on device_ email app experience better in the short term, but are the email
> peers open to the idea?

Anything to improve performance, perceived or real.  I just need to have data and some degree of confidence that we're not putting a band-aid on something while missing the root cause.  And thanks to your supplying your test case, we've got that data, thanks!

I ran the profiler, and the time breakdown with my changes on bug 850815 and your changes here show:
- buildBodyDom: 169 ticks
 - initial layout: 64 ticks
 - resizeFrame triggered re-layout: 41 ticks
 - linkifyHTML: 55 ticks

So, I think we totally want to deal with the linkifyHTML problem, and we can probably do something about the second reflow I am compelling.  For example, the sanitization pass could probably do the newletter inference mode to know if we can fast-path things so there is only one layout required, or maybe that initial flow already is the right width and we are just being dumb by triggering the second reflow.

Regardless, that 64 ticks that may be unavoidable is indeed way too long to wait for.  Additionally, we're going to move to async body fetches with bug 808716 anyways, so we might as well go for it.

Comment 10

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

Comment on attachment 723576 [details] [diff] [review]
Patch proposal

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

::: apps/email/js/message-cards.js
@@ +940,4 @@
>      App.loader.load('js/iframe-shims.js', function() {
> +      setTimeout(function nextTick(self) {
> +        self.buildBodyDom(self.domNode);
> +      }, 0, this);

it's preferable to avoid mixing self and this.  Unless you feel strongly, maybe let's also just .bind(this) in here too?

Comment 11

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

I've filed bug 850839 about trying to avoid the second reflow.  And we already have bug 846373 about the linkification being slow.

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Andrew Sutherland (:asuth) from comment #9)
> (In reply to Etienne Segonzac (:etienne) from comment #8)
> > I think we could probably find 5-10 simple tricks like this one to make the
> > _on device_ email app experience better in the short term, but are the email
> > peers open to the idea?
> 
> Anything to improve performance, perceived or real.  I just need to have
> data and some degree of confidence that we're not putting a band-aid on
> something while missing the root cause.  And thanks to your supplying your
> test case, we've got that data, thanks!

Sounds very reasonable :)

Comment 13

[Etienne Segonzac (:etienne)]

(In reply to Andrew Sutherland (:asuth) from comment #10)
> Comment on attachment 723576 [details] [diff] [review]
> Patch proposal
> 
> Review of attachment 723576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/email/js/message-cards.js
> @@ +940,4 @@
> >      App.loader.load('js/iframe-shims.js', function() {
> > +      setTimeout(function nextTick(self) {
> > +        self.buildBodyDom(self.domNode);
> > +      }, 0, this);
> 
> it's preferable to avoid mixing self and this.  Unless you feel strongly,
> maybe let's also just .bind(this) in here too?

Yep, np.

Comment 14

[Etienne Segonzac (:etienne)]

https://github.com/etiennesegonzac/gaia/commit/5a254e35c2bc927d9c81fe702bbc5b460b6e2e88

Comment 15

[Julien Wajsberg [:julienw]]

Was landed in v1.0.1 as part of Bug 851124 big uplift.
Will land in v1-train tomorrow at the latest too.

Comment 16

[Julien Wajsberg [:julienw]]

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