1439892 - Activity stream initialization causes jank after receiving the sessionstore-windows-restored notification

Status: NEW

(Firefox :: New Tab Page, defect, P3)

Description

[Florian Quèze [:florian]]

https://perfht.ml/2BDM6hD shows onBrowserReady from jar:file:///.../browser/features/[e-mail]!/bootstrap.js taking almost 3s.

This is from a cold startup profile on a slow machine, with a nightly from 2018-02-13

Comment 1

[:Gijs (he/him)]

The impact here seems pretty significant, and although https://bugzilla.mozilla.org/show_bug.cgi?id=1376984 delayed running this initialization, it's not delayed onto an idle task. I also wonder to what degree we avoid doing too much IO for all these modules, and how much we might gain by just having a build step instead of this webpack/redux/require stuff, and having some 36 different jsms... Anyway, fxperf:p2 given the impact on startup.

Comment 2

[Andrei Oprea [:andreio]]

The stack trace just points to Activity Stream addon loading which isn't something we can easily optimize but bug 1447130 should help with this.

Comment 3

[:Gijs (he/him)]

(In reply to Andrei Oprea [:andreio] from comment #2)
> The stack trace just points to Activity Stream addon loading which isn't
> something we can easily optimize but bug 1447130 should help with this.

While I do expect bug 1447130 to help here, I'm a bit confused. I'm not as familiar with the AS code as you are, but surely there are bits that could be loaded later, or if not then perhaps we could improve things by unifying some files? 3s is clearly excessive.

As a completely random example from just looking at this profile for 5 minutes, it seems we load a bunch of redux stuff via webpack/require() that we could presumably just load in the more usual way, and PrefsFeed.jsm eagerly creates a PrerenderData instance that could be created lazily instead.

It also looks like ActivityStream.jsm has a comment about lazy-loading that might be outdated now that we can use ChromeUtils to implement the lazy-loading natively.

Then there's all the stuff inside init(), where all the recursion in Store.get() seems excessive, and some of the names of the callees ("updateSectionContextMenuOptions") don't seem like they should be run as part of startup at all.

Comment 4

[Ed Lee :Mardak]

I don't expect bug 1447130 to change when we decide to actually finish initializing activity stream, i.e., just after sessionstore-windows-restored. Not being a bootstrap add-on will avoid some add-on-related initialization, which happens very early during startup and the reason why activity stream delays until later in startup.

The critical path here is getting content to show up in an initial new window about:home or restored activity stream page -- where the former is used for hero element / first paint measurements. In the past, we had optimized for just getting the search box to appear ASAP with the rest of the page relatively unrendered, but that specific optimization goal was deprioritized as the "hero element" should also favor top sites as users interact with those at high rates too.

Gijs' specific example of context menu stuff could more generalized in looking for things that are not immediately needed to show content for users, e.g., those that require some user action. For the context menu, the user would not notice that we sent a later message to populate the menu -- potentially deferring until the user tries to open the context menu.

(In reply to :Gijs (he/him) from comment #3)
> It also looks like ActivityStream.jsm has a comment about lazy-loading that
> might be outdated now that we can use ChromeUtils to implement the
> lazy-loading natively.
This could be worth measuring, but I have some initial doubts as as the comment says:
// NB: Eagerly load modules that will be loaded/constructed/initialized in the
// common case to avoid the overhead of wrapping and detecting lazy loading.

From bootstrap.js, we lazily load ActivityStream.jsm and immediately call its init(), which will cause all the related jsm files to be loaded synchronously, so lazily loading them won't actually be lazy and introduce some (minor? native) overhead. But if the overhead is pretty minimal now anyway, it could still be worthwhile to consistently "always be lazy" to avoid accidentally eagerly loading things in other places.

Comment 5

[Ed Lee :Mardak]

Oh, I CCed k88hudson and forgot to comment :P

We have previously looked at packaging our jsms into a single file given how we export and do our unit testing anyway. That could avoid the overhead of ~35 jsm compartments.

For actual logic / behavior changes to startup, that might require some rearchitecting to better allow sections / feeds to be more performance-aware in their startup behaviors.

Comment 6

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #4)
> (In reply to :Gijs (he/him) from comment #3)
> > It also looks like ActivityStream.jsm has a comment about lazy-loading that
> > might be outdated now that we can use ChromeUtils to implement the
> > lazy-loading natively.
> This could be worth measuring, but I have some initial doubts as as the
> comment says:
> // NB: Eagerly load modules that will be loaded/constructed/initialized in
> the
> // common case to avoid the overhead of wrapping and detecting lazy loading.
> 
> From bootstrap.js, we lazily load ActivityStream.jsm and immediately call
> its init(), which will cause all the related jsm files to be loaded
> synchronously, so lazily loading them won't actually be lazy and introduce
> some (minor? native) overhead. But if the overhead is pretty minimal now
> anyway, it could still be worthwhile to consistently "always be lazy" to
> avoid accidentally eagerly loading things in other places.

Part of the reason to move lazy module loading into ChromeUtils was performance - the lazy getters that were JS-implemented were not very performant. But if we need most of the things inside the jsms immediately anyway, it's true this may not help much.

(In reply to Ed Lee :Mardak from comment #5)
> Oh, I CCed k88hudson and forgot to comment :P
> 
> We have previously looked at packaging our jsms into a single file given how
> we export and do our unit testing anyway. That could avoid the overhead of
> ~35 jsm compartments.

I think we now use a single-compartment in chrome-land, though I don't know for sure if that's true for (system) add-ons. :mccr8 might know. Either way, it'd probably improve the IO performance to have 1 file instead of 35. I also wonder how much this will improve if we move code into core Firefox so that we preload stuff as part of omni.ja (which AIUI gets mmapped?), instead of in a separate file, which I expect will be slower.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(Related to what we discussed today, Jay)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Is it alright if I unassign you from this, andreio? I think I'm hoping to transform this into a metabug for the work that Bernard and I are going to do to move Redux state computation into the privileged about content process, and make Activity Stream initialization no longer key off of sessionstore-windows-restored notification.