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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[Marco Bonardo [:mak]]

Attachment: 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.

Comment 3

[Shawn Wilsher :sdwilsh]

(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.

Comment 4

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

I like perf wins that are 7x faster

Comment 5

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/863c5c397e3b

Comment 6

[Shawn Wilsher :sdwilsh]

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

requesting approval for a nice performance win (7x faster) with minimal risk.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 8

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8ec9a419d943