Closed Bug 1210860 Opened 5 years ago Closed 5 years ago

Video app launch time regression from 2.2 release

Categories

(Firefox OS Graveyard :: Gaia::Video, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+)

RESOLVED FIXED
FxOS-S10 (30Oct)
blocking-b2g 2.5+

People

(Reporter: hkoka, Assigned: rnicoletti)

References

Details

(Keywords: perf, regression, Whiteboard: [Profile-wanted][landing eta 10/27])

Attachments

(2 files)

From the discussion at https://groups.google.com/forum/#!topic/mozilla.dev.fxos/HeGci2jormU :

Video v2.2 cold launch: 1115ms
Video current cold launch: 1309ms (~190ms regression)
Video v2.2 USS: 12.13MB
Video current USS: 13MB (acceptable)
Russ assigning this to you for investigation and also NI'ing David so he is aware. 

Thanks
hema
Assignee: nobody → rnicoletti
blocking-b2g: --- → 2.5+
Flags: needinfo?(dflanagan)
Priority: -- → P1
Duplicate of this bug: 1211396
My first reaction to reading the "Current state of performance in gaia" thread was that it was probable there was a gecko regression affecting startup time. This is because I was quite sure there have been no changes to the video app since 2.2 that would affect startup time.

To test my theory, I ran tests comparing the 2.2 video app on 2.5 gecko to the 2.5 video app on 2.5 gecko. There was virtually no difference in cold startup time. I also ran tests with the 2.2 app on 2.2 gecko, which proved to have much faster startup times compared to the tests with the 2.5 gecko.

This information, along with the fact that many apps have regressed startup performance since 2.2, suggests to me there is a gecko startup time regression.
I was going to suggest exactly the experiment that Russ already conducted, and I agree with him that this is a Gecko issue, not a Gaia issue.

I wonder if we can do some more investigation to see where the regression is.  If we start with the 2.2 app and add more performance.mark() calls we can do a more fine-grained raptor run on 2.2 and 2.5 to see what is slowing down.

I'd be interested to have a performance.mark() at:

- the very first line of JavaScript that runs
- when the DOMContentLoaded event is fired
- when the load event is fired
- when new MediaDB() is callled
- when MediaDB sends the enumerable event
- when the localized event is sent (does the l10n library still fire that event?)
Flags: needinfo?(dflanagan)
That may take a while, but why not bisect gecko while keeping the same v2.2 video app? I'm not sure if we have enough tooling automate something like:
0) start hg bisect with tip=bad, tag_2.2=good
1) build gecko
2) run raptor
3) compare the perf of this build to gecko-2.2+video-2.2 and gecko-2.5+video-2.2 and decide to mark the build as "good" or "bad"
ni Kanru and Ting for gecko profiling.
Flags: needinfo?(kchen)
Whiteboard: [Profile-wanted]
We have some numbers in bug 1194651. I think Ting has the tool to bisect.
Flags: needinfo?(kchen)
Blocks: 1194651
Raptor [1] shows visuallyLoaded jumps a bit higher around 7/10 and 7/24, will try to bisect.

Eli, did Raptor detect regression at that time?

[1] https://raptor.mozilla.org/dashboard/script/measures?var-device=flame-kk&var-memory=319&var-branch=master&var-test=cold-launch&from=1425181570254&to=1444621853177&var-metric=visuallyLoaded
Flags: needinfo?(eperelman)
Captured a profile [1] on Flame 1G, noticed two things:

1. extensions.js is executed (~120ms) in child process before loading the content
2. a big idle chunk (>200ms) waiting for indexed DB response in child process before visuallyLoaded

[1] http://people.mozilla.org/~bgirard/cleopatra/#report=f3c6c544ca562d1564f69887d423058a4c8d3b76
    Gecko: b68eab795f9d
    Gaia: 87f5c9d55ab6a77dcfa48a3f3a8b4f5016f3c657
