https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/toolkit/components/places/SyncedBookmarksMirror.jsm#2146-2150,2395-2398 run for all new and existing items in the mirror...which, on a first mirror sync, will be *everything*. So we'll invalidate frecencies for all bookmarks, then recalculate them after the sync (https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/toolkit/components/places/SyncedBookmarksMirror.jsm#4530-4536), which is time-consuming, as bug 1462046 shows. Mak, what happens if we keep all frecencies at -1? https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/toolkit/components/places/SQLFunctions.cpp#819-825 suggests that's OK, they'll still show up in autocomplete. I wonder if we could just remove `updateFrecencies` from the mirror until we have a proper way to flag them for recalculation, like you suggest in bug 1462046, comment 18.

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #0) > Mak, what happens if we keep all frecencies at -1? They will still appear in the Address Bar, but with a minimum score. At least until we execute DecayFrecency() on idle-daily, that invokes FixInvalidFrecencies(). Unfortunately that means in the average case the results in the Address Bar will be broken for a day, in the worst case "forever" (until the next change to that place) because we know some users never hit idle-daily. > I wonder if we could just remove `updateFrecencies` from the > mirror until we have a proper way to flag them for recalculation, like you > suggest in bug 1462046, comment 18. A better idea could maybe be to set frecency = -frecency and doing the recalculation in chunks, ORDER BY frecency ASC LIMIT N. What I'm suggesting as a long term fix instead, is to add a frecency_recalc field to moz_places and just set it to true everywhere we must recalculate frecency, that would preserve the previous frecency value, that likely won't be so wrong, and then recalculate frecency in chunks using something like idleDispatch, still using WHERE NOT frecencyIsValid ORDER BY frecency DESC LIMIT N. The most problematic thing about this change is likely writing tests, but we can use waitForCondition to wait for specific frecency values...

Comment on attachment 8982735 [details] Bug 1463938 - Recalculate frecency in chunks for synced bookmarks. https://reviewboard.mozilla.org/r/248630/#review255416 ::: toolkit/components/places/SyncedBookmarksMirror.jsm:412 (Diff revision 1) > * GUIDs of bookmarks to weakly upload. > + * @param {Number} [options.maxFrecenciesToRecalculate] > + * The maximum number of bookmark URL frecencies to recalculate after > + * each merge. Frecency calculation is synchronous, and blocks other > + * Places queries from running, so we work through the backlog in > + * chunks. Is this effectively in chunks, or just handling the first chunk? I don't see a repetition. IF this relies on idle recalc, it should say that. ::: toolkit/components/places/SyncedBookmarksMirror.jsm:4563 (Diff revision 1) > frecency = CALCULATE_FRECENCY(id) > - WHERE frecency = -1`); > + WHERE id IN ( > + SELECT id FROM moz_places > + WHERE frecency < 0 > + ORDER BY frecency <> -1, frecency > + LIMIT :limit please always explicit ASC or DESC, even if the default is fine. I'm not sure I understand the ORDER BY frecency <> -1, could you please explicit that in a comment? Why do you want to handle -1 frecencies first?

Comment on attachment 8982735 [details] Bug 1463938 - Recalculate frecency in chunks for synced bookmarks. https://reviewboard.mozilla.org/r/248630/#review255416 > Is this effectively in chunks, or just handling the first chunk? I don't see a repetition. IF this relies on idle recalc, it should say that. I'll clarify in the comment. The "chunks" are every time we call `apply`, so we'll only handle one chunk here. On the next sync, we'll handle the next chunk, and so on, until we've recalculated everything. If idle recalculation happens before the next sync, that's also fine. We'll use those frecencies instead of calculating ourselves. > please always explicit ASC or DESC, even if the default is fine. > > I'm not sure I understand the ORDER BY frecency <> -1, could you please explicit that in a comment? Why do you want to handle -1 frecencies first? My thinking was, these are new URLs, and so it's better to handle them first before existing ones. In retrospect, this doesn't really make sense: explicitly changing an existing bookmark's URL to point to another existing URL probably isn't common (citation needed), and all negative frecencies weigh the same (IIUC, Places will treat a frecency of "-140" the same as "-1", not "140, but stale"). There's no reason for this to be a special case.

Hmm, should we give https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/toolkit/components/places/nsNavHistory.cpp#3790-3792 the same treatment? It would be really unfortunate if the mirror avoided doing this work, only to have `FixInvalidFrecencies` try to recalculate everything on the next idle, anyway.

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #5) > Hmm, should we give > https://searchfox.org/mozilla-central/rev/ > 292d295d6b084b43b70de26a42e68513bb7b36a3/toolkit/components/places/ > nsNavHistory.cpp#3790-3792 the same treatment? It's probably a good idea. As I said, long term we can look into better solutions.

This isn't as critical now that bug 1467537 has been fixed, but I think it's still worth doing, since a large first sync with several thousand bookmarks will cause us to recalculate frecencies for all the new URLs.

