1376287 - consider using idle dispatch for firefox accounts sync

Status: RESOLVED WONTFIX

(Firefox :: Sync, enhancement, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

I recently opened a firefox profile that I have not used in a number of weeks or months.  I noticed a large amount of CPU being used by the parent process.  It appears this was due to firefox accounts sync running.

I definitely wanted to sync this profile, so running sync is a good thing.  I do worry, though, that triggering a large sync right after re-opening an old profile could interfere with the user.  In particular, this could be presenting a less than ideal user experience just as we have an opportunity to impress the user after a long periof of inactivity.

Could we run accounts sync work from an idle dispatch?  This would reduce the chance it will cause any latency for active user work.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Is the work sync does incremental? If not, idle dispatch might not help too much. Though, there tends to be often 50ms idle periods in parent process.

If sync is incremental, then this would be a really good candidate for idle dispatch.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Considering sync takes on the order of minutes (sometimes > 5 minutes), I think it must be incremental.

Comment 3

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

I guess it depends on how you define "incremental", but I think what we really want is captured in the dev.platform thread "Avoiding jank in async functions and promises" (https://groups.google.com/forum/#!topic/mozilla.dev.platform/A6aRmSgExb0)

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Realistically I don't think we can/want to do this, as idle dispatch can take a long amount of time between when it calls us back (as far as I know).

Sync is transactional, and another client syncing while the first one has a transaction out will cause both syncs to fail. Additionally, there are known bugs where certain sync engines will miss changes that occur while a sync is in progress. These lead us to strongly desire syncs to finish as quickly as possible. It's incremental in that it does a great deal of async work, but not in that it can be split up without causing problems.

Additionally, between when this bug was filed and now, we've learned that the method we've been using to break up functions to complete work incrementally, jankYielder (https://searchfox.org/mozilla-central/rev/adec563403271e78d1a057259b3e17fe557dfd91/services/common/async.js#105), is the cause of a large amount of garbage in practice, see https://bugzilla.mozilla.org/show_bug.cgi?id=1460951, although that doesn't look like it has our profiles attached.

I think we should probably close this as wontfix, markh, what do you think?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

FWIW, you can specify a timeout on requestIdleCallback() which bounds how long it can take to invoke your callback.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

What benefit would doing that get us versus just increasing the amount of time we wait or iterations between waiting in the jankYielder?

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Err, what benefit, given that we'd still request a fairly low timeout for the reasons specified.

Comment 8

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

(In reply to Thom Chiovoloni [:tcsc] from comment #4)
> Additionally, between when this bug was filed and now, we've learned that
> the method we've been using to break up functions to complete work
> incrementally, jankYielder
> (https://searchfox.org/mozilla-central/rev/
> adec563403271e78d1a057259b3e17fe557dfd91/services/common/async.js#105), is
> the cause of a large amount of garbage in practice

(And by "garbage" there, I believe Thom means "js garbage, causing jank due to GC")

Yeah, I suspect that's one of our biggest problems. We introduced this problem somewhat blindly, under assumptions that weren't grounded by data - and I fear making the same mistake here. That said, bug 1168428 is important, and the thing with the most impact we can do is probably to get good measurements on what exactly *is* janky and base our decisions on real data and measured improvements. If you'd like to open a new bug along those lines and work on that, I think we'd all appreciate it.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Just breaking up things to happen more incrementally doesn't solve the scheduling issue - code may still run at a wrong time causing jank. Usually there is plenty of idle time, and such time is good for running any background-ish work (and timer can be used to guarantee the code is run eventually).

Comment 10

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

Idle dispatch could form a part of how we avoid a janky sync, but I'm not sure how. The jankyielder (see comment 4) tries to be a one-size-fits-all solution that we use in many loops - effectively trying to emulate the approach taken in https://searchfox.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1774, but which turns out to *cause* jank when used too often. The dev-platform thread mentioned in comment 3 discusses this a bit more, and maybe I closed this prematurely - I'm happy to be proved wrong by a demonstration that using idle dispatch is a panacea and doesn't sacrifice sync reliability.