(In reply to Ting-Yu Chou [:ting] from comment #9)
> 1. extensions.js is executed (~120ms) in child process before loading the
> content

Fabrice, can we skip this if there's no extension in the zip?
Flags: needinfo?(fabrice)
(In reply to Ting-Yu Chou [:ting] from comment #10)
> (In reply to Ting-Yu Chou [:ting] from comment #9)
> > 1. extensions.js is executed (~120ms) in child process before loading the
> > content
> 
> Fabrice, can we skip this if there's no extension in the zip?

Or we should do this in preload.
(In reply to Ting-Yu Chou [:ting] from comment #9)
> 2. a big idle chunk (>200ms) waiting for indexed DB response in child
> process before visuallyLoaded

It seems openning the indexed DB (videos) is delayed because system app is handling mozbrowserloadend on b2g process main thread.
(In reply to Ting-Yu Chou [:ting] from comment #11)
> (In reply to Ting-Yu Chou [:ting] from comment #10)
> > (In reply to Ting-Yu Chou [:ting] from comment #9)
> > > 1. extensions.js is executed (~120ms) in child process before loading the
> > > content
> > 
> > Fabrice, can we skip this if there's no extension in the zip?
> 
> Or we should do this in preload.

It's unrelated to having the extension in the app. It's to hook up the WebExtensions api that can be used in any frame. That being said, if we can preload it let's do!
Flags: needinfo?(fabrice)
Numbers comparing 512MB configuration (from [1])

Metric                 Mean      StdDev     95% Bound
======                 ====      ======     ========= 
v2.2 visuallyLoaded    1110.267  48.480     1127.615
master visuallyLoaded  1256.967  43.514     1272.538

v2.2 uss               10.303    0.791      10.58
master uss             13.248    0.416      13.397

512MB performance regression: 12.85%
319MB performance regression: 17.40%

[1] https://mail.google.com/mail/u/0/#inbox/1505d81f35bc5c6b
(In reply to Russ Nicoletti [:russn] from comment #14)
> Numbers comparing 512MB configuration (from [1])
> 
> Metric                 Mean      StdDev     95% Bound
> ======                 ====      ======     ========= 
> v2.2 visuallyLoaded    1110.267  48.480     1127.615
> master visuallyLoaded  1256.967  43.514     1272.538
> 
> v2.2 uss               10.303    0.791      10.58
> master uss             13.248    0.416      13.397
> 
> 512MB performance regression: 12.85%
> 319MB performance regression: 17.40%
> 
> [1] https://mail.google.com/mail/u/0/#inbox/1505d81f35bc5c6b

What gecko/gaia revision correspond to the v2.2 and master in your test?
The numbers come from Eli's post, the link to the post is at [1] in comment 14.
(In reply to Russ Nicoletti [:russn] from comment #16)
> The numbers come from Eli's post, the link to the post is at [1] in comment
> 14.

Ah, the link is pointing to a gmail inbox.
My v2.2 build comes from:

https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/tinderbox-builds/mozilla-b2g37_v2_2-flame-kk-eng/20151007064630/

In sources.xml:
  Gaia: 885647d92208fb67574ced44004ab2f29d23cb45
  Gecko: ab6c34bfacf7

The master build came from flame-kk latest, also from PVT, some time on October 12 PT.
See Also: → 1214133
No, Raptor didn't specifically find a regression for Video at that time, but that doesn't mean one didn't occur.

Also, here is an updated link to the performance baseline comparison:

https://gist.github.com/eliperelman/b81a27b7f7f3b3f75f49
Flags: needinfo?(eperelman)
See Also: → 1214516
Keywords: perf, regression
Ting, are you continuing to work on this bug? It seems you have found some potential performance improvements. Would you be able to make the changes?
Flags: needinfo?(janus926)
Bug 1214133 just landed, am still working on bug 1214516. And for the slow mediadb openning, probably bug 1179723 helps a bit.
Flags: needinfo?(janus926)
As opening mediaDB is asynchronous, is it possible that we move initDB() as early as possible? So we will have more chances to run on B2G process main thread.
initDB() is being called very early in the startup process, as soon as we get the `mozL10n.once` callback. It can't be called before then as the db initialization process displays localized text in some circumstances.

In general, if it was possible to improve startup performance by making app changes I would be glad to do it. However, it seems we're going to get the most benefit from gecko changes. Also, I would rather we focus on the reason for the performance regression. Given there have not been any significant app changes since 2.2, I think it's best to focus on the gecko changes that have caused the performance regression.
Actually, after patch from bug 1197454, you do not use any synchronous API that would have to be guarded by mozL10n.once.

The only thing you'd want to guard with mozL10n.once/mozL10n.ready are two places where you use `navigator.mozL10n.language.direction` - although I believe you can instead use `document.documentElement.getAttribute('dir')` in both and remove mozL10n.once/ready all together.
Captured two profiles on Aries:

  Before: http://people.mozilla.org/~bgirard/cleopatra/#report=b45d528bbd6caef095453ec2f1915f623167ad91
  Now: http://people.mozilla.org/~bgirard/cleopatra/#report=1e4fee3cd84c4aefd3cf668398b69993309d32a0

Noticable things are:
  1. Extra js in child process:
    - ExtensionContent.jsm ~10ms
    - video_stats.js >>30ms
  2. System app doing app state transition merges to a huge chunk, and with l10n mutations.

Before:
  gaia=529e05df920
  gecko=b07950eeabc
> System app doing app state transition merges to a huge chunk, and with l10n mutations.

The onMutations from l10n.js takes 4ms total, and is not a significant source of delay, am I correct?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> > System app doing app state transition merges to a huge chunk, and with l10n mutations.
> 
> The onMutations from l10n.js takes 4ms total, and is not a significant
> source of delay, am I correct?

It's 16ms [104385,104600], not significant, just different.
I am currently testing a patch to the video app that enumerates the media db when it is "enumerable" (as opposed to when it is fully ready). The initial results I'm seeing is 100-200ms improvement in visuallyLoaded performance. Actually, the patch has three changes related to performance improvement but it seems only the "onenumerable" change has a significant affect. I will provide more details when I attach the patch, which I hope to be later today.
Status: NEW → ASSIGNED
This patch has three sets of changes toward improving video app startup performance:

1) Delay gaia-header 'font-fit' logic to after visuallyLoaded
2) Init db without waiting for mozL10n.once
3) Enumerate db when it becomes enumerable as opposed to when it is fully ready.

David, I'm assuming I will get feedback from you regarding all three changes.

Zibi, does `IntlHelper.define` still need to be guarded by mozL10n once? -- https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/video.js#L145
Attachment #8677053 - Flags: feedback?(gandalf)
Attachment #8677053 - Flags: feedback?(dflanagan)
Russ,

Do you know anything about the VideoStats stuff in video.js? I have no idea who added that and whether the pref that records the stats is even enabled. Comment #25 indidcates that it adds startup time. So another easy fix would be to lazy-load it.  You could even check whether dom.player.getVideoPlaybackQuality is defined and not even bother loading it if not.
Comment on attachment 8677053 [details] [diff] [review]
video app performance tweaks

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

Russ,

It looks like you did the things needed to get the speedups you want. But you've introduced a couple of bugs in the process...  Overall, this is looking good however.

::: apps/video/index.html
@@ +51,5 @@
>      <div id="app-container">
>        <div id="two-column-spearator"></div>
>        <section role="region" id="thumbnail-views">
> +        <gaia-header ignore-dir id="picker-header" action="back" class="thumbnails-list"
> +         title-start="0" title-end="0" no-font-fit>

Since this header has an action attribute, that means there is a 50px button on the left side of the header. So you should have title-start="50" instead of title-start=0 here.  Otherwise the user might see a flash where the title overlaps the back button, I think.

Same for the other two headers below. 

See gallery/index.html for examples.

::: apps/video/js/db.js
@@ -42,5 @@
> -  videodb.onready = function() {
> -    storageState = false;
> -    updateDialog();
> -    enumerateDB();
> -  };

You can't completely get rid of the onready event handler. When the user plugs the phone into a computer via USB to transfer files, the sdcard gets unmounted, and MediaDB sends an unavailable event. When the sdcard is mounted again, MediaDB sends a ready event. So we still need code to undo the unavailble event and re-scan the sdcard.

@@ +64,5 @@
> +      videodb.state === MediaDB.ENUMERABLE) {
> +    onDbEnumerable();
> +  } else {
> +    videodb.onenumerable = onDbEnumerable;
> +  }

the if case of this if/else will never be true. This code is still inside initDB() and this is running synchronously after the call to new MediaDB(). It won't become enumerable or ready until after this function has returned. So you can just define the onenumerable event handler like you do all the others.


But you do need to have a ready event handler, and you need to make sure that the ready event handler does not just go and enumerate the db again right after the enumerable event handler does.

@@ +136,5 @@
> +      // til now to postpone the overhead.
> +      var headers = document.querySelectorAll('gaia-header');
> +      for (var i = 0; i < headers.length; ++i) {
> +        headers[i].removeAttribute('no-font-fit');
> +      }

consider putting this in a setTimeout() so it gets invoked a little later and does not slow down the enumeration.  But it is okay here if you want to keep things simple.

::: apps/video/test/unit/video_test.js
@@ +26,4 @@
>  requireApp('/video/js/video_utils.js');
>  requireApp('/video/test/unit/mock_metadata.js');
>  requireApp('/video/test/unit/mock_mediadb.js');
> +requireApp('/video/test/unit/mock_forward_rewind_controller.js');

I can't tell why any of these test changes are needed, but I'm assming that the tests broke and this fixes them.
Attachment #8677053 - Flags: feedback?(dflanagan) → feedback+
> Zibi, does `IntlHelper.define` still need to be guarded by mozL10n once? -- https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/video.js#L145

Nope, it doesnt!

The only thing that requires the guard is 'navigator.mozL10n.language.direction' - and you can safely move it to 'document.documentElement.dir' and then you don't need a guard at all.
Comment on attachment 8677053 [details] [diff] [review]
video app performance tweaks

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

As I said in my previous comment - I think it makes sense to move mozL10n.language.direction to documentElement.dir - the reason behind it is subtle. The former gives you a direction of the language selected in the localization context. The latter is a direction of the document.

In 99.9% of cases, those two are exactly the same. But during racy moments (app bootstrapping or language change) they may diverge and l10n context may be in a "in transition" state when we know the new language is requested but we didn't load resources for it yet.

Document on the other hand is synchronous. It's in one language or another.
And since you are altering DOM you should follow document's direction.

(Alternatively, you might want to set the percent as data-percent attribute and just pick it from CSS, that would be semantically even better, but that's not related to l10n)

::: apps/video/js/video.js
@@ +147,4 @@
>  });
>  
> +// Tell performance monitors that our chrome is visible
> +window.performance.mark('navigationLoaded');

You may want to keep the performance mark inside mozL10n.once to indicate that that step is ready only when HTML's localization is complete.

But you don't need to block init() on it or IntlHelper.define.
Attachment #8677053 - Flags: feedback?(gandalf) → feedback+
David, question regarding "You can't completely get rid of the onready event handler. When the user plugs the phone into a computer via USB to transfer files, the sdcard gets unmounted, and MediaDB sends an unavailable event. When the sdcard is mounted again, MediaDB sends a ready event. So we still need code to undo the unavailble event and re-scan the sdcard."

From my reading of the code, we remember that the db has already been enumerated and subsequent calls to enumerateDB return without enumerating. With that being said, my understanding is we need to handle the 'ready' event to remove the 'unplug the phone' overlay, which is accomplished by these two lines:

         storageState = false;
         updateDialog();

I don't believe we need to invoke 'enumerateDB' when we get the 'ready' -- assuming we have already enumerated the db in response to the 'enumerable' event.

What do you think?
Flags: needinfo?(dflanagan)
Whiteboard: [Profile-wanted] → [Profile-wanted][landing eta 10/27]
Target Milestone: --- → FxOS-S10 (30Oct)
(In reply to Russ Nicoletti [:russn] from comment #34)
> David, question regarding "You can't completely get rid of the onready event
> handler. When the user plugs the phone into a computer via USB to transfer
> files, the sdcard gets unmounted, and MediaDB sends an unavailable event.
> When the sdcard is mounted again, MediaDB sends a ready event. So we still
> need code to undo the unavailble event and re-scan the sdcard."
> 
> From my reading of the code, we remember that the db has already been
> enumerated and subsequent calls to enumerateDB return without enumerating.
> With that being said, my understanding is we need to handle the 'ready'
> event to remove the 'unplug the phone' overlay, which is accomplished by
> these two lines:
> 
>          storageState = false;
>          updateDialog();
> 
> I don't believe we need to invoke 'enumerateDB' when we get the 'ready' --
> assuming we have already enumerated the db in response to the 'enumerable'
> event.
> 
> What do you think?

I think that is exactly right. I forget whether the Video app calls mediadb.scan() manually or whether it allows mediadb to handle the scanning automatically.  If we're doing it manually, then you have to be sure that we rescan on the ready event.
Flags: needinfo?(dflanagan)
(In reply to David Flanagan [:djf] from comment #35)
> (In reply to Russ Nicoletti [:russn] from comment #34)
> > David, question regarding "You can't completely get rid of the onready event
> > handler. When the user plugs the phone into a computer via USB to transfer
> > files, the sdcard gets unmounted, and MediaDB sends an unavailable event.
> > When the sdcard is mounted again, MediaDB sends a ready event. So we still
> > need code to undo the unavailble event and re-scan the sdcard."
> > 
> > From my reading of the code, we remember that the db has already been
> > enumerated and subsequent calls to enumerateDB return without enumerating.
> > With that being said, my understanding is we need to handle the 'ready'
> > event to remove the 'unplug the phone' overlay, which is accomplished by
> > these two lines:
> > 
> >          storageState = false;
> >          updateDialog();
> > 
> > I don't believe we need to invoke 'enumerateDB' when we get the 'ready' --
> > assuming we have already enumerated the db in response to the 'enumerable'
> > event.
> > 
> > What do you think?
> 
> I think that is exactly right. I forget whether the Video app calls
> mediadb.scan() manually or whether it allows mediadb to handle the scanning
> automatically.  If we're doing it manually, then you have to be sure that we
> rescan on the ready event.

The video app is relying on the mediadb to automatically scan (or rescan) when the db state becomes 'ready'. For the record, this happens when:
* The db becomes during initialization.
* A new SD card gets mounted.
* An existing SD card gets remounted (including when USB storage is enabled and the USB cable is unplugged).
Comment on attachment 8679164 [details] [review]
[gaia] russnicoletti:bug-1210860 > mozilla-b2g:master

Punam, this patch contains three changes:

1) Delay gaia-header 'font-fit' logic to after visuallyLoaded
2) Init db without waiting for mozL10n.once
3) Enumerate db when it becomes enumerable as opposed to when it is fully ready.

I'll let you read the comments in the bug as they may be helpful when you do the review.

Zibi, I didn't keep the 'navigationInteractive' performance mark inside mozL10n.once block because based on my understanding mozL10n.once is no longer needed.
Attachment #8679164 - Flags: review?(pdahiya)
Attachment #8679164 - Flags: review?(gandalf)
Hi Russ, 

Video unit test is failing on tree herder. Can you please check. Thanks!
Flags: needinfo?(rnicoletti)
Comment on attachment 8679164 [details] [review]
[gaia] russnicoletti:bug-1210860 > mozilla-b2g:master

I think I'd do this differently.

Currently, `init()` is doing three things - initializing DB (backend), adjusting UI (front-end) and binding UI elements to actions (frontend).

The proper order should be:

1) initialize initDB() immediately
2) wait for mozL10n.once and adjust UI, then fire navigationLoaded
3) bind events and fire navigationInteractive

The reason for 2) being in mozL10n.once wrapper is this:

