Closed Bug 482276 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Places, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sdwilsh, Assigned: mak)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file)

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.
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.
Attached patch patch v1.0Splinter Review
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)
Summary: WARNING: We have an inefficient query! → mDBVisitsForFrecency query can't use indices for sorting
(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.
Attachment #366379 - Flags: review?(sdwilsh) → review+
Comment on attachment 366379 [details] [diff] [review]
patch v1.0

r=sdwilsh

I like perf wins that are 7x faster
Summary: mDBVisitsForFrecency query can't use indices for sorting → mDBVisitsForFrecency query doesn't use indices for sorting (we can be 7x faster!)
http://hg.mozilla.org/mozilla-central/rev/863c5c397e3b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Flags: wanted1.9.1?
Target Milestone: mozilla1.9.2a1 → ---
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+
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.