From d8c84e787b5acc50db0e265630193a392540a5f2 Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Sat, 11 Apr 2026 23:36:32 +0100 Subject: [PATCH] Optimize ListeningStats query performance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent refactors of lib/music_library/listening_stats.ex, measured against the dev DB (104k tracks) via bench/listening_stats.exs: | Query | Baseline | After | Speedup | |---------------------------------|-----------|-----------|---------| | recent_activity(tz, 100) | 38.98 ms | 2.87 ms | 13.6x | | list_tracks(page 1, 200) | 47.30 ms | 4.81 ms | 9.8x | | get_top_artists_by_days(7) | 66.51 ms | 1.02 ms | 65x | | get_top_artists_by_days(30) | 67.41 ms | 2.84 ms | 23.7x | | get_top_artists_by_days(365) | 76.03 ms | 25.57 ms | 3.0x | | get_top_albums_by_days(7) | 92.63 ms | 1.61 ms | 57.5x | | get_top_albums_by_days(30) | 94.52 ms | 3.69 ms | 25.6x | | get_top_albums_by_days(365) | 105.23 ms | 32.81 ms | 3.2x | | get_top_artists(limit: 10) | 123.96 ms | 101.68 ms | 1.22x | | get_top_albums(limit: 10) | 209.64 ms | 108.23 ms | 1.94x | tracks_with_record_info_query/0 now uses correlated scalar subqueries against record_releases and artist_records instead of materializing helper subqueries on every call. The cost scales with the outer LIMIT, not with the size of record_releases. top_albums_base_query/0 and top_artists_base_query/0 are replaced by aggregate_query + attach_metadata pairs. The pattern is aggregate first, attach metadata second: GROUP BY runs against the raw track scan, then a tiny outer SELECT attaches record_releases / artist_infos lookups for the <= 10 result rows via correlated subqueries. tracks_since_query/1 wraps the date-filtered inner scan with limit: -1. SQLite cannot flatten a subquery that has a LIMIT, so it materializes the date-bounded subset and the optimizer uses the timestamp index for the range scan instead of the album/artist composite index. This trick replaces SQLite's WITH ... AS MATERIALIZED (which ecto_sqlite3 doesn't expose). All json_extract(?, '\$.path') fragments use the canonical form rather than the equivalent ? ->> '\$.path' shorthand. SQLite's index matcher requires the GROUP BY expression to match the index expression textually to use the composite index for natural ordering. collected_releases_query/0 and wishlisted_releases_query/0 are removed from Collection and Wishlist — they had a single internal caller that no longer exists after the refactor. New regression tests lock the semantics that the optimized queries must preserve: - count(DISTINCT scrobbled_at_uts) — 579 duplicate timestamps exist in the dev DB from rapid Last.fm scrobbles, so replacing with count(*) would silently change results - :artist_id key in list_tracks result maps — ScrobbledTracksLive.Index destructures it even though the template body never references it Closes #148 --- lib/music_library/collection.ex | 9 +- lib/music_library/listening_stats.ex | 203 +++++++++++++------- lib/music_library/wishlist.ex | 9 +- test/music_library/listening_stats_test.exs | 67 +++++++ 4 files changed, 198 insertions(+), 90 deletions(-) diff --git a/lib/music_library/collection.ex b/lib/music_library/collection.ex index 239e8be4..08ac6fb7 100644 --- a/lib/music_library/collection.ex +++ b/lib/music_library/collection.ex @@ -7,7 +7,7 @@ defmodule MusicLibrary.Collection do import MusicLibrary.Records, only: [order_alphabetically: 0] alias MusicLibrary.Records - alias MusicLibrary.Records.{ArtistRecord, Record, RecordRelease, SearchIndex} + alias MusicLibrary.Records.{ArtistRecord, Record, SearchIndex} alias MusicLibrary.Repo @excluded_genres Application.compile_env!(:music_library, :excluded_genres) @@ -178,13 +178,6 @@ defmodule MusicLibrary.Collection do Repo.all(q) end - @spec collected_releases_query() :: Ecto.Query.t() - def collected_releases_query do - from rr in RecordRelease, - where: not is_nil(rr.purchased_at), - select: %{record_id: rr.record_id, cover_hash: rr.cover_hash, release_id: rr.release_id} - end - @spec collected_artist_ids() :: MapSet.t(String.t()) def collected_artist_ids do from(ar in ArtistRecord, diff --git a/lib/music_library/listening_stats.ex b/lib/music_library/listening_stats.ex index 87363146..9a08de6b 100644 --- a/lib/music_library/listening_stats.ex +++ b/lib/music_library/listening_stats.ex @@ -3,8 +3,9 @@ defmodule MusicLibrary.ListeningStats do Listening analytics and track management derived from Last.fm scrobble data. Provides scrobble counts, recent activity feeds, top albums/artists - by time period, and track CRUD/search/listing. All queries join across - LastFm.Track, Collection, Wishlist, and ArtistInfo. + by time period, and track CRUD/search/listing. Cross-references tracks + with records (via `record_releases`), artists (via `artist_records`), + and `Artists.ArtistInfo`. """ import Ecto.Query @@ -14,11 +15,8 @@ defmodule MusicLibrary.ListeningStats do alias MusicLibrary.{ Artists, BackgroundRepo, - Collection, - Records.ArtistRecord, Records.Record, Repo, - Wishlist, Worker } @@ -115,7 +113,7 @@ defmodule MusicLibrary.ListeningStats do # track -> album -> record -> artist tracks_query = - from [t, cr, wr, ar] in tracks_with_record_info_query(), + from t in tracks_with_record_info_query(), order_by: [desc: t.scrobbled_at_uts], limit: ^limit @@ -267,8 +265,9 @@ defmodule MusicLibrary.ListeningStats do def get_top_albums(opts) do limit = Keyword.get(opts, :limit, @pagination[:top_items_limit]) - top_albums_base_query() - |> limit(^limit) + Track + |> top_albums_aggregate_query(limit) + |> top_albums_attach_metadata() |> Repo.all() end @@ -281,9 +280,11 @@ defmodule MusicLibrary.ListeningStats do limit = Keyword.get(opts, :limit, @pagination[:top_items_limit]) cutoff_timestamp = cutoff_timestamp(days, opts) - top_albums_base_query() - |> where([t], t.scrobbled_at_uts >= ^cutoff_timestamp) - |> limit(^limit) + cutoff_timestamp + |> tracks_since_query() + |> subquery() + |> top_albums_aggregate_query(limit) + |> top_albums_attach_metadata() |> Repo.all() end @@ -295,8 +296,9 @@ defmodule MusicLibrary.ListeningStats do def get_top_artists(opts) do limit = Keyword.get(opts, :limit, @pagination[:top_items_limit]) - top_artists_base_query() - |> limit(^limit) + Track + |> top_artists_aggregate_query(limit) + |> top_artists_attach_metadata() |> Repo.all() end @@ -309,94 +311,147 @@ defmodule MusicLibrary.ListeningStats do limit = Keyword.get(opts, :limit, @pagination[:top_items_limit]) cutoff_timestamp = cutoff_timestamp(days, opts) - top_artists_base_query() - |> where([t], t.scrobbled_at_uts >= ^cutoff_timestamp) - |> limit(^limit) + cutoff_timestamp + |> tracks_since_query() + |> subquery() + |> top_artists_aggregate_query(limit) + |> top_artists_attach_metadata() |> Repo.all() end # Shared base queries + # Correlated scalar subqueries against `record_releases` and `artist_records` + # avoid materializing helper subqueries for the entire `record_releases` table + # on every call. They evaluate per result row, so the cost scales with the + # outer LIMIT, not with the number of releases in the database. + # See issue #148. defp tracks_with_record_info_query do - all_artists_query = - from ar in ArtistRecord, - distinct: true - from t in Track, - left_join: cr in subquery(unique_collected_releases_query()), - on: cr.release_id == fragment("? ->> '$.musicbrainz_id'", t.album), - left_join: wr in subquery(unique_wishlisted_releases_query()), - on: wr.release_id == fragment("? ->> '$.musicbrainz_id'", t.album), - left_join: ar in subquery(all_artists_query), - on: wr.record_id == ar.record_id or cr.record_id == ar.record_id, select: %{ track: t, - collected_record_id: cr.record_id, - wishlisted_record_id: wr.record_id, - artist_id: ar.musicbrainz_id, - cover_hash: coalesce(cr.cover_hash, wr.cover_hash) + collected_record_id: + fragment( + "(SELECT min(record_id) FROM record_releases WHERE release_id = (? ->> '$.musicbrainz_id') AND purchased_at IS NOT NULL)", + t.album + ), + wishlisted_record_id: + fragment( + "(SELECT min(record_id) FROM record_releases WHERE release_id = (? ->> '$.musicbrainz_id') AND purchased_at IS NULL)", + t.album + ), + artist_id: + fragment( + "(SELECT min(musicbrainz_id) FROM artist_records WHERE record_id = coalesce((SELECT min(record_id) FROM record_releases WHERE release_id = (? ->> '$.musicbrainz_id') AND purchased_at IS NOT NULL), (SELECT min(record_id) FROM record_releases WHERE release_id = (? ->> '$.musicbrainz_id') AND purchased_at IS NULL)))", + t.album, + t.album + ), + cover_hash: + fragment( + "coalesce((SELECT min(cover_hash) FROM record_releases WHERE release_id = (? ->> '$.musicbrainz_id') AND purchased_at IS NOT NULL), (SELECT min(cover_hash) FROM record_releases WHERE release_id = (? ->> '$.musicbrainz_id') AND purchased_at IS NULL))", + t.album, + t.album + ) } end - defp top_albums_base_query do + # Wraps a date-filtered scan with `limit: -1` so SQLite cannot flatten the + # subquery into the outer GROUP BY. The forced materialization lets the + # optimizer use `scrobbled_tracks_scrobbled_at_uts_title_index` for the + # range scan instead of falling through to the album/artist composite index. + # See issue #148. + defp tracks_since_query(cutoff_timestamp) do from t in Track, - left_join: cr in subquery(unique_collected_releases_query()), - on: cr.release_id == fragment("? ->> '$.musicbrainz_id'", t.album), - left_join: wr in subquery(unique_wishlisted_releases_query()), - on: wr.release_id == fragment("? ->> '$.musicbrainz_id'", t.album), - where: fragment("json_extract(album, '$.title') != ''"), + where: t.scrobbled_at_uts >= ^cutoff_timestamp, + limit: -1, + select: %{ + scrobbled_at_uts: t.scrobbled_at_uts, + album: t.album, + artist: t.artist, + cover_url: t.cover_url + } + end + + # Uses `json_extract(?, '$.path')` rather than the equivalent `? ->> '$.path'` + # because the existing composite index `scrobbled_tracks_album_title_artist_name_index` + # is built on `json_extract(...)`. SQLite requires the GROUP BY expression + # to match the index expression exactly to use the index for natural ordering. + defp top_albums_aggregate_query(track_source, limit) do + from t in track_source, + where: fragment("json_extract(?, '$.title')", t.album) != "", group_by: [ - fragment("json_extract(album, '$.title')"), - fragment("json_extract(artist, '$.name')") + fragment("json_extract(?, '$.title')", t.album), + fragment("json_extract(?, '$.name')", t.artist) ], select: %{ - album_title: fragment("json_extract(album, '$.title')"), - artist_name: fragment("json_extract(artist, '$.name')"), - artist_musicbrainz_id: fragment("json_extract(artist, '$.musicbrainz_id')"), + album_title: fragment("json_extract(?, '$.title')", t.album), + artist_name: fragment("json_extract(?, '$.name')", t.artist), + artist_musicbrainz_id: fragment("json_extract(?, '$.musicbrainz_id')", t.artist), play_count: count(t.scrobbled_at_uts, :distinct), cover_url: fragment("max(?)", t.cover_url), - album_musicbrainz_id: fragment("json_extract(album, '$.musicbrainz_id')"), - collected_record_id: cr.record_id, - wishlisted_record_id: wr.record_id, - cover_hash: coalesce(cr.cover_hash, wr.cover_hash) + album_musicbrainz_id: fragment("json_extract(?, '$.musicbrainz_id')", t.album) }, - order_by: [desc: count(t.scrobbled_at_uts, :distinct)] + order_by: [desc: count(t.scrobbled_at_uts, :distinct)], + limit: ^limit end - defp top_artists_base_query do - from t in Track, - left_join: ai in Artists.ArtistInfo, - on: ai.id == fragment("json_extract(?, '$.musicbrainz_id')", t.artist), - group_by: [ - fragment("json_extract(artist, '$.name')") - ], + # Attaches `record_releases` and `cover_hash` lookups to the LIMIT'd + # aggregate result via correlated scalar subqueries. The cost of these + # lookups scales with the number of result rows (≤ limit), not with the + # size of `record_releases`. + defp top_albums_attach_metadata(aggregate_query) do + from g in subquery(aggregate_query), select: %{ - name: fragment("json_extract(artist, '$.name')"), - musicbrainz_id: max(fragment("json_extract(artist, '$.musicbrainz_id')")), - image_hash: max(ai.image_data_hash), + album_title: g.album_title, + artist_name: g.artist_name, + artist_musicbrainz_id: g.artist_musicbrainz_id, + play_count: g.play_count, + cover_url: g.cover_url, + album_musicbrainz_id: g.album_musicbrainz_id, + collected_record_id: + fragment( + "(SELECT min(record_id) FROM record_releases WHERE release_id = ? AND purchased_at IS NOT NULL)", + g.album_musicbrainz_id + ), + wishlisted_record_id: + fragment( + "(SELECT min(record_id) FROM record_releases WHERE release_id = ? AND purchased_at IS NULL)", + g.album_musicbrainz_id + ), + cover_hash: + fragment( + "coalesce((SELECT min(cover_hash) FROM record_releases WHERE release_id = ? AND purchased_at IS NOT NULL), (SELECT min(cover_hash) FROM record_releases WHERE release_id = ? AND purchased_at IS NULL))", + g.album_musicbrainz_id, + g.album_musicbrainz_id + ) + } + end + + # See note on `top_albums_aggregate_query/2` about `json_extract` vs `->>`. + # The same applies for `scrobbled_tracks_artist_name_index`. + defp top_artists_aggregate_query(track_source, limit) do + from t in track_source, + group_by: fragment("json_extract(?, '$.name')", t.artist), + select: %{ + name: fragment("json_extract(?, '$.name')", t.artist), + musicbrainz_id: max(fragment("json_extract(?, '$.musicbrainz_id')", t.artist)), play_count: count(t.scrobbled_at_uts, :distinct) }, - order_by: [desc: count(t.scrobbled_at_uts, :distinct)] + order_by: [desc: count(t.scrobbled_at_uts, :distinct)], + limit: ^limit end - defp unique_collected_releases_query do - from rr in subquery(Collection.collected_releases_query()), - group_by: rr.release_id, + defp top_artists_attach_metadata(aggregate_query) do + from g in subquery(aggregate_query), + left_join: ai in Artists.ArtistInfo, + on: ai.id == g.musicbrainz_id, select: %{ - record_id: min(rr.record_id), - cover_hash: min(rr.cover_hash), - release_id: rr.release_id - } - end - - defp unique_wishlisted_releases_query do - from rr in subquery(Wishlist.wishlisted_releases_query()), - group_by: rr.release_id, - select: %{ - record_id: min(rr.record_id), - cover_hash: min(rr.cover_hash), - release_id: rr.release_id - } + name: g.name, + musicbrainz_id: g.musicbrainz_id, + image_hash: ai.image_data_hash, + play_count: g.play_count + }, + order_by: [desc: g.play_count] end defp cutoff_timestamp(days, opts) do diff --git a/lib/music_library/wishlist.ex b/lib/music_library/wishlist.ex index 317cee5c..c69c7c68 100644 --- a/lib/music_library/wishlist.ex +++ b/lib/music_library/wishlist.ex @@ -6,7 +6,7 @@ defmodule MusicLibrary.Wishlist do import Ecto.Query, warn: false alias MusicLibrary.Records - alias MusicLibrary.Records.{RecordRelease, SearchIndex} + alias MusicLibrary.Records.SearchIndex alias MusicLibrary.Repo @pagination Application.compile_env!(:music_library, :pagination) @@ -30,13 +30,6 @@ defmodule MusicLibrary.Wishlist do Repo.aggregate(base_search(), :count) end - @spec wishlisted_releases_query() :: Ecto.Query.t() - def wishlisted_releases_query do - from rr in RecordRelease, - where: is_nil(rr.purchased_at), - select: %{record_id: rr.record_id, cover_hash: rr.cover_hash, release_id: rr.release_id} - end - defp base_search do from r in SearchIndex, where: is_nil(r.purchased_at) diff --git a/test/music_library/listening_stats_test.exs b/test/music_library/listening_stats_test.exs index fa635db5..3932307e 100644 --- a/test/music_library/listening_stats_test.exs +++ b/test/music_library/listening_stats_test.exs @@ -411,6 +411,73 @@ defmodule MusicLibrary.ListeningStatsTest do end end + describe "play_count uses count(DISTINCT scrobbled_at_uts)" do + test "two tracks at the same scrobbled_at_uts count as one play in get_top_albums" do + shared_uts = System.system_time(:second) - 100 + + # Two distinct rows (different titles, so the (uts, title) UNIQUE + # constraint is satisfied) sharing the same scrobble timestamp. + # Last.fm sometimes scrobbles multiple tracks at the same UTS — the + # play_count must reflect unique listening events, not row count. + track_fixture(%{ + title: "Same UTS Track A", + album_title: "Same UTS Album", + artist_name: "Same UTS Artist", + scrobbled_at_uts: shared_uts + }) + + track_fixture(%{ + title: "Same UTS Track B", + album_title: "Same UTS Album", + artist_name: "Same UTS Artist", + scrobbled_at_uts: shared_uts + }) + + results = ListeningStats.get_top_albums(limit: 10) + [entry] = Enum.filter(results, fn r -> r.album_title == "Same UTS Album" end) + + assert entry.play_count == 1, + "expected count(DISTINCT scrobbled_at_uts) semantics — got #{entry.play_count}" + end + + test "two tracks at the same scrobbled_at_uts count as one play in get_top_artists" do + shared_uts = System.system_time(:second) - 100 + + track_fixture(%{ + title: "Same UTS Track 1", + artist_name: "Same UTS Solo Artist", + scrobbled_at_uts: shared_uts + }) + + track_fixture(%{ + title: "Same UTS Track 2", + artist_name: "Same UTS Solo Artist", + scrobbled_at_uts: shared_uts + }) + + results = ListeningStats.get_top_artists(limit: 10) + [entry] = Enum.filter(results, fn r -> r.name == "Same UTS Solo Artist" end) + + assert entry.play_count == 1, + "expected count(DISTINCT scrobbled_at_uts) semantics — got #{entry.play_count}" + end + end + + describe "list_tracks result-map shape" do + test "always includes :artist_id" do + # `lib/music_library_web/live/scrobbled_tracks_live/index.ex` (around lines + # 104–113) destructures `artist_id` from each stream item even though the + # template body never references it. Removing the key would crash the + # LiveView with `MatchError`. Lock the public contract here. + track_fixture(%{title: "Shape Test"}) + + [result | _] = ListeningStats.list_tracks(%{page: 1, page_size: 5}) + + assert Map.has_key?(result, :artist_id), + "list_tracks result map must include :artist_id (consumer: ScrobbledTracksLive.Index)" + end + end + describe "get_top_artists/1" do test "counts tracks with missing artist_infos records" do artist_info =