If user is in non-default locale, we will load the language resources and then retranslate UI and change the documentElement.dir and fire mozL10n.once.

If you don't wait for it, you are adjusting the playHead direction to `dir` of the default locale, not the locale used by the user.
You're also firing 'navigationLoaded' before the UI is in the right locale.

(1) definitely doesn't need to wait for either. The question is if (3) should wait for (2). Imho it's safer to make it so.

Not directly related to L10n/Perf, but yYou can also do one more thing, which is to use CSS:

html[dir=ltr] {
  left: attr(data-value);
}
html[dir=rtl] {
  right: attr(data-value);
}

and then you can just set `data-value` in HTML to 0, and then in JS you just set `data-value` of on the element, instead of setting left|right on style.
Attachment #8679164 - Flags: review?(gandalf) → review-
(In reply to Russ Nicoletti [:russn] from comment #38)
> Comment on attachment 8679164 [details] [review]
> [gaia] russnicoletti:bug-1210860 > mozilla-b2g:master
> 
> Punam, this patch contains three changes:
> 
> 1) Delay gaia-header 'font-fit' logic to after visuallyLoaded

The patch misses the code that triggers 'font-fit' logic by removing attribute 'no-font-fit' (See github). I see it's there in feedback patch (attachment 8677053 [details] [diff] [review]).

