mDBVisitsForFrecency query doesn't use indices for sorting (we can be 7x faster!)

RESOLVED FIXED

Status

()

Toolkit
Places
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: mak)

Tracking

({fixed1.9.1})

Trunk
x86
Mac OS X
fixed1.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
I'm seeing this a lot in my console with the patch in bug 481261 applied.  We have a likely inefficient query here:

WARNING: 1 sort operation has occurred for the SQL statement 'SELECT COALESCE(r_t.visit_date, r.visit_date, v.visit_date) date, COALESCE(r_t.visit_type, r.visit_type, v.visit_type) FROM ( SELECT visit_date, visit_type, from_visit FROM moz_historyvisits_temp WHERE place_id = ?1 UNION ALL SELECT visit_date, visit_type, from_visit FROM moz_historyvisits WHERE id NOT IN (SELECT id FROM moz_historyvisits_temp) AND place_id = ?1 ) AS v LEFT JOIN moz_historyvisits r ON r.id = v.from_visit AND v.visit_type IN (5,6) LEFT JOIN moz_historyvisits_temp r_t ON r_t.id = v.from_visit AND v.visit_type IN (5,6) ORDER BY date DESC LIMIT 10'. This may indicate an opportunity to improve performance through the careful use of indexes.: file /Code/moz/central/mozilla/storage/src/mozStoragePrivateHelpers.cpp, line 104

I can't land bug 481261 until this gets fixed because it's pretty darn noisy.
(Assignee)

Comment 1

9 years ago
it's mDBVisitsForFrecency
used by calculatefrecencyInternal, it's the query that samples the visits to calculate frecency.
Number of returned results should be the number of visits on a place_id, won't be so high, but probably the query can be improved.
(Assignee)

Comment 2

9 years ago
Created attachment 366379 [details] [diff] [review]
patch v1.0

this query is quite (7x) faster here and does not warn, but this is another case where i see UNION ALL quite faster then UNION.
Btw this can use indices on both tables for order by, using the original visit_date instead of the redirected one does not change a lot for us, they are added at ms distance.

SELECT v.visit_date, COALESCE(
       (SELECT r.visit_type FROM moz_historyvisits_temp r WHERE v.visit_type IN (5,6) AND r.id = v.from_visit),
       (SELECT r.visit_type FROM moz_historyvisits r WHERE v.visit_type IN (5,6) AND r.id = v.from_visit),
       visit_type)
FROM moz_historyvisits_temp v
WHERE v.place_id = ?1
UNION ALL
SELECT v.visit_date, COALESCE(
       (SELECT r.visit_type FROM moz_historyvisits_temp r WHERE v.visit_type IN (5,6) AND r.id = v.from_visit),
       (SELECT r.visit_type FROM moz_historyvisits r WHERE v.visit_type IN (5,6) AND r.id = v.from_visit),
       visit_type)
FROM moz_historyvisits v
WHERE v.place_id = ?1
AND v.id NOT IN (SELECT id FROM moz_historyvisits_temp)
ORDER BY 1 DESC LIMIT ?2
;

taking for now.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #366379 - Flags: review?(sdwilsh)
(Assignee)

Updated

9 years ago
Summary: WARNING: We have an inefficient query! → mDBVisitsForFrecency query can't use indices for sorting
(Reporter)

Comment 3

9 years ago
(In reply to comment #2)
> this query is quite (7x) faster here and does not warn, but this is another
> case where i see UNION ALL quite faster then UNION.
That makes sense since UNION is going to look to filter out any duplicates, whereas UNION ALL doesn't.
(Reporter)

Updated

9 years ago
Attachment #366379 - Flags: review?(sdwilsh) → review+
(Reporter)

Comment 4

9 years ago
Comment on attachment 366379 [details] [diff] [review]
patch v1.0

r=sdwilsh

I like perf wins that are 7x faster
(Reporter)

Updated

9 years ago
Summary: mDBVisitsForFrecency query can't use indices for sorting → mDBVisitsForFrecency query doesn't use indices for sorting (we can be 7x faster!)
(Assignee)

Comment 5

9 years ago
http://hg.mozilla.org/mozilla-central/rev/863c5c397e3b
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1?
Target Milestone: mozilla1.9.2a1 → ---
(Reporter)

Comment 6

9 years ago
Comment on attachment 366379 [details] [diff] [review]
patch v1.0

requesting approval for a nice performance win (7x faster) with minimal risk.
Attachment #366379 - Flags: approval1.9.1?
Attachment #366379 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.