1089920 - [Messages] Investigate and fix the gaia-header in Messages app (~50ms)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Julien Wajsberg [:julienw]]

Strangely the gaia-header is behaving particularly badly in the Messages app.

See bug 1074783 comment 31 and bug 1087329 comment 6 and bug 1084173.

It's not the same in other apps though.

STR:
1. launch the application
2. look at the title bar while it's loading

Expected:
* we see "Messages" in english, centered

Actual:
* we see various things happening, and eventually "Messages" is displayed centered.

I see this on Buri, but not on Flame. So we likely need a lower-end device.
However even on Flame there is a performance impact from this (see bug 1074783 comment 31)

QAwanted: can you please look at it in v1.4, v2.0, v2.1 and v2.2 with a Buri to see how it behaves? I'd like to know if this was happening the same with the older way of centering the text in v2.0, or if the gaia-header worsened it.

Comment 1

[Wilfred Mathanaraj [:WDM]]

This sounds like a bug that needs further investigation as to what is happening and what performance impact it may have had. For now we are not going to block on it but if something specific is identified and adds value to improving the performance then we can re-nominate for 2.1

Comment 2

[Julien Wajsberg [:julienw]]

Will investigate more during this sprint, but won't possibly fix it.

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Julien Wajsberg [:julienw] from comment #0)
> Actual:
> * we see various things happening, and eventually "Messages" is displayed centered.

Not sure what 'various things happening' behavior looks like. Could you provide a video?

The launching/loading of Messages app seems proper to me on Buri on all versions except v2.2. On 2.2 it would briefly show the title 'Messages' as 'compose' for a split second, but I'm not sure if that's the issue you were looking for.

To stay on topic, I've made a video on Buri 2.0 demonstrating launching of Messages app.

http://youtu.be/X67Jy3atMkw

Note that it makes NO difference whether I have messages in there or not.

Tested on:
Device: Buri 1.4 (no issue)
BuildID: 20141027000203
Gaia: bb76c81f83e1e4acc2d2972a451db2bce78c8f34
Gecko: 1bde54b2e7b0
Version: 30.0 (1.4)
Firmware: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Device: Buri 2.0 (no issue)
BuildID: 20141027000202
Gaia: 2183b4f3ec0eb47ab1f133c31732ec53b08ad253
Gecko: 43bee45176c4
Version: 32.0 (2.0)
Firmware: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Buri 2.1 (no issue)
BuildID: 20141027001201
Gaia: c97463d61f45513a2123b19610386ddbfc916819
Gecko: 4f8c0c021128
Version: 34.0 (2.1)
Firmware: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Buri 2.2 Master (title briefly shows 'compose' before 'Messages' displayed)
BuildID: 20141028040202
Gaia: 6a7fb482a03c5083ef79b41e7b0dfab27527cd04
Gecko: a255a234946e
Version: 36.0a1 (2.2 Master)
Firmware: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Comment 4

[Julien Wajsberg [:julienw]]

Thanks to Wilson, I noticed I had a wrong ttf file in /system/fonts/hidden/ which was producing this to me.

I still want to look closer because I think there is a performance penalty, but then not different than on v2.0.

Comment 5

[Julien Wajsberg [:julienw]]

In sprint 2.1S9 we want to profile what's going on at startup inside the gaia-header machinery.

Comment 6

[Julien Wajsberg [:julienw]]

Here are the huge differences I see:
* v2.0: we load the script in the lazy_loader phase (so after "DOMContenLoaded" event, and after first panel is loaded). It likely means that if we actually needed to recenter the title, it would happen quite late. But for SMS, in english, it's not the case, and that's likely why we didn't have any issue with it. But it's the case for other languages (eg: arabic).
* v2.1: we load the script in a <script defer>.

* v2.0: when the script is loaded it doesn't do much. Every heading is processed in a requestAnimationFrame callback _separately_. The first header produces a sync reflow when acceding to heading.offsetLeft
* v2.1: when the script is loaded it does a lot.
  - adding the CSS for gaia-icons
  - when calling registerElement, "createdCallback" is called synchronously
  - "createdCallback" calls "styleHack".
  - very strange, this line takes a lot of time synchronously: "this.style.visibility = 'hidden';". I would have thought it would produce an asynchronous repaint.