It's good to see videodb listening to enumerable event, it will help if you can post the raptor results with the patch showing performance improvement numbers. Thanks!


> 2) Init db without waiting for mozL10n.once
> 3) Enumerate db when it becomes enumerable as opposed to when it is fully
> ready.
> 
> I'll let you read the comments in the bug as they may be helpful when you do
> the review.
> 
> Zibi, I didn't keep the 'navigationInteractive' performance mark inside
> mozL10n.once block because based on my understanding mozL10n.once is no
> longer needed.
Attachment #8679164 - Flags: review?(pdahiya) → review-
> Zibi, I didn't keep the 'navigationInteractive' performance mark inside mozL10n.once block because based on my understanding mozL10n.once is no longer needed.

mozL10n.once is not blocking, but we need to send performance marks at the right time. mozL10n.once tells you "your DOM is localized in the right language". You should not be sending performance mark saying "UI is ready" until it happens.

So start loading your app before, but use mozL10n.once to send performance mark at the right moment.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #42)
> > Zibi, I didn't keep the 'navigationInteractive' performance mark inside mozL10n.once block because based on my understanding mozL10n.once is no longer needed.
> 
> mozL10n.once is not blocking, but we need to send performance marks at the
> right time. mozL10n.once tells you "your DOM is localized in the right
> language". You should not be sending performance mark saying "UI is ready"
> until it happens.
> 
> So start loading your app before, but use mozL10n.once to send performance
> mark at the right moment.

