1559349 - Re-evaluate sync startup timing

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P3)

Description

[:Gijs (he/him)]

Sync tries pretty hard not to interfere with startup. Unfortunately, most of that code seems to be from well before we had good primitives for saying "do this work when we're not doing other work".

In particular, this code: https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/browser/components/BrowserGlue.jsm#711 looks like it will wait far too long to start talking to sync on profiles with lots of tabs, even if not all those tabs load on startup. Also also, the code there to abort if there's no gBrowser looks... very wrong (ie it might abort even when we're not shutting down, and then we never autoconnect to sync).

This bug is about ripping out all the old stuff and just make initializing sync happen on an idle task when the browser starts. We should be able to trust our idle task scheduling to make that work without perf regressions, and if there are issues (ie if sync does a bunch of CPU-intensive work that still causes jank in that case) then we can probably use more idle tasks to chunk that up a bit.

Mark, can you maybe provide a bit of an "anatomy of sync startup"? The perf team will be happy to either give concrete recommendations of what we could do instead, and potentially provide patches. While this won't improve startup time, it will improve perceived startup time / functionality of sync, so I still think this is an fxperf issue. Put differently, I've definitely seen broken sync UI (send tab to device unavailable) that I strongly suspect is a result of this issue.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs (he/him) from comment #0)

Mark, can you maybe provide a bit of an "anatomy of sync startup"?

Thanks Gijs,
It would be great to get some of this cleaned up. I guess its fair to say there are 2 main categories here:

• Initializing all the sync engines and the scheduler: this is primarily loading quite a few modules from under the services/sync tree. Most of these add various observers, and some will asynchronously load files holding json from disk (or at least try and load them - the common case is hopefully that they don't exist)

• Then we have the sync itself, which involves loading stuff from Firefox accounts (which itself also uses async json files on disk, plus loading stuff from the login manager), and in a best case, just a couple of network requests to realize there's nothing to do. In a worst case there will be many incoming records which need to be applied, the cost of which varies depending on the data (eg, bookmarks are relatively expensive but tend to be low in volume, whereas history tends to be cheap per incoming item, but often has a high volume)

While all that startup code you reference predates my involvement by some years, I believe the problem being avoid was primarily the cost of the module loading and initialization, and since then we've done a fair bit in ensuring we use lazy imports/getters and lazy prefs whereever we can, so we might not find the costs are as significant as we believe these days.

Reducing the initialization will also fix a number of issues around "tracking" - many of the observers I mentioned are so Sync sees changes to, eg, logins, so it knows there's a change to put on the server. Thus, things like saving a new login before Sync has initialized means that login probably isn't going to make it up to the server :( Bookmarks is about the only engine immune to that.

A great first step would be to measure this.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:markh, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[:Gijs (he/him)]

So, if I'm reading this code right, in the worst case (ie both of my desktop machines 🙃) we'll wait 5 minutes (I have >=100 tabs) before starting to sync. Is that right? If so, we probably want to adjust that for the MR1 project... :-)

Edit: updated link from comment 0: https://searchfox.org/mozilla-central/rev/e66593593f3b356901011ea0fcdf9979728e9ae8/browser/components/BrowserGlue.jsm#996-1023

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Yeah, that does sound correct - but I'm not sure adjusting that is the best approach - for one thing, prefs and passwords sync first (so tabs still aren't the first thing we grab) and because we sync things like history there's a risk that we make an already overloaded browser trying to restore a session to go even slower.

