Places result refreshes for every visit, even if the query is limited to the downloads visit transition

RESOLVED FIXED in Firefox 61

Status

()

defect
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: ursula, Assigned: mak)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

While trying to add recent downloads to activity stream's highlights, I noticed on every new history entry, that I was receiving an onDownloadAdded notification that looks to be triggered by a call to invalidateContainer in DownloadsHistory.jsm. The code I'm using plugs right into the normal download functionality where we use DownloadsCommon.getData, and then add a view which will then be notified when changes to downloads happen. Basically what we're doing here: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/browser/components/downloads/content/allDownloadsView.js#224-225

The difference with activity stream though, is that the view gets added to a module which sticks around in the global scope until we shut down the browser. Whereas, for example, the downloads list found under library > downloads doorhanger, only adds a view once you've clicked on the doorhanger for downloads, and then removes the view once the doorhanger disappears. This means in the time that the doorhanger is visible, no new visits happen, and this view doesn't get onDownloadAdded notifications. For more context, here's a WIP pull request to add the feature to activity stream highlights: https://github.com/mozilla/activity-stream/pull/4076/files. You can see that we use DownloadsCommon.getData and then addView to this module, which gets initializes basically at browser startup.

:mak and I did some preliminary debugging and saw that there might be an optimization that we can do here so as not to refresh the whole contents on every visit if we get a visit notification for a different (like downloads in this case) transaction.

My main concern here is that by adding this view and receiving notifications on every history visit, these are going to show up in a profile as a perf regression.

Paolo, do you have any ideas about optimizations here to prevent DownloadsHistory from over-notifying, so that we don't get onDownloadAdded on every Places change?
From the quick debugging we did on IRC, the problem is that a place: history query with maxResults gets mLiveUpdate = QUERYUPDATE_COMPLEX, because when a new visit is added, we don't know if it falls into the maxResults limit or not.
Thus, when we get onVisit we end up here
https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/toolkit/components/places/nsNavHistoryResult.cpp#2551
and we do a full refresh, that in the end will cause invalidateContainer.

DownloadHistoryList.invalidateContainer then calls _insertPlacesNode, that calls _insertSlot that notifies onDownloadAdded.

It looks like there's space for 2 optimizations here:
1. the Places result may ignore the notification if it's limited to specific transitions and the received visit has a different transition
2. DownloadHistoryList could mayve do some kind of "diff" and only notify for really new things?

I can probably look into the former, but I'm a little bit rusty on the latter, and that's why I suggested asking Paolo.
I'd say that the better option is to optimize this in Places. The addition of a new entry in the database will normally add a single new element at the top of the list, and probably remove one at the end. Does this same bug affect our lists in the menus, that are similarly filtered with maxResults?

Separately, I'll mention that calling getList right after startup is going to initialize the entire Downloads subsystem, which will perform additional I/O to read the data of past session downloads from a different file than the Places database. We currently wait 10 seconds after startup before we initialize the Downloads button in the toolbar for this reason.
(In reply to :Paolo Amadini from comment #2)
> I'd say that the better option is to optimize this in Places. The addition
> of a new entry in the database will normally add a single new element at the
> top of the list, and probably remove one at the end.

Sync can add visits at any time in any order, we can predict anything about where a visit is being added, and that's the main reason we can't just add to the top, but we have to invalidate the whole container if it's a limited view.

> Does this same bug
> affect our lists in the menus, that are similarly filtered with maxResults?

As Ursula said, our current views create and destroy the list every time, and as such the likelihood for visits to happen during that timeframe is really short.
sorry I meant "we *can't* predict anything"
What I'm suggesting is to simplify the notification when the visit would be appended at the top, which is the common case. If sync adds visits in the middle, we can trigger the full invalidation. The various Downloads views are also optimized to handle this case.
I can check what we can do, but it may not be trivial to know if a visit will be appended at the top, since sorting and other querying options will change easily the definition of "top".
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxsearch]
And just to be clear, all this querying code is on the main-thread atm. we lack resources to move Places queries off the main-thread for now, it will likely be the next async-ify effort after history and bookmarks are complete (but likely not before 2019, unless we get more devs). There are workarounds to make this async, like using asyncExecuteLegacyQueries and an history observer + an annotations observer. it's a bit more complex than the current code.

