Closed Bug 1478918 Opened 2 years ago Closed 2 years ago

Inflate outgoing records and record observer info outside the bookmarks mirror's merge transaction

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: lina, Assigned: lina)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In bug 1472963, comment 8, we saw that `jankYielder` can yield for over a minute. When done in a transaction, this keeps that transaction open, and prevents the main connection from writing. This happened in the merger, but we have several `jankYielder` calls (via `yieldingIterator`) in `noteObserverChanges` and `fetchLocalChangeRecords`, too.

Fortunately, I don't think those need to be in the merge transaction. `itemsToUpload` stores a snapshot of the local items, and we don't need to keep a transaction open to read from that table. `noteObserverChanges` does this to ensure we pass the correct arguments to the observers, in case the main connection changes the database between merging and notifying...but that's already an issue today.
Neither of these should block the main Places connection from writing
to the database. `itemsToUpload` stores a snapshot of the local items,
so we don't need to keep a transaction open when we read from this
table to inflate records.

`noteObserverChanges` did this to ensure we passed the correct params
to the observers, in case the database changed between merging and
notifying, but this is already racy.

This patch also adds a forgotten `AFTER DELETE` trigger to update
the global Sync change counter for successfully uploaded tombstones.
Comment on attachment 8995425 [details]
Inflate outgoing records and notify observers outside the bookmarks mirror's merge transaction.

Marco Bonardo [::mak] has approved the revision.

https://phabricator.services.mozilla.com/D2454
Attachment #8995425 - Flags: review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/616e64b69485
Inflate outgoing records and notify observers outside the bookmarks mirror's merge transaction. r=mak
https://hg.mozilla.org/mozilla-central/rev/616e64b69485
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(In reply to Lina Cambridge (she/her) [:lina] from comment #0)
> In bug 1472963, comment 8, we saw that `jankYielder` can yield for over a
> minute.

The jankyielder was created largely by an act of faith that it really shouldn't cause harm (other than a minor wall-clock cost) and should help avoid jank. In this bug (and probably also in bug 1449570), we see both of those things being untrue. Excess use of the jank yielder also appears to have a significant GC cost.

Do you think we should open a couple of extra bugs - one to remove the jank yielder entirely from inside transactions, and the other to investigate using it only where we can demonstrate a benefit?
You need to log in before you can comment on or make changes to this bug.