830423 - Avoid repeated execution of expensive daysOfHistory query

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/setup.js#876

(In reply to Taras Glek (:taras) from comment #32)
> Sync is doing main thread io calling 
> SELECT ROUND(( strftime('%s','now','localtime','utc') - ( SELECT visit_date
> FROM moz_historyvisits ORDER BY visit_date ASC LIMIT 1 )/1000000 )/86400) AS
> daysOfHistory  /* places.sqlite */). This is causing contention & freezes

Taras notified 24 calls in a short amount of time, this is all main-thread IO, and its only purpose is to update a UI string (historyDaysCount.label) that tells the user how much history he has.

1. I'm not sure why this has to be updated so often in a short amount of time and not just before a user action
2. if the string is there just to make sure the user doesn't overwrite long history with short history, it doesn't have to be 1-visit precise
3. likely that value could be cached and invalidated when a new visit is added, sync has this info

Comment 1

[Marco Bonardo [:mak]]

at this point I suppose also bookmarksCount.label and passwordsCount.label are not fresh water on our main thread.

Comment 2

[Richard Newman [:rnewman]]

This code should only run during sync setup, when you're choosing which option to use ("Sync will overwrite 16 days of local history…"). It should only run once, during the wizard or reset, and usually on a small profile, so caching seems pointless.

Are you seeing it run in other contexts?

Comment 3

[(dormant account)]

(In reply to Richard Newman [:rnewman] from comment #2)
> This code should only run during sync setup, when you're choosing which
> option to use ("Sync will overwrite 16 days of local history…"). It should
> only run once, during the wizard or reset, and usually on a small profile,
> so caching seems pointless.
> 
> Are you seeing it run in other contexts?

I saw this query execute 24 times during normal browser usage. I did not add any new sync devices during this time.

Comment 4

[Marco Bonardo [:mak]]

notice the query in Sync is copied from the query in Places, we don't know 100% which of the 2 is firing. The query in Places happens only if the history Sidebar or the Library are open, and may happen if Sync executes many runInBatch calls.
That has to be debugged and verified, may be comment 2 is right and this is just a side effect of batching many additions and not wrapping those additions into a larger batch.

Comment 5

[Richard Newman [:rnewman]]

Manually tested by adding a dump where that query is invoked in Sync. I could only get it to execute by starting the pairing process and clicking through Sync Options, or visiting Reset Sync.

If this is Sync's fault, it's not through direct invocation.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] from comment #4)
> The query in Places happens only if the
> history Sidebar or the Library are open, and may happen if Sync executes
> many runInBatch calls.

History calls updatePlaces with 50 records at a time. 

Bookmarks calls runInBatchMode explicitly around the entire engine sync operation, including all queries, inserts (insertBookmark), and updates (changeBookmarkURI etc.).

Is there something else we should be doing, or is this a Places UI niggle?

Comment 7

[Marco Bonardo [:mak]]

Thanks I will investigate it Place side and see what I can find.
My suspect is that those queries are executed when browsing while having the Library window open and something causes a views refresh (may be a batch, or a simple notification that the view doesn't know how to handle). I must verify that.

What Sync does with runInBatchMode looks correct, provided it's done only if there are actual changes to sync, I expect otherwise it won't do anything.
Though we have some implicit batches, like tagging/untagging, that should be verified. If sync does tagging/untagging inside the above batch it's likely not the direct culprit.

Taking this for now to investigate.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] from comment #7)

> What Sync does with runInBatchMode looks correct, provided it's done only if
> there are actual changes to sync,

It incorporates the entire engine-specific sync: deciding whether to sync, downloading items, applying them, fetching new items to upload, uploading them, and persisting any state.

Is that incorrect? Would it be better to put the batching around a smaller chunk of work?

> Though we have some implicit batches, like tagging/untagging, that should be
> verified. If sync does tagging/untagging inside the above batch it's likely
> not the direct culprit.

We tag and untag within Bookmarks' batch.

Thanks, mak!

Comment 9

[Marco Bonardo [:mak]]