I reported here only the largest times; but there are also lots of small times in other code locations.

Some obvious and easy wins I can see:
* load the gaia icons CSS in SMS' index.html
* don't insert <gaia-header> elements that are not displayed yet; lazy-display them after the first panel
* don't get offsetLeft, instead pass the value as parameter to the component (in the SMS app case, the first panel will always have offsetLeft = 0, other panels will always have offsetLeft = 50).
* do we really need to set the visibility ?
* can we do what's inside createdCallback in a setTimeout?

NI Wilson to have some other insights. But Wilson is in PTO so I guess I'll have to continue myself :)


Here are my WIP branches to measure what happens in the font_size_utils/gaia_header:

v2.0: https://github.com/julienw/gaia/compare/mozilla-b2g:v2.0...v2.0-measure-fontsize-utils?expand=1
v2.1: https://github.com/julienw/gaia/compare/mozilla-b2g:v2.1...v2.1-measure-gaia-header?expand=1

Comment 7

[Ting-Yu Chou [:ting] (away)]

Julien, sorry to bother but Thinker has some concerns about the status of this bug:

1) Do you think we can fix this in the schedule of v2.2?
2) For bug 1074783 comment 66, does that mean you're trying to hard code a parameter in either manifest or javascript? If yes, he thinks this is not a good idea and prefers to remember the calculated size. If not, could you please elaborate on how're you planning to fix it?

Comment 8

[Julien Wajsberg [:julienw]]

Attachment: WIP PR

Here is a first WIP.

It makes the SMS app launch noticeably faster.

There are other ways to make it work. For example we could instead pass a button width, and make gaia header calculate the width from the buttons count. I think it's not that easy though, because we don't know if the buttons are at the left or the right. I think the solution I chose here is simpler and so is more efficient.

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Ting-Yu Chou [:ting] from comment #7)
> Julien, sorry to bother but Thinker has some concerns about the status of
> this bug:
> 
> 1) Do you think we can fix this in the schedule of v2.2?

Actually I want to fix it for v2.1 ;)

> 2) For bug 1074783 comment 66, does that mean you're trying to hard code a
> parameter in either manifest or javascript? If yes, he thinks this is not a
> good idea and prefers to remember the calculated size. If not, could you
> please elaborate on how're you planning to fix it?

Yes I'm planning to hardcode a parameter. The logic is that the application's developer knows more about his application than gaia-header. Therefore as the application developer I can give the information to the gaia-header as a hint to help the calculation. It doesn't sound bad at all in my opinion.

There are several issues with storing the calculated size:
* how do you store it? If you store it using localStorage you'll have a penalty when storing it. If you store it using indexedDB you'll have a penalty when loading it. The only "sane" option is to use a cookie. But then it makes it difficult to know which header produces which value.
* how do you handle the same gaia header that can have several cases and thus calculations. We have such an example in the SMS application.

These 2 issues makes me think that this option is a no-go.

Comment 10

[Ting-Yu Chou [:ting] (away)]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Yes I'm planning to hardcode a parameter. The logic is that the
> application's developer knows more about his application than gaia-header.
> Therefore as the application developer I can give the information to the
> gaia-header as a hint to help the calculation. It doesn't sound bad at all
> in my opinion.

I agree, it quite makes sense to me.

Comment 11

[Wilson Page [:wilsonpage]]

Comment on attachment 8523896 [details] [review]
WIP PR

You probably know this, but the changes need to be landed in gaia-header repo [1].

[1] http://github.com/gaia-components/gaia-header

Comment 12

[Julien Wajsberg [:julienw]]

Yep Guillaume told me; but it's easier to just try out the patch like this.

also the WIP patch broke the fallback, I'm currently making it work again and make things work like before, on top of other optimizations I found when I added some logs. We'll likely want to split this in several patches in the end though...

Comment 13