Just to clarify: performance mark inside mozl10n.once should be 'navigationloaded' (step 2) #comment40 not 'navigationinteractive'. Is that correct? 

Also, Video app first screen loaded chrome has two icons in the footer. Considering footer icons are not localized do we really need to listen to moz10n.once on navigation loaded mark? Thanks!
Flags: needinfo?(gandalf)
good point! in that case you can skip mozl10n.once for perf marks :)

the only thing that i'd like to ensure is that the code that checks html.dir is executed after mozl10n.once.
Flags: needinfo?(gandalf)
(or is declarative in CSS like in my proposal from Comment 40, so that when l10n changes dir, CSS adjusts automatically)
Comment on attachment 8679164 [details] [review]
[gaia] russnicoletti:bug-1210860 > mozilla-b2g:master

Ok, I have updated the patch.

Punam, thanks for catching that it was missing the logic to remove the 'no-font-fit' attribute. Regarding the failing tests, there was an issue with master (fixed this past weekend according to kgrandon) that caused treeherder integration tests to fail. The resolution was to rebase, which did in fact solve the problem.

Zibi, I did not make the change to handle text direction in the css. I want to limit this patch to performance changes, especially given how close it is to 2.5 RA. But I have ensured the code that accesses html.dir is executed after mozl10n.once. I also added some functions to help organize the app initialization process.
Flags: needinfo?(rnicoletti)
Attachment #8679164 - Flags: review?(pdahiya)
Attachment #8679164 - Flags: review?(gandalf)
Attachment #8679164 - Flags: review-
Comment on attachment 8679164 [details] [review]
[gaia] russnicoletti:bug-1210860 > mozilla-b2g:master

