Closed Bug 1007448 Opened 8 years ago Closed 4 years ago

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

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: markh, Unassigned)

References

Details

(Keywords: meta)

Attachments

(4 obsolete files)

The nested event loop spinning in Sync is evil and must be destroyed.
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.
Attachment #8419116 - Flags: feedback?
This patch makes the resource authenticators promise based.
Attachment #8419117 - Flags: feedback?
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 :)
Attachment #8419120 - Flags: feedback?
A test-only change that removes all spinning code from tests.
Attachment #8419121 - Flags: feedback?
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)
Depends on: 890203
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.
Attachment #8419116 - Flags: feedback? → feedback?(rnewman)
Depends on: 1120695
Attachment #8419116 - Flags: feedback?(rnewman)
Flags: firefox-backlog+
Priority: -- → P2
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.
Summary: Refactor sync to use promises/tasks to avoid all event loop spinning → [meta] Refactor sync to use promises/tasks to avoid all event loop spinning
Attachment #8419116 - Attachment is obsolete: true
Attachment #8419117 - Attachment is obsolete: true
Attachment #8419117 - Flags: feedback?
Attachment #8419120 - Attachment is obsolete: true
Attachment #8419120 - Flags: feedback?
Attachment #8419121 - Attachment is obsolete: true
Attachment #8419121 - Flags: feedback?
Depends on: 1210296
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.
Blocks: 1252058
Clearing the priority from all our meta bugs.
Priority: P2 → --
Flags: firefox-backlog+
Priority: -- → P3
Depends on: 1343158
Depends on: 1355677
Duplicate of this bug: 600059
\o/
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.