Add additional indexes for address bar autocomplete

[shawnz]

This PR aims to address cause 2 identified in bug https://bugzilla.mozilla.org/show_bug.cgi?id=2056115.

In rare cases where the table stats of the places DB get stuck in a poisoned state, the lack of an index on moz_origins(host) can cause extremely poor performance. One way of mitigating this scenario is by indexing moz_origins(host).

Aside from that, while investigating this bug I also observed that indexes on moz_origins(host, frecency) and moz_places(origin_id, frecency) could help improve the performance of some queries in the address bar autocomplete path even in the absense of the poisoned table stats.

Here you can see the benchmark results on my machine, including two benchmarks which cover the poisoned-stats case (which I didn't include in the changes here since I intend to address the root cause of this in another PR):

origin_autocomplete: NO idx_origins_host_frecency
                        time:   [3.0447 ms 3.0638 ms 3.0885 ms]
origin_autocomplete: idx_origins_host_frecency
                        time:   [497.98 µs 501.11 µs 504.27 µs]
origin_url: originidindex
                        time:   [4.6239 ms 4.6674 ms 4.7581 ms]
origin_url: idx_places_origin_frecency
                        time:   [128.45 µs 130.27 µs 132.25 µs]

# I didn't include these next two in the changes, but they're here for completeness
origin_autocomplete (poisoned stats): NO idx_origins_host_frecency
                        time:   [7.6453 s 7.6775 s 7.7142 s]
origin_autocomplete (poisoned stats): idx_origins_host_frecency
                        time:   [3.4920 ms 3.5073 ms 3.5369 ms]

Observe how these changes fix the poisoned stats case but also provide a notable improvement in the healthy stats case.

I used Claude to assist with this investigation, and also this is my first contribution to a Mozilla project. So this change may warrant some scruitiny from those more experienced with the codebase. But to the best of my knowledge, it seems to be a positive change which helps in part to address the original issue I faced.

Thanks, Shawn

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • This PR follows the breaking change policy:
    • This PR has no breaking API changes, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
  • This PR only adds benchmarks -- it doesn't add unit tests, since there should be no behavioural change. Existing unit tests already cover testing the schema migration code.
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • This change doesn't include any user-facing functional changes and historically, index changes like this haven't gotten changelog entries.
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

[mhammond]

Thanks for this - I'll dig into this some more later this week.

[mhammond]

Thanks for this - I'll dig into this some more later this week.

I should say more I guess :) In particular, we copied the schema from desktop for most of places, so I'd really like to know where desktop is now for this part of the schema. Then we can maybe chat to the desktop experts like @mak77

[mak77]

It depends on how you query the database, which queries were used to benchmark?
In both cases the initial case is still < 10ms, that doesn't sound too bad.
Indices are not free, every time you add an index you duplicate all the indexed data, growing the database file size. if we'd start to super optimize every query, we'd end up putting an index on everything.

For host, one problem is we have an index on (prefix, host) but that index should have been on (host, prefix)... that's https://bugzilla.mozilla.org/show_bug.cgi?id=2025999.
origin_id has an index.
Now adding frecency to these indices surely avoids a lookup, but then why wouldn't we add indices in all the cases were we can avoid a lookup... the database would grow a lot to save some milliseconds here and there.

[shawnz]

I'd really like to know where desktop is now for this part of the schema

It doesn't have either of these indexes, but I could open a change there if there's interest

It depends on how you query the database, which queries were used to benchmark?

I found these candidates by looking at the queries in the address bar autocomplete code path, and in particular these were meant to target the URL_SQL and ORIGIN_SQL queries.

In both cases the initial case is still < 10ms, that doesn't sound too bad. Indices are not free, every time you add an index you duplicate all the indexed data, growing the database file size. if we'd start to super optimize every query, we'd end up putting an index on everything.

I agree, this change is not necessarily a clear and obvious win. The main reason for which I was even exploring adding new indexes was to work around the bad query stats degenerate case I describe in bug 2056115, but I think you can make an argument that #7485 is the "real" root cause fix for that. I only kept pursuing this in addition because it seemed to make a decent, but marginal improvement even in the non-degenerate case.

As I understand, these run on every keystroke in the address bar, so it could be worth pursuing such micro-optimizations even if small. But I will let you be the judge of that.

Now adding frecency to these indices surely avoids a lookup, but then why wouldn't we add indices in all the cases were we can avoid a lookup

Well actually, the motivation in adding frecency to the moz_places index was not to avoid a lookup, but to improve the ORDER BY performance in URL_SQL. It's not reflected as clearly in the benchmarks here, but on my real life places DB i saw a 115x improvement with this index, from 80ms to 0.7ms, even after fixing the bad table stats.

But you are right that adding frecency to the moz_origins index on the other hand merely avoids a lookup (although without frecency it's actually a slight net loss perf-wise in my testing -- my main motivation for adding the moz_origins index was to fix the degenerate table stats issue, not to improve general perf).

For host, one problem is we have an index on (prefix, host) but that index should have been on (host, prefix)... that's https://bugzilla.mozilla.org/show_bug.cgi?id=2025999.

Such a change would also prevent my degenerative table stats case. So if that's the direction you want to go, I could remove the change to moz_origins here. Although I think the change to moz_places is maybe worth considering independently.

I should note that while investigating this, I also considered several other candidates for new indexes in the address bar autocomplete path which I didn't include here because I figured they'd have diminishing returns:

• moz_places_metadata(total_view_time): this gave me a 4x improvement on the metadata QUERY_SQL query, but it was already a ~2ms query
• moz_places_metadata(updated_at): this gave me a 2.5x improvement on metadata GET_SINCE_SQL, a previously 7ms query. This actually runs 2x per keystroke, in both the SearchTerm and RecentSearch provider, but it seemed like it could cause a lot of write amplification.
• moz_inputhistory(input): adaptive history is not being used today outside of desktop, but if it were, this could be worth considering.

etc.

Thanks, Shawn

[mak77]

Your analysis is appreciated. My points were mostly the motivations behind desktop shape. Mobile has slight differences that may also make worth to take more optimizations.
It's surely an interesting idea to add frecency to compound indices for quick sorting of results, but if the primary filter of the query is good enough, then the sorting happens on a small enough subset of results, for which even a linear scan should be fine. Surely that's not a given for very short strings (1 or 2 chars), but desktop tries to make up for that, by having a delay between queries, and canceling in-flight queries egerly. Not sure on Mobile.

We'll keep these measurements into account, but at least for now Desktop won't add compund indices on frecency, we have bigger fishes to fry, to be honest.
Inverting the (prefix,host) index bug is the primary wanted fix, imo.

[shawnz-swiftly]

Makes sense to me, thank you for the insight. I believe mobile also kills in flight queries as you input additional characters.

In light of that thinking I'll close this PR and, if nobody else is taking it, perhaps try and look into opening a change to reverse the columns in that existing index on desktop and mobile

[shawnz]

I attempted to put up a change for the index column order reversal on desktop in bug 2025999. If that goes OK, then I can make the equivalent change on mobile as a follow up