(In reply to Richard Newman [:rnewman] from comment #8)
> It incorporates the entire engine-specific sync: deciding whether to sync,
> downloading items, applying them, fetching new items to upload, uploading
> them, and persisting any state.
> 
> Is that incorrect? Would it be better to put the batching around a smaller
> chunk of work?

the only "incorrect" thing I see up there is "deciding whether to sync".. batch should only wrap writes and you should not start a batch until you know you will do some writes. Batches make writes cheaper, but they are also expensive if done for any other reason.

Comment 10

[Marco Bonardo [:mak]]

also fetching new items to upload, upload and any other read operation. basically there is no gain using a batch around reads, rather a loss.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] from comment #10)
> also fetching new items to upload, upload and any other read operation.
> basically there is no gain using a batch around reads, rather a loss.

OK. I'll file a bug to investigate whether this is worth changing for the current bookmark engine (probably).

Should we be explicitly wrapping a batch execution around history items (if we can), or does updatePlaces do so?

Comment 12

[Marco Bonardo [:mak]]

updatePlaces handles the thing by itself for now, so that's just for bookmarks.

Comment 13

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #11)

> OK. I'll file a bug to investigate whether this is worth changing for the
> current bookmark engine (probably).

Filed (and possibly fixed) Bug 830502.

Comment 14

[Marco Bonardo [:mak]]

So, this is caused by the history view:
Today
Yesterday
January
...

Thus, it happens when either:
1. the Library window is open and History container is open
2. the history sidebar By Date or By Date And Site is open

Unfortunately, this view is complicated to live-update (especially when we cross the midnight and today becomes yesterday), so we just refresh it every time, this is not extremely expensive cause it's a maximum of 6 containers. But figuring out the number of container may be a bit expensive (thus this bug!)
What I can do here, is figure out a good condition to tell the view it doesn't need to be refreshed, to avoid this work in most cases.

Comment 15

[Marco Bonardo [:mak]]

So I plan to improve this in 2 parts:
1. cache the daysOfHistory value and invalidate the cache on removals or first visit of the day.  Moreover unify this with hasHistoryEntries, so any of those will keep the cache up-to-date and there's just one query for both.
2. Teach the query that onVisit it should refresh only if the new visit is "tomorrow"

This should globally reduce number of these 2 queries.
I have a patch for (1), (2) will be funny :)

Comment 16

[Marco Bonardo [:mak]]

Attachment: part 1 - Cache daysOfHistory and reuse it for hasHistoryEntries

Comment 17

[Marco Bonardo [:mak]]

Attachment: Part 2 - Don't refresh the view if Today exists and the visit falls into it

Comment 18

[Marco Bonardo [:mak]]

https://tbpl.mozilla.org/?tree=Try&rev=02d76aed448b

Comment 19

[Asaf Romano (gone)]

Comment on attachment 711851 [details] [diff] [review]
part 1 - Cache daysOfHistory and reuse it for hasHistoryEntries

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

::: toolkit/components/places/nsNavHistory.cpp
@@ +735,5 @@
>  {
>    MOZ_ASSERT(!aGUID.IsEmpty());
> +  if (mDaysOfHistory == 0) {
> +    mDaysOfHistory = 1;
> +  } else if (aTime > mLastCachedEndOfDay || aTime < mLastCachedStartOfDay) {

Please comment here explaining the condition etc.

@@ +779,3 @@
>    bool hasResult;
>    if (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
> +    // If we get NULL, then there's no visits, otherwise there must always be

nit: there are no visits.

@@ +1140,5 @@
>  
>  NS_IMETHODIMP
>  nsNavHistory::GetHasHistoryEntries(bool* aHasEntries)
>  {
> +  MOZ_ASSERT(NS_IsMainThread(), "This can only be called on the main thread");

It's probably better to assert in GetDaysOfHistory.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 711853 [details] [diff] [review]
Part 2 - Don't refresh the view if Today exists and the visit falls into it

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

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +4630,5 @@
>      // that a matching query either was not expanded or it does not exist.
>      uint32_t resultType = mRootNode->mOptions->ResultType();
>      if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY ||
> +        resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_SITE_QUERY) {
> +      // If the visit falls into the today bucket and the bucket exists, it was

nit: Today bucket.

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ea788e50f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9fe5a74270

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6ea788e50f6
https://hg.mozilla.org/mozilla-central/rev/5e9fe5a74270