I'm pointing this out because even with optimizations, having this result open in background may cause synchronous I/O, if a Refresh() is issued for any reason.
Summary: Places refreshes for every visit, even if the query is limited to the downloads visit transition → Places result refreshes for every visit, even if the query is limited to the downloads visit transition
mak, how bad is this bug if we land bug 1433230 without this fix in 61? Assuming we appropriately delay download initialization, it looks like talos is not impacted even without this fix, but this behavior might just be something not measured.
Blocks: 1433230
Flags: needinfo?(mak77)
(In reply to Ed Lee :Mardak from comment #8)
> mak, how bad is this bug if we land bug 1433230 without this fix in 61?
> Assuming we appropriately delay download initialization, it looks like talos
> is not impacted even without this fix, but this behavior might just be
> something not measured.

I plan to work on this tomorrow, hopefully it will be done by EOW, in any case I think we never had a constantly active history view, so it can have a very negative impact in general on browsing speed.
Flags: needinfo?(mak77)
Comment on attachment 8968596 [details]
Bug 1453353 - Optimize transition limited history results to not invalidate on every visit.

https://reviewboard.mozilla.org/r/237284/#review243286

::: toolkit/components/places/nsNavHistoryResult.cpp:2525
(Diff revision 1)
> +      // Optimization for a common case: if the query has maxResults and is
> +      // sorted by date, get the current boundaries and check if the added visit
> +      // would fit.
> +      // Later, we may have to remove the last child to respect maxResults.
> +      if (mOptions->MaxResults() &&
> +          mChildren.Count() >= mOptions->MaxResults()) {

Looks like we need some casting here, try is complaining:

error: comparison of integers of different signs: 'int32_t' (aka 'int') and 'uint32_t' (aka 'unsigned int')

::: toolkit/components/places/nsNavHistoryResult.cpp:2566
(Diff revision 1)
>          }
>        }
>  
> +      // Trim the children if necessary.
> +      if (mOptions->MaxResults() &&
> +          mChildren.Count() > mOptions->MaxResults()) {

Casting here as well:

error: comparison of integers of different signs: 'int32_t' (aka 'int') and 'uint32_t' (aka 'unsigned int')

::: toolkit/components/places/tests/queries/test_downloadHistory_liveUpdate.js:28
(Diff revision 1)
> +  });
> +  result.addObserver(resultObserver, false);
> +  return resultObserver;
> +}
> +
> +add_task(async function test_downloadhistory_query() {

Could we extend this test, or add a new one to check that added/updated items are correctly filtered for the live update?

Hitting the max results limit and checking that would also be good.
Attachment #8968596 - Flags: review?(standard8)
Comment on attachment 8968596 [details]
Bug 1453353 - Optimize transition limited history results to not invalidate on every visit.

https://reviewboard.mozilla.org/r/237284/#review243336


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/places/nsNavHistoryResult.cpp:2531
(Diff revision 3)
> +        uint16_t sortType = GetSortType();
> +        if (sortType == nsINavHistoryQueryOptions::SORT_BY_DATE_ASCENDING &&
> +            aTime > std::max(mChildren[0]->mTime,
> +                             mChildren[mChildren.Count() -1]->mTime)) {
> +          return NS_OK;
> +        } else if (sortType == nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING &&

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

 0:13.33         } else if (sortType == nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING &&
 0:13.33           ^~~~~~~~
 0:13.33 Warning: modernize-use-equals-default in /cache/sa-central/toolkit/components/places/nsNavHistoryResult.cpp: use '= default' to define a trivial destructor
Comment on attachment 8968596 [details]
Bug 1453353 - Optimize transition limited history results to not invalidate on every visit.

https://reviewboard.mozilla.org/r/237284/#review243390

Much nicer with the extra test. r=Standard8
Attachment #8968596 - Flags: review?(standard8) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/aeb5dd47f88a
Optimize transition limited history results to not invalidate on every visit. r=standard8
https://hg.mozilla.org/mozilla-central/rev/aeb5dd47f88a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.