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

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tcsc, Assigned: ursula)

Tracking

unspecified
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

a year ago
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

a year ago
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

a year ago
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.
Reporter

Comment 3

a year ago
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.
Assignee

Comment 4

a year ago
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?
(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. :-(
Assignee

Comment 6

a year ago
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

a year ago
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.
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?
Assignee

Updated

a year ago
Assignee: nobody → usarracini
Priority: -- → P1
Assignee

Comment 9

a year ago
I think that sounds like a good idea

Comment 11

a year ago
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

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Updated

a year ago
Blocks: 1444522

Comment 12

a year ago
https://hg.mozilla.org/mozilla-central/rev/9df46d48a21c
Iteration: --- → 60.4 - Mar 12
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.