1169322 - [meta] Awesomebar history search performance

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Richard Newman [:rnewman]]

We do a bunch of things sub-optimally, to put it mildly.

* We query more often than we should; I haven't checked the code, but probably once per keypress. We should debounce (wait a little while to see if another keypress comes in).

* IIRC we don't hold off on querying until we have a string of reasonable length. IIRC we use 3 on Android.

* We don't abandon queries when we kick off another. This might require SQLite blood and guts. Worst-case we have multiple queries running at once. This should never occur, and if it does it'll really kill perf.

* We do LIKE %foo% querying (over two columns, but that shouldn't matter much). That's a table walk right there. We should use FTS if we can.

* We should make sure we're not blocking the UI thread waiting for responses.

* We should run an EXPLAIN on a query logged from the app, just to ensure that the query plan is sane. (Bug 1137462 might cover this, if it's not already done.)


All of this is much more urgent now that Sync is dumping thousands of rows into the history table.

Comment 3

[Richard Newman [:rnewman]]

"it freezes at times and it starts lagging when typing…for example I’ll start typing a letter t for example and then I’ll type the letter h and it will lag a second or two before it shows up, and then when I type the letter e, the lag is now two or three seconds"

Comment 4

[OOO until Jun 29 - Dan Mosedale (:dmosedale, :dmose)]

Do we want some telemetry here?  If lots of people are experiencing problems like comment 3, it seems like it could be a substantial engagement and market-share bleed.  I've had to stop using Firefox in favor of Firefox Focus.  See bug 1417117 for the particulars of my circumstances.

Comment 5

[OOO until Jun 29 - Dan Mosedale (:dmosedale, :dmose)]

Adding a needinfo in the hopes that Stefan can shed some light on next steps here...

Comment 6

[:garvan]

Updating some original questions about this code, I see that it has debouncing, and an `inProgress` object that enables cancellation, which AFAIK addresses:

(In reply to Richard Newman [:rnewman] from comment #0)
> * We query more often than we should; I haven't checked the code, but
> probably once per keypress. We should debounce (wait a little while to see
> if another keypress comes in).
> * We don't abandon queries when we kick off another. This might require
> SQLite blood and guts. Worst-case we have multiple queries running at once.
> This should never occur, and if it does it'll really kill perf.

The patch I added for https://bugzilla.mozilla.org/show_bug.cgi?id=1417117 would address this item

(In reply to Richard Newman [:rnewman] from comment #0)
> * We should make sure we're not blocking the UI thread waiting for responses.


Richard's additional concerns are mentioned here: https://github.com/mozilla-mobile/firefox-ios/pull/3504#discussion_r153262395
Adding them to this bug:
> * I find it hard to believe that linear search as done by this completionForDomain pattern is as responsive as it could be. Surely we can compute an index?
> * Do we get better apparent responsiveness by doing this while the query is running? If we have an index of domains in memory, can we do a quick lookup on the main thread in didSet?
> * Can we build a domain index from the user's history and do the same thing?

Comment 7

[:garvan]

Attachment: Pull request

Comment 8

[:garvan]

I'll paste in the message on the github PR in Comment 7 here. 
This approach was based on profiling the SQL query and finding which part was the most costly. 
Fortunately this change accounts for 80% of the original SQL query time.

PR Comment:

Multiple views are recalculated on every keystroke, but one can assume
the database is unchanging while in the autocomplete view state.
Thus, create a temporary table when the autocomplete state is entered to
reduce the number of queries.
Also, joining result sets with favicon tables is adding 15-20% more time
to the query, and the client code never uses this data in the result
table. The client view that shows the favicons (correctly) looks up the
favicon for a site as needed.

iPhone5 queries are 2.5-3 sec per keystroke, with this patch they are
0.4 sec consistently. There will be some overhead to create the temp
table, however it is off main, and the overall perf is so much better
I don't notice any issues.

The next thing to do is to remove the favicon sql joins from the code.
I think it is a valuable cleanup task.

Comment 9

[:garvan]

Note for QA: please verify searching in the URL bar without sync, then signing into sync, waiting for history to be synced, and then searching again.

Comment 10

[:garvan]

landed on master and 10.x

Comment 11

[Simon Basca [:SimonB]]

Tested on iPhone 6 iOS 10.3 and iPad Air 2 iOS 11.2.5 on Firefox beta 10.5 (8741) 

As suggested in comment 9 tried the following scenarios: 
1. Search in the URL bar without sync
2. Sing in to sync, finish sync and search

Every search result was returned promptly. No lagginess or slow performance of the Awsomebar was observed.