Closed Bug 503360 Opened 13 years ago Closed 13 years ago

Better queries for asynchronous location bar


(Toolkit :: Places, defect)

Not set





(Reporter: mak, Assigned: mak)




(1 file, 3 obsolete files)

This bug is to investigate/find queries that better behave with async location bar.
Attached patch checkpoint 1 (obsolete) — Splinter Review
This fixes the main base query, is blazing fast.

still investigating the other queries...
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).
Attached patch patch v1.0 (obsolete) — Splinter Review
limiting on maxRichResults adds the need to recreate statements when that pref changes, this moves the pref dependent statements in loadPrefs
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.
Attachment #387845 - Flags: review?(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?
oh, the LIMIT -1 trick? yes, sounds like a wise idea.
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.
Attached patch patch v1.1 (obsolete) — Splinter Review
yeah, i need to switch off some time :)
Attachment #387740 - Attachment is obsolete: true
Attachment #387845 - Attachment is obsolete: true
Attachment #387870 - Flags: review?(sdwilsh)
Attachment #387845 - Flags: review?(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.
Attachment #387870 - Flags: review?(sdwilsh) → review+
Attached patch patch v1.2Splinter Review
Addresses my review comments
Attachment #387870 - Attachment is obsolete: true
Attachment #387930 - Flags: review+
Forgot to mark this as fixed earlier...
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-places]
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.