We already sync tabs when synced-tabs UI is shown, so maybe extending that approach is better - ie, force a tabs-only sync when about:home is being shown? That will still mean people restoring a large session (so about:home probably isn't even loaded) remains the same, and the thing we care about (tabs) come in even faster than they would by reducing that delay. It also means tabs get a quick sync every time about:home loads and not just at startup.

IOW, I'm suggesting explicit calls to something like https://searchfox.org/mozilla-central/rev/69a482382823f42f482e840f65725218d7654cc4/services/sync/modules/SyncedTabs.jsm#137 might be better than reducing that delay. WDYT?

(we might actually need a followup to ensure these quick-syncs don't impact regularly scheduled syncs - I suspect that syncing just tabs will reset the general sync timer, which isn't ideal)

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(to be clear though, I'm inclined to agree that 5 minutes is too conservative, so tweaking this seems reasonable - but this is possibly orthogonal to how we can quickly get remote tabs loaded for MR1)

Comment 6

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #4)

We already sync tabs when synced-tabs UI is shown, so maybe extending that approach is better - ie, force a tabs-only sync when about:home is being shown? That will still mean people restoring a large session (so about:home probably isn't even loaded) remains the same, and the thing we care about (tabs) come in even faster than they would by reducing that delay. It also means tabs get a quick sync every time about:home loads and not just at startup.

IOW, I'm suggesting explicit calls to something like https://searchfox.org/mozilla-central/rev/69a482382823f42f482e840f65725218d7654cc4/services/sync/modules/SyncedTabs.jsm#137 might be better than reducing that delay. WDYT?

This makes sense to me. To be clear, I think given current designs (still shifting at the moment, hopefully more final by the end of this week), to enable notifications for "hey you have new mobile tabs you can use", I think we'd want to sync tabs (only?) shortly after startup.

(In reply to Mark Hammond [:markh] [:mhammond] from comment #5)

(to be clear though, I'm inclined to agree that 5 minutes is too conservative, so tweaking this seems reasonable - but this is possibly orthogonal to how we can quickly get remote tabs loaded for MR1)

OK. In that case, I guess we need a separate bug for more aggressively syncing tabs shortly after startup, and we'd use this one for removing the 5 minute thing? In any case I expect that if/when we roll out the work from bug 1752664 we'll end up syncing way earlier than that timeout, which somewhat defeats its purpose, right? Or does the listening for those events not kick in until the timeout has expired?

(we might actually need a followup to ensure these quick-syncs don't impact regularly scheduled syncs - I suspect that syncing just tabs will reset the general sync timer, which isn't ideal)

Hm, yeah, that doesn't sound great.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs (he/him) from comment #6)

This makes sense to me. To be clear, I think given current designs (still shifting at the moment, hopefully more final by the end of this week), to enable notifications for "hey you have new mobile tabs you can use"

Oh dear - I hope this doesn't mean notification spam about tabs :(

I think we'd want to sync tabs (only?) shortly after startup.

I'd still prefer to only sync tabs when they are offering value to the user. So assuming about:firefox-home isn't open, and assuming most users would disable notifications around tabs (I know for sure I would!), I'd prefer to not bother syncing tabs.

This effort increasing the perception of Firefox being sluggish is one of the most obvious things to avoid IMO (next to spamming notifications, but I digress ;)

(In reply to Mark Hammond [:markh] [:mhammond] from comment #5)

(to be clear though, I'm inclined to agree that 5 minutes is too conservative, so tweaking this seems reasonable - but this is possibly orthogonal to how we can quickly get remote tabs loaded for MR1)

OK. In that case, I guess we need a separate bug for more aggressively syncing tabs shortly after startup

As above, I think unconditionally syncing tabs very soon after startup would be a mistake. The first sync of a session is likely to hit a couple of FxA servers before any of the storage servers, so the second sync is significantly more light-weight.

and we'd use this one for removing the 5 minute thing?

If startup isn't a concern, let's just do a full sync very soon after startup, arrange for tabs to be the first thing synced, then remove all this code completely? Conversely, if startup is a concern, IMO that's incompatible with an unconditional first sync soon after startup.

Hm, yeah, that doesn't sound great.

I opened bug 1755611.

Comment 8

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #7)

(In reply to :Gijs (he/him) from comment #6)

This makes sense to me. To be clear, I think given current designs (still shifting at the moment, hopefully more final by the end of this week), to enable notifications for "hey you have new mobile tabs you can use"

Oh dear - I hope this doesn't mean notification spam about tabs :(

Sorry, I think I was misleading here - what is in the design is an "attention" dot on some piece of UI, a bit like the one on the hamburger menu for updates, indicating there are available tabs from mobile. No desktop notification spam will be involved.

If startup isn't a concern, let's just do a full sync very soon after startup, arrange for tabs to be the first thing synced, then remove all this code completely? Conversely, if startup is a concern, IMO that's incompatible with an unconditional first sync soon after startup.

I don't think that startup isn't a concern at all, but I think waiting 5 minutes is way too long, and not much better for startup times than waiting for CPU idle after we finish creating all the tabs. IOW, I think doing the initial sync from startup idle tasks would be a good compromise in not interfering with startup performance, and still doing the sync relatively soon: https://searchfox.org/mozilla-central/rev/3de1b6791ea569cdf7773f7cd512cf2e6cc1d3f0/browser/components/BrowserGlue.jsm#2376-2397 . Mike, can you give a second opinion on the startup impact and/or whether there's an even better place to do something like this?

(In a way, perhaps if we could also check for "network idle" or something like that, it'd behave even better, but I don't think we have abstractions for that at the moment.)

Comment 9

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

I think _scheduleStartupIdleTasks is a reasonable place to put this - it'll wait until we have time to service non-urgent events, which should be after the critical work for starting the browser and making it responsive.

There is also the option of _scheduleBestEffortUserIdleTasks, which has the added bonus of waiting for the user to become idle (not send any user input events to the process for 20s of time). The drawback there is that if the user never becomes idle, then sync would never start, which is probably not what we want.

So the first option, the one you identified, is probably best - sync is exactly the sort of thing that would fit in with that functions' purpose.

Comment 10

[:Gijs (he/him)]

Sammy tells me he's working on this. :-)

Comment 11

[Sammy Khamis [:skhamis]]

Attachment: Bug 1559349 - Change Sync startup to be after browser idle r?markh,gijs

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcef8271055b
Change Sync startup to be after browser idle r=markh,Gijs

Comment 13

[Atila Butkovits]

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