lgtm!
Attachment #8679164 - Flags: review?(gandalf) → review+
As far as including raptor results, I'm not able to do that at the moment. I'm not able to install raptor, some npm/node issue. In any event, I've been using the "app startup time" metric we added as part of the 2.5 telemetry feature. It calculates startup time using performance marks exactly like raptor does. I generated visuallyLoaded numbers with and without my patch. The averages were:

with:    2083ms 
without: 2293ms

This is not as statistically relevant as raptor results because I am manually opening and closing the app and am not controlling for other variables as raptor does but I think the difference is sufficiently large to indicate the patch makes a significant performance improvement. 

With that said, regardless of the actual improvement as reported by raptor, I believe this is the best that can be done in the 2.5 timeframe.
> I'm not able to install raptor, some npm/node issue.

You need node 0.12 for raptor and raptor-compare. I use `nvm` package manager for switching between node 4.1 and 0.12
Comment on attachment 8679164 [details] [review]
[gaia] russnicoletti:bug-1210860 > mozilla-b2g:master

Hi Russ

Few nits noted in github, patch looks good and has my r+. I ran raptor with the patch and glad to see ~ 160 ms improvement in cold launch time. Yay! Thanks for the patch!
Attachment #8679164 - Flags: review?(pdahiya) → review+
I was able to install raptor to test my patch. Here are my results. TL;DR -- visuallyLoaded is 200ms improved.

