1101532 - [Messages] load the gaia header asynchronously

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Once bug 1101518 is fixed, we can load the gaia header in an asynchronous way.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: github PR

Depending on whether async scripts are automatically skipped by build's optimize step, we might need to add data-skip-optimize on the script.

Comment 2

[Julien Wajsberg [:julienw]]

I tried other apps (settings, gallery, video, music, contacts) but didn't see improvements in other apps. It was either worse or same launch performance.

Comment 3

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

Steve asked me to do a profile with the PR, I did it on a Nexus-4: http://people.mozilla.org/~bgirard/cleopatra/#report=eef2263ef7a5e4e1c1ddc9fae5c317de019d415f.

There're still 3 large runFontFit(): 165ms [1985,2151], 25ms [2175,2202], 65ms [2260,2326], which shows no improvements (see bug 1089920 comment 24).

I applied the PR and |cd gaia; make production|, Julien, did I miss anything?

Comment 4

[Steve Chung [:steveck]]

After some simple profiling I didn't feel a significant improvement, so I asked Ting-Yu for more detailed information. It looks like the reflow effort for runfontfit is still there even we put the gaia-header script execution in async. Could you explain which part of the overhead could be eliminated in your patch?

Comment 5

[Julien Wajsberg [:julienw]]

Hey

Yep this patch does not remove the reflow at all. The bigger patch in bug 1089920 does. I'm currently tring to split the patch from bug 1089920 into smaller pieces, this bug is one of the pieces.

However I saw a really good difference with only this patch when testing on Buri (compared side by side, I can try to provide a video later).

I admit I don't know exactly where the difference comes from. It could also be different on Flames, I'll see if I can borrow a Flame today to look at this.

Comment 6

[Julien Wajsberg [:julienw]]

See http://youtu.be/Ct7Q1CyrsdY for a comparison with 2 Flames. The Flame on the right has the patch.

See http://youtu.be/-dhKs4uGoIE for a comparison with 2 Buri. The Buri on the right has the patch.

(note: all phones have the same data workload, which is a workload from my own usage of the SMS aplication).

On both cases, we see this patch has a good effect. The effect is greater on Buri, which is expected. On Flame it stays quite small.

Comment 7

[Steve Chung [:steveck]]

Will find some buri device for testing this, since flame couldn't see any difference per my testing result.

Comment 8

[Steve Chung [:steveck]]

I can't find 2 buri for side by side comparison, so the only thing I can do is profiling some loading record from it.

To be honest, I still can't find much improvement on buri in average. Since the loading time varies a lot, it's possible that we can see the device with patch runs faster sometimes. But the difference in average would be just few ms.

Another issue developer HUD seems unable to fetch the correct app loaded time after this patch applied. Maybe this patch did affect the timing of window load event(The load time usually raised up to 10sec, while the DOMContentLoaded event should be around 1~2 sec). I didn't discover this issue on flame, but I'm not sure if is safe to take this patch. Hi Julien, do you think it's fine to land it and leave this issue since we will not profile for buri?

Comment 9

[Julien Wajsberg [:julienw]]

To be honest I found this was especially good when we have bug 1089920 too.

Note that the time given by the developer HUD is really not the good way to measure this, because it does not measure the "time to first panel", which is what we want here. What you want to run is the performance tests (but it's more painful to run than just running 2 phones side by side).

Regarding why the load time moves to 10sec, I suspect it happens also without the patch. I think it's more a consequence that we moved loading a lot of things to DOMContentLoaded. What can happen is that we load the first panel before the "load" event, and so we start lazy loading scrips before the "load" event, and consequently the "load" event is delayed until after all these scripts are loaded.

If you look at the videos from comment 6 (note you can reduce the play speed to 0.25x on youtube) you see that on Flame we gain between 0 and 50ms while on Buri we gain ~200ms (quite constant gain from what I've seen).

So I'd definitely take this patch. The improvement is either 0 or better on Flame, and always better on Buri. But I'm quite partial here :)

Comment 10

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> 
> Regarding why the load time moves to 10sec, I suspect it happens also
> without the patch. I think it's more a consequence that we moved loading a
> lot of things to DOMContentLoaded. What can happen is that we load the first
> panel before the "load" event, and so we start lazy loading scrips before
> the "load" event, and consequently the "load" event is delayed until after
> all these scripts are loaded.

You're right that this still happends without this patch. The original reproduce rate is probably around 10~20%, but it will be increased up to ~80% after patch applied. So how about adding some warning comment for async attribute? It seems safe since message app listens to DOMContentLoaded event, but not for the app listens to load event. 

> 
> If you look at the videos from comment 6 (note you can reduce the play speed
> to 0.25x on youtube) you see that on Flame we gain between 0 and 50ms while
> on Buri we gain ~200ms (quite constant gain from what I've seen).
> 
> So I'd definitely take this patch. The improvement is either 0 or better on
> Flame, and always better on Buri. But I'm quite partial here :)

I'm neutral about taking the patch, since it didn't introduce regression at least :)

Comment 11

[Steve Chung [:steveck]]

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

Let's land this patch first and keep Bug 1089920 moving!

Comment 12

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #10)
> (In reply to Julien Wajsberg [:julienw] from comment #9)
> > 
> > Regarding why the load time moves to 10sec, I suspect it happens also
> > without the patch. I think it's more a consequence that we moved loading a
> > lot of things to DOMContentLoaded. What can happen is that we load the first
> > panel before the "load" event, and so we start lazy loading scrips before
> > the "load" event, and consequently the "load" event is delayed until after
> > all these scripts are loaded.
> 
> You're right that this still happends without this patch. The original
> reproduce rate is probably around 10~20%, but it will be increased up to
> ~80% after patch applied.


so, from the logic I outlined earlier, it definitely means that the first panel load happens sooner more often, right ? :)


> So how about adding some warning comment for async
> attribute? It seems safe since message app listens to DOMContentLoaded
> event, but not for the app listens to load event. 

I don't think the issue is with the async attribute by itself. Maybe I could add a comment in startup.js where we listen for DOMContentLoaded?

Comment 13

[Julien Wajsberg [:julienw]]

I added the comment, rebased, pushed, and waiting for a new try run.

Comment 14

[Julien Wajsberg [:julienw]]

GrREEN!!!

master: 8256c59f6aae0065d81c8fc7bde06804a9270c4f

Comment 15

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> 
> so, from the logic I outlined earlier, it definitely means that the first
> panel load happens sooner more often, right ? :)
> 
Maybe, but I definitely hope so

> 
> > So how about adding some warning comment for async
> > attribute? It seems safe since message app listens to DOMContentLoaded
> > event, but not for the app listens to load event. 
> 
> I don't think the issue is with the async attribute by itself. Maybe I could
> add a comment in startup.js where we listen for DOMContentLoaded?

Ya it's not the async's "problem", it would be fine if user knows the calling sequence and use it properly.