Closed Bug 707954 Opened 8 years ago Closed 8 years ago

Index exclusion for tags

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla11
Tracking Status
firefox9 --- wontfix
firefox10 --- verified

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, Whiteboard: [qa!])

Attachments

(1 file)

with lots of tags the query doesn't take the best path, we should enforce it
JOIN moz_bookmarks t ON t.id = +b.parent
this reduces time on a profile I have locally (with 1600 tags) to 30ms, from about 5 seconds.
Attached patch patch v1.0Splinter Review
Simple and powerful
Attachment #579501 - Flags: review?(dietrich)
The problem here happens because SQLite cannot know that tags are "special" entries in the bookmarks table, indeed it's a nonsense thing. It then just do an optimization thinking all bookmarks are equal, and not that some is more equal than others.
Comment on attachment 579501 [details] [diff] [review]
patch v1.0

Review of attachment 579501 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. is there a way to detect suboptimal execution choice like this? or do we need to do a manual examination of queries to find others that need column exclusion like this?
Attachment #579501 - Flags: review?(dietrich) → review+
I think we need to work on a test harness where to collect all of our queries, build avg and max dbs based on telemetry data, and all queries on those detecting variations. This would be far better than the Dirty tests we have now. I'll file a bug to track this.

In the meanwhile I think bug 699051 will greatly help us.
filed bug 708413
https://hg.mozilla.org/mozilla-central/rev/8db3e759e1f3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 579501 [details] [diff] [review]
patch v1.0

Review of attachment 579501 [details] [diff] [review]:
-----------------------------------------------------------------

I would like to port this simple queries fix to the stabilization branches, the results are pretty good, as I said on some query it may save seconds, someone noticed improvements to the awesome bar responsiveness in the last days. Will especially help users with many tags, that right now have a really bad responsiveness experience.
Attachment #579501 - Flags: approval-mozilla-beta?
Attachment #579501 - Flags: approval-mozilla-aurora?
Comment on attachment 579501 [details] [diff] [review]
patch v1.0

[Triage Comment]
We're now at the point in the 9 cycle where we're only taking fixes that may require us to chemspill if left unresolved. Approving for Aurora 10 though.
Attachment #579501 - Flags: approval-mozilla-beta?
Attachment #579501 - Flags: approval-mozilla-beta-
Attachment #579501 - Flags: approval-mozilla-aurora?
Attachment #579501 - Flags: approval-mozilla-aurora+
Thanks, I understand the will to stabilize.  Landed to Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/0c3f20e0594b
I assume this might be something QA can verify given access to the profile mentioned in comment 0. Would this be possible?
Whiteboard: [qa?]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> I assume this might be something QA can verify given access to the profile
> mentioned in comment 0. Would this be possible?

The profile is not mine, has been sent to me by the reporter of bug 701145 and I can't share it without his consent. We may ask him there, or I could make a test profile with many tags. Will do the former for now.
Thanks Marco for providing your profile off-line. Here are the results of our testing:

* in Firefox 10 beta 3 the search took about 2 seconds maybe less
* in Firefox 9.0.1 the same search took about 15 seconds
* in Firefox 8.0.1 the search took about 15 seconds
* in Firefox 7.0.1 the search took about 15 seconds

Given this and discussions offline with Marco, marking this verified.
Status: RESOLVED → VERIFIED
Whiteboard: [qa?] → [qa!]
You need to log in before you can comment on or make changes to this bug.