[Ting-Yu Chou [:ting] (away)]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> (In reply to Ting-Yu Chou [:ting] from comment #7)
> > 2) For bug 1074783 comment 66, does that mean you're trying to hard code a
> > parameter in either manifest or javascript? If yes, he thinks this is not a
> > good idea and prefers to remember the calculated size. If not, could you
> > please elaborate on how're you planning to fix it?
> 
> Yes I'm planning to hardcode a parameter. The logic is that the
> application's developer knows more about his application than gaia-header.
> Therefore as the application developer I can give the information to the
> gaia-header as a hint to help the calculation. It doesn't sound bad at all
> in my opinion.
> 
> There are several issues with storing the calculated size:
> * how do you store it? If you store it using localStorage you'll have a
> penalty when storing it. If you store it using indexedDB you'll have a
> penalty when loading it. The only "sane" option is to use a cookie. But then
> it makes it difficult to know which header produces which value.
> * how do you handle the same gaia header that can have several cases and
> thus calculations. We have such an example in the SMS application.
> 
> These 2 issues makes me think that this option is a no-go.

Thinker, I believe just reading the remembered size can definitely save launch time.

But my understanding is it's not how fast reading rememmbered size is, it's the penalty to take care all the cases when it needs a recalculation. For example, SMS maybe not be running when font changes, so every time it is launched, it needs to check current font and and see if it's the same with the font used for calculation before. Also it probably needs to check language, orientation, etc. Besides, the 2nd issue Julien mentioned above shows also the need of checking the content of gaia-header.

How do you think?

Comment 14

[Julien Wajsberg [:julienw]]

See http://youtu.be/XnEQy2ajSvI to see the effect of the patch:
* Buri on the left runs SMS v2.0 on top of Gaia master
* Buri in the middle runs SMS master on top of Gaia master
* Buri on the right runs SMS master with this patch on top of Gaia master

What can we see?

* we see clearly that v2.0 is still faster by a fair margin
* we also see clearly that the patch improves things a lot

Tomorrow we'll  spend time with Guillaume to pair-review the patch and possibly split it in several pieces.

Comment 15

[Thinker Li [:sinker]]

