1007448 - [meta] Refactor sync to use promises/tasks to avoid all event loop spinning

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P3)

Description

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

The nested event loop spinning in Sync is evil and must be destroyed.

Comment 1

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

Attachment: 0001-Part-1-Get-engines-working-with-promises-tasks.patch

All attachments are a WIP, and all have f? without nominating anyway - all feedback welcome - particularly from Richard who is likely to draw the short review straw.

This is a huge patch, but the changes are largely mechanical.  I can't see a reasonable way to split it up, other than splitting out the tests - but as the tests are all grouped together that doesn't seem to offer much value.

With this patch we still end up with a number of places the event loop spins, most notably during observer notifications and in the resource authenticators.

Comment 2

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

Attachment: 0002-Part-2-resource-authenticators-promise-based.patch

This patch makes the resource authenticators promise based.

Comment 3

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

Attachment: 0003-Part-3-Add-concept-of-pending-promises-so-observers-.patch

This patch removes most of the rest of spinning code by adding the concept of "pending promises".  Things like observer notifications add a "pending promise" then return immediately.  At certain times during the sync process, these pending promises are all resolved before continuing (eg, before syncing we resolve all promises, then after syncing we resolve any new ones added by the sync.

This patch is somewhat scary as it means the order of promise resolution isn't deterministic - in effect they are all running concurrently.  Tests all pass though :)

Comment 4

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

Attachment: 0004-Part-4-remove-all-spinning-code-from-tests.patch

A test-only change that removes all spinning code from tests.

Comment 5

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

These patches also stops using runInBatchMode for places which will hurt performance.  Bug 890203 is looking into adding a batch mode that is promise-friendly, so blocking on that (and note that the code has a comment referencing that bug)

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8419116 [details] [diff] [review]
0001-Part-1-Get-engines-working-with-promises-tasks.patch

Gonna mark one of these for me, just so it shows up in my queue. Won't get to it until later this week, mind.

Comment 7

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

It's going to take some time to review and land all the parts of this puzzle, so I'm going to treat this as a meta-bug and land each patch in its own bug.

Comment 8

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

FTR, a quick update:

* I haven't changed the patch so it deals with 3rd-party engines. That change shouldn't be difficult, but will require some minor restructuring (and *will* still spin event-loops if any such engines are installed).

* I'm not comfortable landing this until we get decent stack-traces from Promises. I *think* that will happen when we move away from Promise.jsm, but we only use that indirectly via Task.jsm, so *that* needs to be changed to work with DOM promises - which in theory sounds easy, but my quick-and-obvious attempt at that failed for non-obvious reasons.

Comment 9

[Chris Karlof [:ckarlof]]

Clearing the priority from all our meta bugs.

Comment 11

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

\o/