1197098 - Sync is janky when processing incoming history entries due to work done for about:newtab

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

Sync causes significant UI jank when processing incoming history records, which I've tracked down to onFrecencyChanged() in NewTabUtils.jsm being called once per new record while Sync is adding up to 5000 records.

Sync calls Cc["@mozilla.org/browser/history;1"].getService(Ci.mozIAsyncHistory).updatePlaces() in batches of 50 items. While nsINavHistoryObserver has onBeginUpdateBatch and onEndUpdateBatch, this history implementation doesn't make use of it - mainly because almost noone seems to call this interface with multiple records at once - sync and profile migrators seem to be the exceptions.

I'm a little reluctant to change History.cpp to make this call if multiple items are being added. History.jsm doesn't implement its |update| method, and if it did implement it as described it wouldn't suit Sync's requirements (there's no callback for errors). So I took a "low road" approach of having Sync send these notification before and after it does its thing, and having NewTabUtils do the right thing on that notification.

These methods were added to NewTabUtils in bug 911307, which was implemented by Drew (hence r?) and reviewed by Tim (hence f?), and cc Mak as he knows everything about places :)

I've struggled to measure jank accurately, so I've just been observing it by having the Sync button in the toolbar and watching how smooth it is. The button spins very jankily without this patch and quite smoothly with it, and about:newtab looks correct after Sync completes. What do you think?

Comment 2

[Marco Bonardo [:mak]]

I suspect in future we might want to remove manual batch notifications (there's no plan yet, but it was discussed) cause they are dangerous; suppose you issue a beginbatch and then something throws in the middle and you never issue an endbatch, the UI would be broken until the next restart.
It would be much better if we could manage things centrally with a dispatcher, it could evaluate the notifications traffic and act properly (maybe inserting batch notifications automatically when needed). But that first requires all APIs to be async.

Regardless, looks like your fix would break frecency sorting since you are skipping all the work. you need to schedule at least an onManyFrecenciesChanged.

Maybe onFrecencyChanged could be smarter and use a timer to manage changes, I don't see why it needs to do the work synchronously. For example it could collect changes and fire a timer (cancel it if a new notification is received before it fired), when the timer elapses if the number of changes is big it might call onManyFrecenciesChanged once.

Or we could modify updatePlaces to notify a batch when the number of entries is bigger than a threshold (not for any multiple addition). then the consumer could schedule a batch change.

We could even modify nsNavHistory::DispatchFrecencyChangedNotification to do the timeout thing, if it detects more than N notifications in M milliseconds, it could shut them up and fire an onManyFrecenciesChanged. This would break the assumption that notifications are fired before an async method call backs though, so I don't know how it might break things in future.

This is all very imperfect, but I don't think we have resources to write a dispatcher and change all consumers now.

Comment 3

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

(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #2)
> Regardless, looks like your fix would break frecency sorting since you are
> skipping all the work. you need to schedule at least an
> onManyFrecenciesChanged.

I'm not sure what you mean here - there is a new call to this.onManyFrecenciesChanged()

> This is all very imperfect, but I don't think we have resources to write a
> dispatcher and change all consumers now.

Agreed - but I'm not sure if your conclusion is "it's imperfect but a reasonable work-around" or something else?

Thanks!

Comment 4

[Marco Bonardo [:mak]]

(In reply to Mark Hammond [:markh] from comment #3)
> I'm not sure what you mean here - there is a new call to
> this.onManyFrecenciesChanged()

Ah sorry, I missed that, but you should invoke that only if an onFrecencyChanged has been seen during a batch.

> > This is all very imperfect, but I don't think we have resources to write a
> > dispatcher and change all consumers now.
> 
> Agreed - but I'm not sure if your conclusion is "it's imperfect but a
> reasonable work-around" or something else?

I think using batches for now it's reasonable, as I said I'd also be fine doing that in updatePlaces, when the number of entries is over a threshold, since I expect this is not the only consumer that would be hit by lots of additions, our UI views might suffer as well.

Plus, the newtab code could maybe schedule work instead of doing it inside the notification.

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8650895 [details] [diff] [review]
0001-Bug-XXXXXXX-have-Sync-notify-history-observers-of-a-.patch

Review of attachment 8650895 [details] [diff] [review]:
-----------------------------------------------------------------

This looks very reasonable to me, thanks Mark.  The only change I would make is to keep a counter of how many batches we've seen, not a bool -- recognize that batches can be nested in other words.

I could go either way on what Marco said about this:

(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #4)
> but you should invoke that only if an
> onFrecencyChanged has been seen during a batch.

I don't think it's worth the complexity, but on the other hand it would only be a tiny amount of complexity.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bb8a63b97083

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb8a63b97083