(In reply to Ting-Yu Chou [:ting] from comment #13)
> How do you think?
I think all you should do is to check it with an experiment.  For example, read several values (5 or more) from localstorage to measure it.

Comment 16

[Thinker Li [:sinker]]

For production build, every time of system upgrade, user agent string would be changed!  If it is true, I think it is a simple solution to check changes.

Comment 17

[Thinker Li [:sinker]]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> See http://youtu.be/XnEQy2ajSvI to see the effect of the patch:
> * Buri on the left runs SMS v2.0 on top of Gaia master
> * Buri in the middle runs SMS master on top of Gaia master
> * Buri on the right runs SMS master with this patch on top of Gaia master
> 
> What can we see?
> 
> * we see clearly that v2.0 is still faster by a fair margin
> * we also see clearly that the patch improves things a lot
> 
> Tomorrow we'll  spend time with Guillaume to pair-review the patch and
> possibly split it in several pieces.

Apparently, the right one is fast in total.  But, middle is faster in show first picture.  By the definition of launch time as first picture, it is actually slower.  The definition could be debated  and changed.  But, we need a way to measure launch time.

Comment 18

[Thinker Li [:sinker]]

Another question is why we show only one entry in first picture?

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to Thinker Li [:sinker] from comment #15)
> (In reply to Ting-Yu Chou [:ting] from comment #13)
> > How do you think?
> I think all you should do is to check it with an experiment.  For example,
> read several values (5 or more) from localstorage to measure it.

The real issue with localStorage is when we _store_ the value.

Comment 20

[Julien Wajsberg [:julienw]]

(In reply to Thinker Li [:sinker] from comment #17)
> (In reply to Julien Wajsberg [:julienw] from comment #14)
> > See http://youtu.be/XnEQy2ajSvI to see the effect of the patch:
> > * Buri on the left runs SMS v2.0 on top of Gaia master
> > * Buri in the middle runs SMS master on top of Gaia master
> > * Buri on the right runs SMS master with this patch on top of Gaia master
> > 
> > What can we see?
> > 
> > * we see clearly that v2.0 is still faster by a fair margin
> > * we also see clearly that the patch improves things a lot
> > 
> > Tomorrow we'll  spend time with Guillaume to pair-review the patch and
> > possibly split it in several pieces.
> 
> Apparently, the right one is fast in total.  But, middle is faster in show
> first picture.  By the definition of launch time as first picture, it is
> actually slower.  The definition could be debated  and changed.  But, we
> need a way to measure launch time.

Yes, I'd agree with you in one way, but we found that partners really want to lower the time to show the first screen (so the first 6 to 9 threads, depending on the screen size).

That said I don't know why we can't try to show the header before the "load" event (which is what's not happening with the patch). It likely means we miss some CSS at load time. BTW this does not happen when I run the app in Firefox.

(In reply to Thinker Li [:sinker] from comment #18)
> Another question is why we show only one entry in first picture?

The answer is quite complicated.
The normal way is that we add thread one by one, without batching. The IPC itself is batched inside the Gecko API, but we still display thread one by one.
Note that we tried doing differently but it didn't prove to be that useful for a much bigger complexity. We're currently doing some deep changes that could make it easier to do, so we may revisit in the future.
So I suspect the main thread is busy between first and second thread on the middle device.

About first and last devices: we can't display a thread before mozL10n is initialized; when mozL10n is intialized we display all threads that are already loaded. That's what happens here.

So basically, the difference between 2nd and 3rd panel is either that mozL10n could initialize earlier or that we could get the thread information earlier (or both). All this made possible because gaia-header is behaving better.

Comment 21

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> (In reply to Thinker Li [:sinker] from comment #17)
> > (In reply to Julien Wajsberg [:julienw] from comment #14)
> > > See http://youtu.be/XnEQy2ajSvI to see the effect of the patch:
> > > * Buri on the left runs SMS v2.0 on top of Gaia master
> > > * Buri in the middle runs SMS master on top of Gaia master
> > > * Buri on the right runs SMS master with this patch on top of Gaia master
> > > 
> > > What can we see?
> > > 
> > > * we see clearly that v2.0 is still faster by a fair margin
> > > * we also see clearly that the patch improves things a lot
> > > 
> > > Tomorrow we'll  spend time with Guillaume to pair-review the patch and
> > > possibly split it in several pieces.
> > 
> > Apparently, the right one is fast in total.  But, middle is faster in show
> > first picture.  By the definition of launch time as first picture, it is
> > actually slower.  The definition could be debated  and changed.  But, we
> > need a way to measure launch time.
> 
> Yes, I'd agree with you in one way, but we found that partners really want
> to lower the time to show the first screen (so the first 6 to 9 threads,
> depending on the screen size).
> 
> That said I don't know why we can't try to show the header before the "load"
> event (which is what's not happening with the patch). It likely means we
> miss some CSS at load time. BTW this does not happen when I run the app in
> Firefox.

I tried playing with async and now I have this fixed too \o/
And it's even faster than before.

The good thing it that merely using async on the gaia-header script is already a win, even without the other changes, so I think we can easily uplift this on v2.1. I'm filing a separate bug just for this.

Comment 22

[Ting-Yu Chou [:ting] (away)]

Nice! Can we apply similar change to the other apps, such as Contacts, Gallery, and Music?

Comment 23

[Julien Wajsberg [:julienw]]

I'll try and see if this improves something :)

Comment 24

[Ting-Yu Chou [:ting] (away)]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> (In reply to Thinker Li [:sinker] from comment #15)
> > (In reply to Ting-Yu Chou [:ting] from comment #13)
> > > How do you think?
> > I think all you should do is to check it with an experiment.  For example,
> > read several values (5 or more) from localstorage to measure it.
> 
> The real issue with localStorage is when we _store_ the value.

Thinker still wants the experiment, so I use the heading's text as the key to keep calcualted margins and font size in localStorage, the code is here:

  https://github.com/janus926/gaia/commit/adc367d8a259855f22312c0f83e54455131ed352

Grabbed profiles from Nexus-4 with v2.2 (reference-workload-medium):

Without the experiment patch:
  http://people.mozilla.org/~bgirard/cleopatra/#report=4bd449e91e5592dd8181d26f5dd04b81d0702fa6
  proto.runFontFit() @ gaia-header.js = 168ms [2103,2272], 25ms [2295,2321], 66ms [2381,2447]

With the experiment patch when the style is read from localStorage:
  http://people.mozilla.org/~bgirard/cleopatra/#report=ab541449ea4dd0e738afc7b6b5f58ac75c710e9f
  proto.runFontFit() @ gaia-header.js = 10ms [1964,1975], 1ms [1994,1996], 2ms [2005,2008]

Comment 25

[Thinker Li [:sinker]]

Julien, could you do profiling for your solution?  We need a number for decisions.  Thank you!

Comment 26

[Julien Wajsberg [:julienw]]

My own profiling is done comparing 2 Buris side by side and adding console.time() at right locations in the code. I never could profile properly using the more advanced tools ;)

I'll prepare an uptodate patch for you so that you can profile as you want.

Comment 27

[Kai-Chih Hu [:khu]]

Bobby, please help to see if we're planning to have this bug fixed in 2.2. Please either +'ed it or -'ed it with feature-b2g flag. 

Thanks.

Comment 28

[Bobby Chien]

Based on above comments, this fix is feasible in v2.2 scope. Tagged as 2.2+ for monitoring.

Comment 29

[Thinker Li [:sinker]]

julien, is it ok to assign this bug to you?

Comment 30

[Julien Wajsberg [:julienw]]

Yes sure.

(sorry I could not progress on comment 26, last 2 days were busy with ideation groups; should be less busy today and tomorrow)

Comment 31

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 32

[Julien Wajsberg [:julienw]]

Attachment: Another WIP PR

Comment 33

[Julien Wajsberg [:julienw]]

Thinker, Ting-Yu, I'd be happy if you can profile both PR here. I don't see much improvement on Buri, except on the second PR, only sometimes. I don't have 2 Flames here to do a head to head comparison.

Thanks!

Comment 34

[Ting-Yu Chou [:ting] (away)]

Julien, here're the profiles I captured on a Nexus-4, the result shows attachment 8538765 [details] [review] improves most, even more than bug 1102154. Later if you want to get more profiles, you can try the command below and maybe check MDN [1] as well. Let me know if you need any help.

  |$ ./profile.sh start -p b2g && ./profile.sh start -p Homescreen && ./profile.sh start -p [preallocated pid]; sleep 5; ./profile.sh capture|

Gaia: c428ab85656394824ea732a95e79e069fe433f5a

No patch: https://people.mozilla.org/~bgirard/cleopatra/#report=72af6a93fad77360a40c34f7cba98d54a4a7b6c5
  e() @ gaia-header.js = 180ms [2299,2480]
  runFontFit = 29ms [2330,2360], 16ms [2378,2395], 34ms [2444,2479]

Attachment 8538725 [details]: https://people.mozilla.org/~bgirard/cleopatra/#report=53ea8e08b113c259e8e31f8bdb834bb8838948be
  e() @ gaia-header.js = 118ms [2169,2288]
  runFontFit = 13ms [2200,2214], 65ms [2925,2991]

Attachment 8538765 [details]: https://people.mozilla.org/~bgirard/cleopatra/#report=2d250710449d51c5ac6dad355108920bbb2365a6
  e() @ gaia-header.js = 53ms [2179,2233]
  runFontFit = 6ms [2193,2200], 6ms [2204,2211], 2ms [2211,2214]

Bug 1102154: https://people.mozilla.org/~bgirard/cleopatra/#report=4b3637148d37ef2161a2e575d91a09f15405bbb6
  e() @ gaia-header.js = 112ms [2312,2425]
  runFontFit = 9ms [2343,2353], 1ms [2371,2373], 1ms [2422,2424]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler

Comment 35

[Ting-Yu Chou [:ting] (away)]

BTW, when running with attachment 8538765 [details] [review], I can see a white screen when launch SMS. This seems a side effect?

Comment 36

[Julien Wajsberg [:julienw]]

(In reply to Ting-Yu Chou [:ting] from comment #35)
> BTW, when running with attachment 8538765 [details] [review], I can see a white
> screen when launch SMS. This seems a side effect?

You mean, all the time, or from time to time?

about attachment 8538765 [details] [review]: it does less things and is not really complete, it was more a quick hack to try something. Especially it does not initialize the gaia-headers that are not displayed at startup. I'm glad it improves more, maybe that means we should initialize those headers only the first time they're displayed. In attachment 8538725 [details] [review] I initialize those headers once the first panel is displayed only.

I don't really understand how you find the gaia-header ful time. I'm not sure the number you report relates to something in the real life. I mean, I'm not sure that the full time you report is spent only in gaia-header.js. The runFontFit nuumbers are more sensible though.

It's also weird you have only two calls for runFontFit for Attachment 8538725 [details], because we have 3 headers, we should have 3  calls.

Weird also that you have 3 calls for Attachment 8538765 [details], because we're supposed to ignore completely 2 headers.

I'll look at my code, it's probable I have something wrong :)

Comment 37

[Ting-Yu Chou [:ting] (away)]

(In reply to Julien Wajsberg [:julienw] from comment #36)
> (In reply to Ting-Yu Chou [:ting] from comment #35)
> > BTW, when running with attachment 8538765 [details] [review], I can see a white
> > screen when launch SMS. This seems a side effect?
> 
> You mean, all the time, or from time to time?

Sometimes.

> I don't really understand how you find the gaia-header ful time. I'm not
> sure the number you report relates to something in the real life. I mean,
> I'm not sure that the full time you report is spent only in gaia-header.js.
> The runFontFit nuumbers are more sensible though.

Take [2169,2288] as an example, in that period you can fild the samples have |e() @ gaia-header.js| on their stack.
 
> It's also weird you have only two calls for runFontFit for Attachment
> 8538725 [details], because we have 3 headers, we should have 3  calls.

Note the second one has attributeChanged() on the stack.
 
> Weird also that you have 3 calls for Attachment 8538765 [details], because
> we're supposed to ignore completely 2 headers.
> 
> I'll look at my code, it's probable I have something wrong :)

Probably they're the same one. The timestamps are close actually, between them are some missed samples: runFontFit = 6ms [2193,2200], 6ms [2204,2211], 2ms [2211,2214].

Comment 38

[Julien Wajsberg [:julienw]]

> Note the second one has attributeChanged() on the stack.

So this is likely when we remove the "skip-init" attribute to one of the headers, this is good :) It's also likely that the profile miss some samples to see all calls.

> Take [2169,2288] as an example, in that period you can fild the samples have |e() @  
> gaia-header.js| on their stack.

Mmm I see. I'll try to relate with console.time()/timeEnd() calls in the code itself ;)

Comment 39

[Julien Wajsberg [:julienw]]

Attachment: 3rd WIP PR

Comment 40

[Julien Wajsberg [:julienw]]

I updated attachment 8538725 [details] [review] with latest code, and created attachment 8540184 [details] [review] with some more hack (I'm sure it fails in some situations).

I've noticed that the line [1] is really costly too so I moved this to `init` as well.

[1] https://github.com/gaia-components/gaia-header/blob/master/gaia-header.js#L37

I'd like to try other things like setting display:none and see if this improves, instead of moving all this block.

All in all I'm not very happy with all this work because I fail to see a real improvement on the device. Yes, when I measure, I see I gain ~100ms on loading gaia-header, and even more with the 3rd patch, but because it's loaded using "async" since bug 1101532 I think it's not in the critical path now.

I'd like other inputs to know if I should move forward with all this.

Comment 41

[Thinker Li [:sinker]]

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #40)
> loading gaia-header, and even more with the 3rd patch, but because it's
> loaded using "async" since bug 1101532 I think it's not in the critical path
> now.

Do you mean that you just make these work loads to async, but not save or avoid it?

Comment 42

[Julien Wajsberg [:julienw]]

In bug 1101532, that's what I did.

Here I avoid the reflow for the SMS app for the header in first panel, and avoid reflow during launch for other headers.

What I explain in comment 40 is that I don't see an improvement for the "display first panel of thread" scenario when I compare Buri or Flame side by side, when I compare the work here and current master (containing patch to bug 1101532). Today I'll try to measure using make test-perf.

Comment 43

[Julien Wajsberg [:julienw]]

Some results of using test-perf with 50 iterations.

This is the result for moz-app-visually-complete which is the measurement we're optimizing for.
I'm using my own DB, which should be quite similar to the "high" workload.
I'm running on a Flame with a master build.

master: mean = 1530, median = 1514, std = 119
with patch from attachment 8540184 [details] [review]: mean = 1500, median = 1460, std = 146
with patch from attachment 8538725 [details] [review]: mean = 1483, median = 1480, std = 70

So the median of attachment 8540184 [details] [review] is better, but the mean is higher. So I think we can move forward with the first patch in attachment 8538725 [details] [review], and then move forward in a separate bug with attachment 8540184 [details] [review].

As I said earlier, the improvements are not as good as the pure profiler showed, but it seems we still have something even if I can't really see it on the device.

Comment 44

[Ting-Yu Chou [:ting] (away)]

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #43)
> master: mean = 1530, median = 1514, std = 119
> with patch from attachment 8540184 [details] [review]: mean = 1500, median = 1460,
> std = 146
> with patch from attachment 8538725 [details] [review]: mean = 1483, median = 1480,
> std = 70

How about attachment 8538765 [details] [review]?

Comment 45

[Julien Wajsberg [:julienw]]

The results from comment 34 were not valid because attachment 8538765 [details] [review] was not finished (we did not init other gaia headers except the first panel) so I think the improvements were not meaningful compared to the first one. I can try to pursue it if you think it's useful.

Comment 46

[Ting-Yu Chou [:ting] (away)]

I tried another WIP [1] to use a template element instead of setting shadow DOM's innerHTML directly, it improves ~50ms (117ms -> 69ms). I expect it could improve more after adopt the skip-init you created.

Profile: http://people.mozilla.org/~bgirard/cleopatra/#report=f1f43739f712ec6ae233bdd406292c1684401c57

[1] https://github.com/janus926/gaia/commit/3289cb69f3c995a6fb8aa3228fa2d94f1c0b98b9

Comment 47

[Julien Wajsberg [:julienw]]

You should ask Wilson about this, I think he used to use this but then changed because of an issue... but I don't remember it so I don't remember if it's still a current issue.

If we can do this, I'd be very happy :)

Comment 48

[Ting-Yu Chou [:ting] (away)]

Wilson, per comment 46 & 47, do you remember why template element is not used for creating the shadow DOM of gaia header?

Comment 49

[Wilson Page [:wilsonpage]]

(In reply to Ting-Yu Chou [:ting] from comment #48)
> Wilson, per comment 46 & 47, do you remember why template element is not
> used for creating the shadow DOM of gaia header?

Wow that's interesting! We moved from cloning `template.content` to setting `innerHTML` within `createdCallback` only to simplify the code. If we get a win from reverting back to using <template> then we should roll this out across all the gaia-components, this may require some adjustments to the base gaia-component module.

Comment 50

[Julien Wajsberg [:julienw]]

I updated the first WIP PR with the new code from bug 1112131. I didn't test the code yet so you miht want to wait before trying it ;)

Ting-Yu, can you please file a separate bug about using a template elemnt?

Comment 51

[Ting-Yu Chou [:ting] (away)]

(In reply to Julien Wajsberg [:julienw] (PTO until 1/5) from comment #50)
> Ting-Yu, can you please file a separate bug about using a template elemnt?

Bug 1119626.

Comment 52

[Bobby Chien]

Hi Julien, could you help to follow up this case?

Comment 53

[Julien Wajsberg [:julienw]]

If you mean bug 1119626, I can try, but if someone else can do it it would be better. This is not normaly my primary area of work :)

Comment 54

[Julien Wajsberg [:julienw]]

Updated PR because the one from yesterday had an issue with our build system.

Today I'll do try runs to determine if we still have launch time improvements.

Comment 55

[Ting-Yu Chou [:ting] (away)]

Julien, sorry to bother, but may I understand when do you think the patch will be ready to land (roughly)? BTW, since the patches change not only gaia-header but also the app, will there be followup bugs for the other apps?

Comment 56

[Julien Wajsberg [:julienw]]

See the dependent bug, Wilson took over the patch and he asked me a review on it. I should be able to do this review today.

As I said earlier too, I didn't see much improvements on other apps when quickly testing, but we can look again.

Comment 57

[Johan Lorenzo [:jlorenzo]]

Triage: Blocking per the new performance criteria in 2.2.

Comment 58

[Bobby Chien]

julien, could you help to update this case? I saw all dependencies are resolved. could we resolve meta case? thanks.

Comment 59

[Julien Wajsberg [:julienw]]

This is not a meta.

I need to update the SMS patch to use the new functionality we landed in bug 1112131. I think I can have an updated patch ready for review later today.

Comment 60

[Julien Wajsberg [:julienw]]

Comment on attachment 8538725 [details] [review]
github PR

Hey Steve,

what do you think of this?

I measured again, I get ~50ms improvement compared to master.

Comment 61

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #60)
> Comment on attachment 8538725 [details] [review]
> github PR
> 
> Hey Steve,
> 
> what do you think of this?
> 
> I measured again, I get ~50ms improvement compared to master.

Some questions about the new functionality in gaia header:
- Does it meaningful to set "no-font-fit" to a gaia header that no l10n id provided at first(like messages panel)?
- Maybe we also need this "no-font-fit" for information/group panel(Now we have seperate panels with independent gaia header for information/group panel)?

Comment 62

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #61)
> (In reply to Julien Wajsberg [:julienw] from comment #60)
> > Comment on attachment 8538725 [details] [review]
> > github PR
> > 
> > Hey Steve,
> > 
> > what do you think of this?
> > 
> > I measured again, I get ~50ms improvement compared to master.
> 
> Some questions about the new functionality in gaia header:
> - Does it meaningful to set "no-font-fit" to a gaia header that no l10n id
> provided at first(like messages panel)?
> - Maybe we also need this "no-font-fit" for information/group panel(Now we
> have seperate panels with independent gaia header for information/group
> panel)?

Yes I think you're right for both questions :)

I'll do these changes and request another review.

Comment 63

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #62)
> > Some questions about the new functionality in gaia header:
> > - Does it meaningful to set "no-font-fit" to a gaia header that no l10n id
> > provided at first(like messages panel)?

Actually I kept it for consistency and also because we might want to skip some more action than just font-fit in the future (like skipping the shadowRoot creation in the "create" callback). So even if it brings nothing right now, it can bring something "for free" later.

Comment 64

[Julien Wajsberg [:julienw]]

Ready for another review. I added the follow-ups in a separate commit.

Comment 65

[Steve Chung [:steveck]]

Comment on attachment 8538725 [details] [review]
github PR

Thanks for the description, overall looks fine and I'm ok to keep the "no-font-fit". However I still have some concern about the title-start/title-end offset, since it's based on the button's width, which is defined in gaia-header instead of message app. So it seems not robust that any button styling changes in gaia-header will cause title space fit issue regressed easily. Do you think we should add some warning in the comment?

Comment 66

[Julien Wajsberg [:julienw]]

I know what you mean; maybe we should have used a "button-count" attribute instead, and leave gaia-header multiply this with the button width.

Hopefully any issue will be very visible here because it's in the main panel. I'll add a small comment to indicate the possible issue.

Comment 67

[Julien Wajsberg [:julienw]]

NI me to not forget to land.

Comment 68

[Julien Wajsberg [:julienw]]

master: https://github.com/mozilla-b2g/gaia/commit/8c6aaa86657108a2acbfdc4dc1934e0176ceb4f9

Comment 69

[Julien Wajsberg [:julienw]]

Comment on attachment 8538725 [details] [review]
github PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: performance launch time
[Testing completed]: yes, intensively
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 70

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.2 uplift.

Comment 71

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1089920-v2.2-uplift > mozilla-b2g:v2.2

Comment 72

[Julien Wajsberg [:julienw]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/95092f507a5e12fbc9f17cb9c05b2e47df6b54da