Closed Bug 386396 Opened 18 years ago Closed 2 years ago

investigate if we can push filtering down into a sql query, instead of filtering at FilterResultSet()

Categories

(Firefox :: Bookmarks & History, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: moco, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: perf, Whiteboard: [Snappy:P3][snt-scrubbed])

FilterResultSet() results earlier, in RowToResult() right now, we build up all the results, append to our array, and then filter For filtering, we should do this in RowToResult(). Do our check of title-contains-filter-string || url-contains-filter-string there, and only on match, do we append. this will work if we ever extend our search to annotations as well, as if those checks fail, we can query. this will save us allocations, copying, and all the work we do in RowToResult() that we won't have to do if there is no match. think about filtering 10,000 history items on "cheese ball" to get just that one result you care about. I was going to suggest we look into using SQL's OR operators and LIKE predicates WHERE url LIKE %term% OR title LIKE %term%, but this won't work for non ascii data. From http://www.sqlite.org/lang_expr.html "(A bug: SQLite only understands upper/lower case for 7-bit Latin characters. Hence the LIKE operator is case sensitive for 8-bit iso8859 characters or UTF-8 characters. For example, the expression 'a' LIKE 'A' is TRUE but 'æ' LIKE 'Æ' is FALSE.)." but beyond that, doing the filtering ourselves might be necessary for annotation searching on results.
I still think there is a win here, but we should measure it. note, we have i18n friendly LIKE now (thanks to sdwilsh). but if we push the search terms in sqlite, we need to make sure we handle searching tags (which dietrich added, but then backed out because of another issue) another reason to move search into sqlite (besides performance), is the limit problem (see bug #394508, found by Colin) I think this bug should morph to cover moving filtering into a sql query. dietrich / mano, what do you think?
seth: yes, i think we definitely should look into this for M9. in bug 384228, filtering by folder is moving to into FilterResultSet(), which is necessary, but may become problematic for large bookmark/tag collections. anything we can do to mitigate the size of the set passed into FilterResultSet() will help.
morphing bug. I think this will be perf issue, but actualy numbers will be necessary before checking in. setting to m9. again, this is only possible because we have i18n friendly LIKE (thanks sdwilsh!)
Assignee: nobody → sspitzer
Blocks: 380307, 394508
Depends on: 384228
Keywords: perf
Summary: FilterResultSet() results earlier, in RowToResult() → investigate if we can push filtering down into a sql query, instead of filtering at FilterResultSet()
Target Milestone: --- → Firefox 3 M9
we are doing more work in FilterResultSet() these days such as searching tags (see bug #393464) and handling LIMIT when we can't do it in SQL (see bug #394508), so if were were going to push filtering into SQL, we'd have to take this work into account. moving out to m10 to be re-evaluted then.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
clearing target milestone. more work is being done in nsNavHistory::FilterResultSet(): folder filtering, limit handling, and now tag searching. I don't know how much (if anything) we can push into a sqlite query at this point. handing back to nobody as I'm not going to investigate any time soon.
Assignee: sspitzer → nobody
Target Milestone: Firefox 3 M10 → ---
Whiteboard: [TSnappiness]
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Whiteboard: [TSnappiness] → [TSnappiness][Snappy]
:mak, is there a potential big win to be had here? (triaging "snappy" flagged bugs.)
Whiteboard: [TSnappiness][Snappy] → [Snappy]
I did this ages ago, it turns out.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
no no, this is another issue, not related to autocomplete. This is related to Places views, so the toolbar, the library, the sidebar, the menus. This change is what we need to be able to build asynchronous results, we already removed a good percentage of work from FilterResultSet() but there is still work to do, that involves removing Livemarks from bookmarks (I'm doing that), removing Tags from bookmarks (runner up) and being able to filter on searchterm (mostly done, needs some cleanup). So the responsiveness win here is conditioned to having new views, that is a long term task yet.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Whiteboard: [Snappy] → [Snappy:P4]
Thanks for remembering me this bug exists, actually I'm really near to fixing it, just matter of changing something about tags.
Depends on: 702639
Whiteboard: [Snappy:P4] → [Snappy:P3]
This is practically done for everything but tags containers. Last dependency is bug 424160, though I don't exclude there may be a quicker way to achieve the same result.
Depends on: 424160
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: REOPENED → RESOLVED
Closed: 13 years ago6 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Priority: -- → P3
Resolution: INACTIVE → ---
Severity: normal → S3

This is a valid bug, but we'd like to do a much larger refactoring of the querying code, therefore it's not worth looking into smaller-scale optimizations like this one.

Status: REOPENED → RESOLVED
Closed: 6 years ago2 years ago
Resolution: --- → WONTFIX
Whiteboard: [Snappy:P3] → [Snappy:P3][snt-scrubbed]
You need to log in before you can comment on or make changes to this bug.