Comment on attachment 8989906 [details] Bug 1463938 - Recalculate frecency in chunks on idle. https://reviewboard.mozilla.org/r/254906/#review262818 looks ok for now, one day we'll just delegate all frecency calculations to a separate module... ::: toolkit/components/places/nsNavHistory.cpp:367 (Diff revision 1) > + nsresult > + NotifyObservers() > + { > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + NS_ENSURE_STATE(obs); > + (void)obs->NotifyObservers(nullptr, TOPIC_FRECENCY_UPDATED, nullptr); From a quick search doesn't look like anyone is using this topic... I guess we can remove it. ::: toolkit/components/places/nsNavHistory.cpp:2635 (Diff revision 1) > - return NS_OK; > - } > -}; > - > nsresult > nsNavHistory::DecayFrecency() Let's rename this to FixAndDecayFrecencies ::: toolkit/components/places/nsNavHistory.cpp:3840 (Diff revision 1) > -}; > - > -} // namespace > - > nsresult > nsNavHistory::FixInvalidFrecencies() who is invoking this now? It was only invoked by DecayFrecency before, but you removed that call. Is not this dead code now?

Comment on attachment 8982735 [details] Bug 1463938 - Recalculate frecency in chunks for synced bookmarks. https://reviewboard.mozilla.org/r/248630/#review261876 ::: toolkit/components/places/SyncedBookmarksMirror.jsm:411 (Diff revision 2) > * The current server time, in seconds. > * @param {String[]} [options.weakUpload] > * GUIDs of bookmarks to weakly upload. > + * @param {Number} [options.maxFrecenciesToRecalculate] > + * The maximum number of bookmark URL frecencies to recalculate after > + * this merge. Frecency calculation is synchronous, and blocks other I don't think this is correct, frecency calculation should always be async... unless by syncrhonous you mean "not delayed".

Comment on attachment 8982735 [details] Bug 1463938 - Recalculate frecency in chunks for synced bookmarks. https://reviewboard.mozilla.org/r/248630/#review261876 > I don't think this is correct, frecency calculation should always be async... unless by syncrhonous you mean "not delayed". I meant "not delayed", but it's confusing, so reworded.

Comment on attachment 8989906 [details] Bug 1463938 - Recalculate frecency in chunks on idle. https://reviewboard.mozilla.org/r/254906/#review262818 > From a quick search doesn't look like anyone is using this topic... I guess we can remove it. Done! > who is invoking this now? It was only invoked by DecayFrecency before, but you removed that call. > Is not this dead code now? It is, I forgot to remove it. :-) Thanks!

Comment on attachment 8989906 [details] Bug 1463938 - Recalculate frecency in chunks on idle. https://reviewboard.mozilla.org/r/254906/#review262932 ::: toolkit/components/places/tests/unit/test_frecency_observers.js:58 (Diff revision 2) > - // FixInvalidFrecencies is at the end of a path that DecayFrecency is also on, > - // so expect two notifications. Trigger the path by making nsNavHistory > + // Fix and decay frecencies by making nsNavHistory observe the idle-daily > + // notification. > - // observe the idle-daily notification. > PlacesUtils.history.QueryInterface(Ci.nsIObserver). > observe(null, "idle-daily", ""); > - await Promise.all([onManyFrecenciesChanged(), onManyFrecenciesChanged()]); > + await Promise.all([onManyFrecenciesChanged()]); I'm not sure if we want this. Before, we'd notify from both paths, but, since we've merged them now, I think we can collapse into one notification. WDYT?

(In reply to Lina Cambridge (she/her) [:lina, :kitcambridge] from comment #16) > I'm not sure if we want this. Before, we'd notify from both paths, but, > since we've merged them now, I think we can collapse into one notification. > WDYT? I agree, one notification should be enough.

Comment on attachment 8989906 [details] Bug 1463938 - Recalculate frecency in chunks on idle. https://reviewboard.mozilla.org/r/254906/#review263050 Thanks

Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1b72ef544d88 Recalculate frecency in chunks for synced bookmarks. r=mak https://hg.mozilla.org/integration/autoland/rev/8de2dc60c55b Recalculate frecency in chunks on idle. r=mak

https://hg.mozilla.org/mozilla-central/rev/1b72ef544d88 https://hg.mozilla.org/mozilla-central/rev/8de2dc60c55b

Would this bug have had the effect of greatly reducing the amount of time reported by PLACES_IDLE_FRECENCY_DECAY_TIME_MS? It looks as though about a third of these idle decays take less than 50ms now (previously it was a sixth). http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/285/alerts/?from=2018-07-11&to=2018-07-11 https://mzl.la/2vWFxC8

Oops, I'm sorry I missed this, Chutten! >.> https://hg.mozilla.org/mozilla-central/rev/8de2dc60c55b did change how we collect `PLACES_IDLE_FRECENCY_DECAY_TIME_MS`. I expected some difference, but not so significant. Before, we recorded the start time on the main thread, ran all queries on the async thread, then recorded the time once the queries finished running and we were back on the main thread. Now, we only record the raw time it takes to run the queries, all on the async thread. Maybe the reduction is from removing the overhead of dispatching back and forth? I think it would be tricky to revert to how we recorded the timings before, since we also fix invalid frecencies as part of the same runnable.

No worries! Maybe it's also possible this is no longer measuring time spent waiting for the async thread to be scheduled? It has "idle" in the name which suggests that the main thread ought not be particularly busy, but *insert shrugging emoji here* As far as I know this metric isn't part of a product review or program analysis so it's fine to change however you want it to. When we come across these changes and can't pin them down we start to wonder if something's wrong. Glad to hear in this instance that isn't the case. Notifying people about changes we notice is just one of the services our team provides :)