Skip to content

Conversation

skhamis
Copy link
Contributor

@skhamis skhamis commented Sep 23, 2025

Building off of the work done in #6906. There was another identified query in that file that could be sped up. I tried to optimize the WHERE to be index-friendly and fixing the query itself actually did most of the heavy lifting and we're able to net some free wins by getting it to use existing indexes. So no new index + migration needed here. This turned out to be false when actually testing on android. There are semantics we need to keep for ensuring frecency shows up as expected, I did end up having to use an index and thus migration here...

Bench Stats:

117.69 µs -> 4.2667 µs = 96% speedup! No index needed as well so we don't need a migration here.
118.27 µs -> 58.014 µs = 50% speedup, not as cool as above however one of the new indexes idx_visits_place_type should speed up a few queries actually.

--- EXPLAIN QUERY PLAN Index friendly query ---
  5-0: SEARCH h USING INDEX frecencyindex (frecency>?)
  23-0: CORRELATED SCALAR SUBQUERY 1
  31-23: SEARCH v USING COVERING INDEX idx_visits_place_type (place_id=?)
--- END EXPLAIN ---

top_frecent: ORIGINAL WHERE
                        time:   [118.12 µs 118.27 µs 118.48 µs]
                        change: [−0.0176% +0.1369% +0.3010%] (p = 0.08 > 0.05)
                        No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

top_frecent: optimized_1
                        time:   [57.760 µs 58.014 µs 58.336 µs]
                        change: [−27.092% −26.685% −26.301%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

match anywhere url      time:   [354.85 ns 355.07 ns 355.34 ns]
                        change: [−1.6828% −1.3105% −0.9811%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low mild
  3 (3.00%) high mild
  6 (6.00%) high severe

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
  • 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
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

@skhamis skhamis requested review from a team and lougeniaC64 September 23, 2025 21:21
@skhamis skhamis marked this pull request as ready for review September 23, 2025 21:22
@skhamis
Copy link
Contributor Author

skhamis commented Sep 23, 2025

cc @mhammond since you also have context from the previous PR

@skhamis skhamis removed request for a team and lougeniaC64 September 24, 2025 01:02
@skhamis skhamis marked this pull request as draft September 24, 2025 01:03
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great, thanks!

@skhamis skhamis force-pushed the optimize-top-frec-query branch from 7541108 to 449b51c Compare September 24, 2025 20:21
@skhamis skhamis marked this pull request as ready for review September 25, 2025 01:18
@skhamis
Copy link
Contributor Author

skhamis commented Sep 25, 2025

@mhammond thanks for the review, I got a bit of a painful lesson in frecency (previous patch had the android test failing and how it works however I was able to still get close to the original speedup when it was in the other patch (see updated description). Since this is significant enough of a change I'm going to re-review this.

@skhamis skhamis force-pushed the optimize-top-frec-query branch from e87ce5e to b896835 Compare September 25, 2025 21:24
@skhamis skhamis added this pull request to the merge queue Sep 26, 2025
Merged via the queue into mozilla:main with commit 8499cbd Sep 26, 2025
13 checks passed
@skhamis skhamis deleted the optimize-top-frec-query branch September 26, 2025 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants