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
46 bytes, text/x-phabricator-request
|Details | Review|
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 email@example.com: https://hg.mozilla.org/integration/autoland/rev/616e64b69485 Inflate outgoing records and notify observers outside the bookmarks mirror's merge transaction. r=mak
(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.