503360 - Better queries for asynchronous location bar

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

This bug is to investigate/find queries that better behave with async location bar.

Comment 1

[Marco Bonardo [:mak]]

Attachment: checkpoint 1

This fixes the main base query, is blazing fast.

still investigating the other queries...

Comment 2

[Shawn Wilsher :sdwilsh]

Couple of things:
1) I don't think it's worthwhile to make this a lazy getter, but using a function to share the similarities is a good idea.
2) We should LIMIT each of these queries to |this._maxResults| so we don't do way more work than we have to (even if it is on a different thread).

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

limiting on maxRichResults adds the need to recreate statements when that pref changes, this moves the pref dependent statements in loadPrefs

Comment 4

[Marco Bonardo [:mak]]

can't better optimize other queries, i tried a bunch of ideas, but they take just 3ms each, any change does not move that timing and due to timers resolution is hard to see if minor movements are toward up or down. btw all changes i did try caused bigger explains, so i doubt those could have given any microsec gain.

Further optimization will come mostly from schema changes, particularly getting tags out of bookmarks table will remove the need for 3 joins in each query.

Comment 5

[Shawn Wilsher :sdwilsh]

Why does it require to create new statements?  Why can't we bind the limit like we do in the current implementation on trunk?

Comment 6

[Marco Bonardo [:mak]]

oh, the LIMIT -1 trick? yes, sounds like a wise idea.

Comment 7

[Shawn Wilsher :sdwilsh]

I'm not familiar with that - I'm referring to doing LIMIT :limit in the query, and then binding it where we bind other parameters.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.1

yeah, i need to switch off some time :)

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 387870 [details] [diff] [review]
patch v1.1

>+  var sql_fragment = function(aTableName) {
nit: base_sql_fragment, and I think you can just declare the function inline and not use a variable.  If you can't though, s/var/let/ please

>+  this._sql_base = sql_fragment("moz_places_temp") +
no reason to use a member variable here, is there?

r=sdwilsh with comments addressed.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: patch v1.2

Addresses my review comments

Comment 11

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/projects/places//rev/98c36c2d0a52

Comment 12

[Shawn Wilsher :sdwilsh]

Forgot to mark this as fixed earlier...
http://hg.mozilla.org/mozilla-central/rev/98c36c2d0a52