Better queries for asynchronous location bar

RESOLVED FIXED in mozilla1.9.2a1

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

Trunk
mozilla1.9.2a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
This bug is to investigate/find queries that better behave with async location bar.
(Assignee)

Comment 1

9 years ago
Created attachment 387740 [details] [diff] [review]
checkpoint 1

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

Comment 3

9 years ago
Created attachment 387845 [details] [diff] [review]
patch v1.0

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

Comment 4

9 years ago
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.
(Assignee)

Updated

9 years ago
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?
(Assignee)

Comment 6

9 years ago
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.
(Assignee)

Comment 8

9 years ago
Created attachment 387870 [details] [diff] [review]
patch v1.1

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+
Created attachment 387930 [details] [diff] [review]
patch v1.2

Addresses my review comments
Attachment #387870 - Attachment is obsolete: true
Attachment #387930 - Flags: review+
Forgot to mark this as fixed earlier...
http://hg.mozilla.org/mozilla-central/rev/98c36c2d0a52
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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.