Reduce queries load on visits addition

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
Places
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox50 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Priority: P1 → P2
Whiteboard: [fxsearch]
(Assignee)

Updated

2 years ago
Blocks: 1138151
(Assignee)

Comment 1

2 years ago
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: → bug 1271035
(Assignee)

Updated

a year ago
Blocks: 1265845
(Assignee)

Updated

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

Comment 3

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

Comment 4

a year ago
Created attachment 8758692 [details]
MozReview Request: Bug 1209027 - Reduce queries load on visits addition. r=adw

Review commit: https://reviewboard.mozilla.org/r/56908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56908/
Attachment #8758692 - Flags: review?(adw)
(Assignee)

Comment 5

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

Comment 6

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a3e515513e3e779f22d11e34373adc8d0eb14cd
(Assignee)

Updated

a year ago
Blocks: 1277235
(Assignee)

Updated

a year ago
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+
(Assignee)

Comment 8

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

Comment 9

a year ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/fx-team/rev/c33f555ec47f
Reduce queries load on visits addition. r=adw

Comment 10

a year ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/fx-team/rev/0907a56af1fd
add missing explicit. r=bustage

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c33f555ec47f
https://hg.mozilla.org/mozilla-central/rev/0907a56af1fd
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.