659740 - Frecency update causes SQL sort warning

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Richard Newman [:rnewman]]

WARNING: 1 sort operation has occurred for the SQL statement '0x10bb8b578'.  See https://developer.mozilla.org/En/Storage/Warnings details.: file /Users/rnewman/moz/git/alder/storage/src/mozStoragePrivateHelpers.cpp, line 144

Every. Damn. Record.

Comment 1

[Philipp von Weitershausen [:philikon]]

Marco + sdwilsh: is there a way for us to see what query might be triggering this warning?

Comment 2

[Shawn Wilsher :sdwilsh]

NSPR logging is generally the way to go.  You can also change https://hg.mozilla.org/mozilla-central/annotate/5e1ee7f192ad/storage/src/mozStoragePrivateHelpers.cpp#l118 to print the sql string instead of a pointer.

Comment 3

[Richard Newman [:rnewman]]

The query culprit is

  SELECT ROUND((strftime('%s','now','localtime','utc') - v.visit_date/1000000)/86400), COALESCE( (SELECT r.visit_type FROM moz_historyvisits r WHERE v.visit_type IN (5,6)  AND r.id = v.from_visit), visit_type), visit_date FROM moz_historyvisits v WHERE v.place_id = :page_id ORDER BY visit_date DESC LIMIT 10

from

  toolkit/components/places/nsNavHistory.cpp:1295

That query gets called for each history record, presumably via a Places API call.

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> That query gets called for each history record, presumably via a Places API
> call.
Shouldn't be hard to figure out which.  Getting a backtrace is the way to move forward here.

Comment 5

[Marco Bonardo [:mak]]

You are adding a visit and that causes us to recalculate frecency for that page, this happens on the async thread though.

I touched this code today, so I could look why this warns, the problem may be minimal though, like caused by the subquery.

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> I touched this code today

That should be "I'm working on a patch that touches this code", but my patch is mostly removing some supporting code, not fixing any query or such.

Comment 7

[Richard Newman [:rnewman]]

(In reply to comment #5)

> I touched this code today, so I could look why this warns, the problem may
> be minimal though, like caused by the subquery.

I'll just say: we'd all love to see this go away :D  Thanks mak!

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I think we are going to see an increase in warnings, looks like current SQLite sometimes ignores existing indices, that's because if there are a few entries (like 2 or 3) doing sorting using a scan is faster than doing sorting using a b-tree, indeed in this case if I force it to use the index (through INDEXED BY moz_historyvisits_placedateindex) the warning goes away. All of this uses sqlite_stat1 (and, if present, sqlite_stat2) table, so ANALYZE is really important.
Based on this, I've added a /* do not warn */ since the indices exist but are ignored by the optimizer.

Apart this, the patch slightly improves the queries, and introduces 2 stataments caches for them, removing all the crazy stuff that was introduced to init those statements. In a separate bug we can remove GetStatementByID and GetStatement and directly use these caches for all statements.

Comment 9

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 10

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/8b9ce2b1e438

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/8b9ce2b1e438