1442383 - Activity stream spends a lot time fetching highlights after bookmark sync.

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

After a bookmark sync, activity stream spends a long time (approximately 9s in the profile attached below) in fetchHighlights.

This may be a duplicate of bug 1440276, but I'm not familiar enough with the code to be able to tell. (It seems different and I was encouraged to submit either way).

Profile showing this: https://perfht.ml/2oFsVgb, which was taken on central during a sync of 10k records with the buffered bookmark engine (services.sync.engine.bookmarks.buffer: true).

Comment 1

[Ed Lee :Mardak]

Probably because it tries to update on each bookmark:
https://github.com/mozilla/activity-stream/blob/defc6cfecb0b5d7090a08e466c224335a50cf38c/system-addon/lib/HighlightsFeed.jsm#L211-L215

I wonder if it would be so bad to just expire but not fetch on these actions and only fetch on SYSTEM_TICK (every 5 minutes) if stale?

Although this would mean a just bookmarked page wouldn't show up in highlights immediately anymore…

Comment 2

[Ed Lee :Mardak]

Alternatively there's some batching of multiple history deletes:
https://github.com/mozilla/activity-stream/blob/defc6cfecb0b5d7090a08e466c224335a50cf38c/system-addon/lib/PlacesFeed.jsm#L42-L55

But unclear if that will be regressed due to the Promise.resolve().then usage.

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

You could probably also wait until it's been N (for relatively small N) seconds since the last notification before fetching them. In the new bookmark engine (which is admittedly preffed off at the moment) these are called in a loop, so the amount of time between them should be fairly low. That's probably true to some degree now as well, but less so.

Comment 4

[Ursula Sarracini (:ursula)]

For what it's worth, you're right Thom, this is different than bug 1440276. Ed, maybe we can be smarter about when we update by keeping count of how many times we've entered onItemAdded within the timespan of one SYSTEM_TICK. If it's more than 10 times or 5 times or something then we just expire but not fetch, if not we can go ahead and call fetchHighlights?
Unless there's a way to know that we're doing a sync rather than just a regular bookmark add, and ignore those incoming events?

Comment 5

[Lina Butler [:lina]]

(In reply to Ursula Sarracini (:ursula) from comment #4)
> Unless there's a way to know that we're doing a sync rather than just a
> regular bookmark add, and ignore those incoming events?

I'm not sure if this helps, but Sync passes `PlacesUtils.bookmarks.SOURCES.SYNC` as the source to `onItem{Added, Removed, Changed, Moved}`. Would it make sense for Activity Stream to batch bookmark changes from SYNC, and maybe RESTORE and IMPORT?

Places used to support `on{Begin, End}UpdateBatch` for this, but the new async APIs don't call them. The observer overhaul in bug 1340498 should make this a lot easier eventually, but I think everyone needs to do their own batching today. :-(

Comment 6

[Ursula Sarracini (:ursula)]

Ed, is there any reason we can't just ignore bookmarks notifications that come in from a sync (i.e with `PlacesUtils.bookmarks.SOURCES.SYNC` as Kit mentioned)? The next time we update activity stream it will naturally get the new bookmarks that were saved that would show up in highlights anyways.

Comment 7

[Ed Lee :Mardak]

I suppose the main failure is the user explicitly synced and wanted to see the bookmarks show up immediately. Otherwise the periodic background updating of both synced bookmarks and bookmarks in highlights could probably just appear whenever.

One thing to note is that a bookmark appears in Highlights only if it both: 1) has a visit (moz_places.last_visit_date) and 2) is considered recent (moz_bookmarks.dateAdded). A plain bookmark creation doesn't trigger (1), and I believe sync does try to preserve original bookmark creation for (2). So even after syncing bookmarks and a fresh Highlights might not even show any additional bookmarks.

A separate alternate approach would be to watch for sync start/end notifications (?), and if we can differentiate manual vs automatic syncs… then do something different in activity stream.

Comment 8

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

It sounds like it might work OK to do both - ignore source=sync for a bookmark notification and on a bookmark sync finish notification rebuild the top sites?

Comment 9

[Ursula Sarracini (:ursula)]

I think that sounds like a good idea

Comment 10

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4035

Comment 11

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/4abb781bc58c352742f8ea091a8a8bdbf13a15f8
Fix Bug 1442383 - Activity stream spends a lot time fetching highlights after bookmark sync

https://github.com/mozilla/activity-stream/commit/8eb612ceebda75b60b425ba68e065cb3769da50c
Merge pull request #4035 from sarracini/bug_1442383

Fix Bug 1442383 - Activity stream spends a lot time fetching highlights after bookmark sync

Comment 12

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/9df46d48a21c