Closed Bug 1209027 Opened 5 years ago Closed 4 years ago

Reduce queries load on visits addition

Categories

(Toolkit :: Places, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox44 --- affected
firefox50 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(1 file)

Unfortunately I don't remember where, but in a bug I annotated some possible perf improvements for visits addition that could improve navigation perf.
One of the possible improvements is that, if we stop notifying the visit id (that is mostly pointless) we could remove the query that fetches it.

I noticed some other improvements just by looking at the queries log. To sum up, we fetch the page info twice for each visit addition, while we could just fetch once.
Priority: P1 → P2
Whiteboard: [fxsearch]
Blocks: 1138151
bug 1271035 is one of the examples where reducing this load would be a win (provided disabling history when it doesn't matter is still the best way to go)
Assignee: nobody → mak77
See Also: → 1271035
Blocks: 1265845
Depends on: 1261386
Marco, I was just wondering what the priority is of this for you. I need the changes we discussed it in order to finish the onVisited WebExtensions API event, in case that has any impact on its priority for you.
I have the patch improving performance ready.

I want to add the required information on top of this performance patch, so I'll likely move that to a dependency bug, cause I don't want to complicate the review process too much. I'm starting now on that, if there are no roadblocks I should have that patch ready by the end of the week.
I'll start by filing the new bug and fixing the dependency tree.
what does the patch do:
1. remove separate referrer object, passed around everywhere. We only need a few things from the referrer, so there's no need to move that much memory, we can just reuse the original place object.
2. add a thread-safe store_last_inserted_id() SQL function, that will store the last inserted id for visits and places (for now). This way we don't have to I/O just to fetch those ids. Note we cannot use last_insert_rowid() cause of thread-safety problems (hybrid sync/async) connection.
3. since now we always have the placeId, we don't need anymore 2 different paths (placeId or url) for most queries.
4. avoid unhiding places whose frecency was not zero, since those should already be unhidden
5. avoid an I/O access in case the referrer is the same as the page

This should have an interesting impact on both memory and I/O. We avoid at minimum 2 read queries per inserted visit, 4 read queries in the best case.
Blocks: 1277235
No longer blocks: 1265845
Comment on attachment 8758692 [details]
MozReview Request: Bug 1209027 - Reduce queries load on visits addition. r=adw

https://reviewboard.mozilla.org/r/56908/#review55216

Nice.

::: toolkit/components/places/History.cpp:924
(Diff revision 1)
>        bool typed = place.typed;
>        bool hidden = place.hidden;
>  
>        // We can avoid a database lookup if it's the same place as the last
>        // visit we added.
> -      bool known = lastFetchedPlace && lastFetchedPlace->IsSamePlaceAs(place);
> +      bool known = lastFetchedPlace && (*lastFetchedPlace).spec.Equals(place.spec);

Why (*lastFetchedPlace).spec instead of lastFetchedPlace->spec?

::: toolkit/components/places/History.cpp:1166
(Diff revision 1)
>  
>        rv = stmt->Execute();
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
>  
> -    if (!aPlace.hidden) {
> +    if (!aPlace.hidden && aPlace.frecency == 0) {

The frecency == 0 check confuses me a little.  The comments says "... if the frecency is now nonzero" and the SQL's where clause has frecency <> 0.  What exactly is going on in this block?  Is aPlace.frecency out of date in this case?
Attachment #8758692 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #7)
> Why (*lastFetchedPlace).spec instead of lastFetchedPlace->spec?

Good question, I don't know. Let me fix those.

> ::: toolkit/components/places/History.cpp:1166
> The frecency == 0 check confuses me a little.  The comments says "... if the
> frecency is now nonzero" and the SQL's where clause has frecency <> 0.  What
> exactly is going on in this block?  Is aPlace.frecency out of date in this
> case?

Sorry, the frecency updating code is a little bit hairy.
Here I was trying to guess the previous hidden status based on the previous frecency value. But that's confusing and probably wrong. So I'll rather introduce a .shouldUpdateHidden property.
The idea is to try to unhide a page only when it was previously marked as hidden, otherwise it'd be pointless work.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/fx-team/rev/c33f555ec47f
Reduce queries load on visits addition. r=adw
https://hg.mozilla.org/mozilla-central/rev/c33f555ec47f
https://hg.mozilla.org/mozilla-central/rev/0907a56af1fd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.