With the patch:
| Metric                | Mean   | Median | Min    | Max    | StdDev | 95% Bound |
| --------------------- | ------ | ------ | ------ | ------ | ------ | --------- |
| navigationLoaded      | 1164   | 1164   | 1164   | 1164   | 0      | 1164      |
| navigationInteractive | 1175   | 1175   | 1175   | 1175   | 0      | 1175      |
| visuallyLoaded        | 1837   | 1837   | 1837   | 1837   | 0      | 1837      |
| contentInteractive    | 1839   | 1839   | 1839   | 1839   | 0      | 1839      |
| fullyLoaded           | 3395   | 3395   | 3395   | 3395   | 0      | 3395      |

Without:
| Metric                | Mean   | Median | Min    | Max    | StdDev | 95% Bound |
| --------------------- | ------ | ------ | ------ | ------ | ------ | --------- |
| navigationLoaded      | 1579   | 1579   | 1579   | 1579   | 0      | 1579      |
| navigationInteractive | 1603   | 1603   | 1603   | 1603   | 0      | 1603      |
| visuallyLoaded        | 2037   | 2037   | 2037   | 2037   | 0      | 2037      |
| contentInteractive    | 2039   | 2039   | 2039   | 2039   | 0      | 2039      |
| fullyLoaded           | 3432   | 3432   | 3432   | 3432   | 0      | 3432      |
Master: https://github.com/mozilla-b2g/gaia/commit/6c15484434d022006f70cd3ba3a2361b2e1cb5a1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
cool! btw. the stdev is weird. How many runs did you do? I'd recommend at least 30 for the current stdev on Flame.
My latest test with 30 runs produced a difference of 109ms. 

With patch
==========

| Metric                | Mean     | Median   | Min    | Max    | StdDev | 95% Bound |
| --------------------- | -------- | -------- | ------ | ------ | ------ | --------- |
| navigationLoaded      | 1560.900 | 1571.500 | 1171   | 1655   | 78.941 | 1589.149  |
| navigationInteractive | 1580.300 | 1594     | 1187   | 1680   | 79.609 | 1608.788  |
| visuallyLoaded        | 2029.867 | 2039.500 | 1817   | 2133   | 53.048 | 2048.850  |
| contentInteractive    | 2035.400 | 2046     | 1820   | 2138   | 53.494 | 2054.542  |
| fullyLoaded           | 3505.067 | 3515.500 | 3383   | 3643   | 62.527 | 3527.442  |

Without patch
=============

| Metric                | Mean     | Median   | Min    | Max    | StdDev | 95% Bound |
| --------------------- | -------- | -------- | ------ | ------ | ------ | --------- |
| navigationLoaded      | 1676.033 | 1671.500 | 1570   | 1757   | 38.941 | 1689.968  |
| navigationInteractive | 1696.367 | 1691     | 1590   | 1775   | 38.709 | 1710.218  |
| visuallyLoaded        | 2142.067 | 2153.500 | 1999   | 2220   | 43.864 | 2157.763  |
| contentInteractive    | 2148.200 | 2158.500 | 2001   | 2225   | 44.644 | 2164.176  |
| fullyLoaded           | 3522.100 | 3536     | 3333   | 3686   | 76.835 | 3549.595  |
This patch looks great on dashboard :) Really nice wins all around (mem and perf!)
You need to log in before you can comment on or make changes to this bug.