1329712 - AutoMigrate still imported in nsBrowserGlue.js - probably no longer necessary?

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

https://dxr.mozilla.org/mozilla-central/rev/d192a99be4b436f2dc839435319f7630d5d8f4b0/browser/components/nsBrowserGlue.js#652-653

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1329712 - stop importing AutoMigrate.jsm early on startup now that we no longer observe places changes,

Review commit: https://reviewboard.mozilla.org/r/133610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133610/

Comment 2

[:Gijs (he/him)]

I should really just have removed this in bug 1285577, but it got missed, and then I forgot I'd filed this. But AIUI we can now just request this module when we need it (usually only in startup migration, or when we're showing the relevant "did you want to undo this thing" notifications from about:home or about:newtab).

Given that about:home and about:newtab still load this, it's possible there's more of a perf gain to be had by making that code conditional on automigration being enabled and/or lazy-ifying e.g. the MigrationUtils import (which isn't necessary to just check if we're going to show the notification in the common case that we won't show it).

Florian, is that worth pursuing in a separate bug? Don't know if you've already noticed this in startup profiling or no.

Comment 3

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #2)
> I should really just have removed this in bug 1285577, but it got missed,
> and then I forgot I'd filed this. But AIUI we can now just request this
> module when we need it (usually only in startup migration, or when we're
> showing the relevant "did you want to undo this thing" notifications from
> about:home or about:newtab).
> 
> Given that about:home and about:newtab still load this, it's possible
> there's more of a perf gain to be had by making that code conditional on
> automigration being enabled and/or lazy-ifying e.g. the MigrationUtils
> import (which isn't necessary to just check if we're going to show the
> notification in the common case that we won't show it).
> 
> Florian, is that worth pursuing in a separate bug? Don't know if you've
> already noticed this in startup profiling or no.

Here is what I have in my existing startup profiles:

Mac: https://perfht.ml/2pltLPZ Loading AutoMigrate.jsm takes 3ms, and from the profile I would be tempted to say removing Preferences.jsm from that module (or at least from the _checkIfEnabled method) may be a good idea; but 1ms is just one sample, so can't say for sure.

Windows: https://perfht.ml/2plmtfc Loading AutoMigrate.jsm is visible but just 1ms, so not really significant.

I should have access to a reference device soon, so I'll have more detailed profiles.

If it's easy, lazy-ifying MigrationUtils would still be a good idea: it doesn't seem to be expensive in time (all the modules it imports are lazy-loaded), but would still save some memory.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8861624 [details]
Bug 1329712 - stop importing AutoMigrate.jsm early on startup now that we no longer observe places changes,

https://reviewboard.mozilla.org/r/133610/#review136898

Makes sense, thanks.

Comment 5

[:Gijs (he/him)]

Pulsebot seems to be offline or something, but:

https://hg.mozilla.org/integration/autoland/rev/d5c71159c510

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d5c71159c510