ML-169.2: performed performance audit
This commit is contained in:
Claudio Ortolina
@@ -0,0 +1,262 @@
|
||||
---
|
||||
id: doc-27
|
||||
title: "Phase 4 Performance Audit: Low-Hanging Fruit Findings"
|
||||
type: specification
|
||||
created_date: "2026-05-19 07:44"
|
||||
tags:
|
||||
- audit
|
||||
- performance
|
||||
- findings
|
||||
---
|
||||
|
||||
# Phase 4 Performance Audit: Low-Hanging Fruit Findings
|
||||
|
||||
**Date**: 2026-05-19
|
||||
**Parent Task**: ML-169 (Comprehensive Application Audit)
|
||||
**Query Trace**: `/tmp/queries-ml-audit.sql`
|
||||
|
||||
## Summary
|
||||
|
||||
Audited all LiveView pages for unnecessary DB queries, redundant stream reloads,
|
||||
server round-trips that could be client-side, missing optimistic UI, and blocking
|
||||
mount work. **9 findings** across 4 severity levels.
|
||||
|
||||
---
|
||||
|
||||
## Finding 1: Display toggle (grid/list) triggers full DB reload **[HIGH]**
|
||||
|
||||
**Files**: `lib/music_library_web/live_helpers/index_actions.ex:136-143`,
|
||||
`lib/music_library_web/live/collection_live/index.ex:336`,
|
||||
`lib/music_library_web/live/wishlist_live/index.ex:259`
|
||||
|
||||
**Problem**: `handle_set_display` calls `load_and_assign_records/2` which
|
||||
re-executes the full FTS search query (`search_records_count` + `search_records`)
|
||||
when the user toggles between grid and list views. The toggle only changes the
|
||||
CSS layout class (`@display == :grid` vs `@display == :list`). No data changes.
|
||||
|
||||
**Severity**: HIGH — every display toggle wastes 2 FTS queries.
|
||||
|
||||
**Fix**: Remove `load_and_assign_records` call from `handle_set_display`.
|
||||
Only `assign(:display, mode)` is needed. The existing `:if={@display == :grid}`
|
||||
conditionals already handle showing/hiding the appropriate view.
|
||||
|
||||
```diff
|
||||
def handle_set_display(socket, mode) do
|
||||
mode = parse_mode(mode)
|
||||
{:noreply,
|
||||
socket
|
||||
|> assign(:display, mode)}
|
||||
- |> load_and_assign_records(socket.assigns.record_list_params)}
|
||||
end
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Finding 2: StatsLive.Index mount has 10 synchronous queries blocking TTFB **[HIGH]**
|
||||
|
||||
**File**: `lib/music_library_web/live/stats_live/index.ex:67-109`
|
||||
|
||||
**Problem**: The mount callback executes 10 synchronous Ecto queries before
|
||||
returning `{:ok, socket}`. This blocks the initial render (TTFB).
|
||||
|
||||
Queries executed (confirmed via QueryReporter):
|
||||
|
||||
1. `Collection.get_latest_record()` — ORDER BY purchased_at LIMIT 1
|
||||
2. `Collection.count_records_by_artist(limit: 20)` — json_each + GROUP BY
|
||||
3. `Collection.count_records_by_genre(limit: 20)` — json_each + GROUP BY + subquery exclusion
|
||||
4. `Collection.count_records_by_release_year(limit: 20)` — substr + GROUP BY
|
||||
5. `Collection.get_records_on_this_day(date)` — strftime + ORDER BY
|
||||
6. `Collection.count_records_by_format()` — GROUP BY format
|
||||
7. `Collection.count_records_by_type()` — GROUP BY type
|
||||
8. `Wishlist.count()` — FTS search count
|
||||
9. `ListeningStats.recent_activity(timezone)` — 4 correlated subqueries × 100 rows
|
||||
10. `ListeningStats.scrobble_count()` — simple COUNT
|
||||
|
||||
**Severity**: HIGH — 10 blocking queries on the home page. The `assign_async`
|
||||
pattern is already in use for TopAlbums/TopArtists on the same page — apply it
|
||||
consistently.
|
||||
|
||||
**Fix**: Use `assign_async` to defer non-critical queries. The critical ones
|
||||
(collection_count, wishlist_count, scrobble_count for the counters) can stay
|
||||
sync since they're fast. Move the heavy ones (count_records_by_artist/genre/release_year,
|
||||
get_records_on_this_day, recent_activity) to `assign_async`.
|
||||
|
||||
---
|
||||
|
||||
## Finding 3: Stream reset on every scrobble PubSub update **[HIGH]**
|
||||
|
||||
**Files**:
|
||||
|
||||
- `lib/music_library_web/live/stats_live/index.ex:576-583` (`assign_scrobble_activity`)
|
||||
- `lib/music_library_web/live/scrobbled_tracks_live/index.ex:279` (`handle_info(%{track_count: _count})`)
|
||||
- `lib/music_library_web/live/scrobbled_tracks_live/index.ex:334` (`load_and_assign_tracks`)
|
||||
|
||||
**Problem**: Every scrobble batch (cron: every 5 minutes) triggers a
|
||||
`"listening_stats:update"` PubSub message which causes both StatsLive.Index
|
||||
and ScrobbledTracksLive.Index to reload their entire stream with `reset: true`.
|
||||
StatsLive re-executes `recent_activity` (the expensive 4-correlated-subquery
|
||||
query) + `scrobble_count`. ScrobbledTracksLive re-executes the full paginated
|
||||
list query.
|
||||
|
||||
**Severity**: HIGH — fires every 5 minutes on all connected clients, even
|
||||
those who haven't interacted with the page.
|
||||
|
||||
**Fix**:
|
||||
|
||||
- For StatsLive: Use `stream_insert` for new tracks instead of `reset: true`
|
||||
- For ScrobbledTracksLive: Use `stream_insert` for new tracks only if they
|
||||
match the current search/filter; otherwise show a "new scrobbles" badge
|
||||
that refreshes on user click
|
||||
- Consider a "last seen" cursor to only insert genuinely new tracks
|
||||
|
||||
---
|
||||
|
||||
## Finding 4: Redundant stream_insert + immediate full reload **[MEDIUM]**
|
||||
|
||||
**Files**:
|
||||
|
||||
- `lib/music_library_web/live/scrobble_rules_live/index.ex:247-253` (`handle_info({Form, {:created, ...})`)
|
||||
- `lib/music_library_web/live/online_store_template_live/index.ex:195-201` (`handle_info({Form, {:saved, ...})`)
|
||||
|
||||
**Problem**: After a create/save operation, the code performs a correct
|
||||
`stream_insert` (incremental insert) but then immediately calls
|
||||
`load_and_assign_*/2` which does a full `stream(..., reset: true)` —
|
||||
completely negating the benefit of `stream_insert`.
|
||||
|
||||
The same pattern exists for delete operations in both LiveViews.
|
||||
|
||||
**Severity**: MEDIUM — wastes a database query after every create/save/delete.
|
||||
|
||||
**Fix**: Remove the `load_and_assign_*` call after `stream_insert`. The stream
|
||||
already has the correct data. For delete, `stream_delete` already removes the
|
||||
item from the stream — no full reload needed.
|
||||
|
||||
---
|
||||
|
||||
## Finding 5: Full stream resets on every search/pagination/order change **[MEDIUM]**
|
||||
|
||||
**Files**:
|
||||
|
||||
- `lib/music_library_web/live_helpers/index_actions.ex:89` (`load_and_assign_records`)
|
||||
- `lib/music_library_web/live/scrobbled_tracks_live/index.ex:334` (`load_and_assign_tracks`)
|
||||
- `lib/music_library_web/live/artist_live/show.ex:698-699` (`assign_records`)
|
||||
|
||||
**Problem**: Every search query change, order toggle, or pagination click
|
||||
triggers a full `stream(..., reset: true)`. While `reset: true` is the
|
||||
correct behavior when the entire dataset changes (new search query), it's
|
||||
unnecessary for pagination within the same search.
|
||||
|
||||
**Severity**: MEDIUM for pagination (could append/prepend), LOW for search/order
|
||||
changes (data genuinely changes).
|
||||
|
||||
**Fix for pagination**: Use `stream(..., at: -1)` (append) for "load more"
|
||||
patterns, or `reset: true` only when search/order changes, not on page change.
|
||||
|
||||
---
|
||||
|
||||
## Finding 6: Missing optimistic UI for delete operations **[MEDIUM]**
|
||||
|
||||
**Files**:
|
||||
|
||||
- `lib/music_library_web/live_helpers/index_actions.ex:94-98` (`handle_delete`)
|
||||
- `lib/music_library_web/live/collection_live/show.ex:381-385` (`handle_event("delete")`)
|
||||
- `lib/music_library_web/live/scrobbled_tracks_live/index.ex:305-310` (`handle_event("delete")`)
|
||||
|
||||
**Problem**: All delete operations wait for the database transaction to complete
|
||||
before updating the UI. `stream_delete` is already used in some places, but the
|
||||
pattern is inconsistent. CollectionLive.Show deletes don't use `stream_delete`
|
||||
at all (they navigate away).
|
||||
|
||||
**Severity**: MEDIUM — adds latency to delete operations that could feel instant.
|
||||
|
||||
**Fix**: Use `stream_delete` immediately, then perform the DB operation async.
|
||||
If DB fails, re-insert the item and show error toast.
|
||||
|
||||
```elixir
|
||||
def handle_delete(socket, id) do
|
||||
record = Records.get_record!(id)
|
||||
socket = stream_delete(socket, :records, record)
|
||||
|
||||
Task.start(fn -> Records.delete_record(record) end)
|
||||
|
||||
{:noreply, socket}
|
||||
end
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Finding 7: Correlated subqueries in ListeningStats.recent_activity **[MEDIUM]**
|
||||
|
||||
**File**: `lib/music_library/listening_stats.ex:339-397` (`tracks_with_record_info_query`)
|
||||
|
||||
**Problem**: The `tracks_with_record_info_query` includes 3 correlated subqueries
|
||||
per row (matching_records, artist_id, cover_hash), each with a nested subquery
|
||||
through `record_releases`. This runs for up to 100 tracks on every StatsLive
|
||||
mount and every scrobble update.
|
||||
|
||||
SQL captured:
|
||||
|
||||
```sql
|
||||
SELECT ...,
|
||||
(SELECT json_group_array(...) FROM records r
|
||||
WHERE r.musicbrainz_id = (
|
||||
SELECT r2.musicbrainz_id FROM records r2
|
||||
INNER JOIN record_releases rr ON rr.record_id = r2.id
|
||||
WHERE rr.release_id = (album ->> '$.musicbrainz_id') LIMIT 1
|
||||
)),
|
||||
(SELECT min(ar.musicbrainz_id) FROM artist_records ar ...),
|
||||
(SELECT r.cover_hash FROM records r ...)
|
||||
FROM scrobbled_tracks ORDER BY scrobbled_at_uts DESC LIMIT 100;
|
||||
```
|
||||
|
||||
**Severity**: MEDIUM — the 3 correlated subqueries per row (300 total for 100 tracks)
|
||||
have significant overhead on each refresh. Already mitigated by the comment
|
||||
"cost scales with number of result rows (≤ limit)" but still notable.
|
||||
|
||||
**Fix**: Consider a batch post-processing approach: fetch tracks, collect all
|
||||
unique album release IDs, batch-look up records in a single query, then merge
|
||||
in Elixir.
|
||||
|
||||
---
|
||||
|
||||
## Finding 8: Search input debounce — all verified **[PASS]**
|
||||
|
||||
**Files checked**:
|
||||
|
||||
- `lib/music_library_web/components/core_components.ex:75` — `phx-debounce="500"` (shared `search_form`)
|
||||
- `lib/music_library_web/components/add_record.ex:48` — `phx-debounce="500"`
|
||||
- `lib/music_library_web/components/scrobble_rule_picker.ex:43` — `phx-debounce="500"`
|
||||
- `lib/music_library_web/live/universal_search_live/index.ex:30` — `phx-debounce="300"`
|
||||
- `lib/music_library_web/live/record_set_live/record_picker.ex:37` — `phx-debounce="500"`
|
||||
|
||||
**Result**: All search inputs have `phx-debounce` set. ✓ No action needed.
|
||||
|
||||
---
|
||||
|
||||
## Finding 9: N+1 audit — no direct N+1 issues found **[PASS]**
|
||||
|
||||
**Files audited**: All LiveViews and LiveComponents under `lib/music_library_web/`
|
||||
|
||||
**Result**: No direct `Repo` calls found in LiveView/LiveComponent files. All
|
||||
data access goes through context modules which use paginated queries with
|
||||
limit/offset. The `assign_async` pattern is correctly used for expensive
|
||||
operations (TopByPeriod, ArtistLive similar_artists, Release component).
|
||||
|
||||
**Note**: The `ListeningStats.recent_activity` correlated subqueries (Finding 7)
|
||||
are a related performance concern but not a classic N+1.
|
||||
|
||||
---
|
||||
|
||||
## Severity Summary
|
||||
|
||||
| Finding | Severity | Impact |
|
||||
| ----------------------------------- | -------- | ------------------------------------------- |
|
||||
| #1 Display toggle FTS reload | HIGH | Every grid/list toggle wastes 2 FTS queries |
|
||||
| #2 StatsLive 10 sync queries | HIGH | Home page TTFB blocked by 10 queries |
|
||||
| #3 Scrobble reset on every batch | HIGH | Fires every 5 min on all clients |
|
||||
| #4 Redundant insert+reload | MEDIUM | Wasted queries on create/save/delete |
|
||||
| #5 Full resets on search/pagination | MEDIUM | Excessive for pagination within same search |
|
||||
| #6 Missing optimistic UI | MEDIUM | Delete/form latency |
|
||||
| #7 Correlated subqueries | MEDIUM | 300 subqueries per recent_activity call |
|
||||
| #8 Search debounce | PASS | All inputs have debounce ✓ |
|
||||
| #9 N+1 audit | PASS | No N+1 issues found ✓ |
|
||||
@@ -1,10 +1,10 @@
|
||||
---
|
||||
id: ML-169.2
|
||||
title: "Phase 4: Performance low-hanging fruit audit"
|
||||
status: To Do
|
||||
status: Done
|
||||
assignee: []
|
||||
created_date: "2026-05-08 08:59"
|
||||
updated_date: "2026-05-11 06:46"
|
||||
updated_date: "2026-05-19 07:44"
|
||||
labels:
|
||||
- audit
|
||||
- ready
|
||||
@@ -14,6 +14,10 @@ references:
|
||||
- lib/music_library_web/live_helpers/index_actions.ex
|
||||
- lib/music_library_web/live_helpers/record_actions.ex
|
||||
- .agents/skills/query-reporter/SKILL.md
|
||||
- /tmp/queries-ml-audit.sql
|
||||
- >-
|
||||
backlog/documents/doc-27 -
|
||||
Phase-4-Performance-Audit-Low-Hanging-Fruit-Findings.md
|
||||
parent_task_id: ML-169
|
||||
priority: medium
|
||||
---
|
||||
@@ -39,12 +43,36 @@ Key pre-flagged areas:
|
||||
|
||||
<!-- AC:BEGIN -->
|
||||
|
||||
- [ ] #1 SQL traces captured via QueryReporter for /collection, /collection/:id, /stats, /artists/:id
|
||||
- [ ] #2 Stream reset audit complete: every stream(:name, data, reset: true) call site evaluated for incremental-update alternatives
|
||||
- [ ] #3 Display toggle (grid/list) evaluated: confirmed whether CSS-only toggle can replace full DB reload
|
||||
- [ ] #4 StatsLive.Index mount blocking work quantified: 5 queries identified, assign_async feasibility assessed
|
||||
- [ ] #5 Search input debounce verified: phx-debounce attribute presence checked on all search forms
|
||||
- [ ] #6 Optimistic UI opportunities identified for delete, form save, toggle operations
|
||||
- [ ] #7 N+1 audit complete: handle_params, stream rendering, and LiveComponent Repo calls checked
|
||||
- [ ] #8 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations
|
||||
- [x] #1 SQL traces captured via QueryReporter for /collection, /collection/:id, /stats, /artists/:id
|
||||
- [x] #2 Stream reset audit complete: every stream(:name, data, reset: true) call site evaluated for incremental-update alternatives
|
||||
- [x] #3 Display toggle (grid/list) evaluated: confirmed whether CSS-only toggle can replace full DB reload
|
||||
- [x] #4 StatsLive.Index mount blocking work quantified: 5 queries identified, assign_async feasibility assessed
|
||||
- [x] #5 Search input debounce verified: phx-debounce attribute presence checked on all search forms
|
||||
- [x] #6 Optimistic UI opportunities identified for delete, form save, toggle operations
|
||||
- [x] #7 N+1 audit complete: handle_params, stream rendering, and LiveComponent Repo calls checked
|
||||
- [x] #8 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations
|
||||
<!-- AC:END -->
|
||||
|
||||
## Final Summary
|
||||
|
||||
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||||
|
||||
## Audit Complete: 9 findings across 4 severity levels
|
||||
|
||||
**SQL traces captured** via QueryReporter at `/tmp/queries-ml-audit.sql` for all 4 key pages (StatsLive, CollectionLive.Index, CollectionLive.Show, ArtistLive.Show).
|
||||
|
||||
### Key findings:
|
||||
|
||||
**HIGH severity (3)**:
|
||||
|
||||
1. **Display toggle triggers full DB reload** — grid/list toggle in `handle_set_display` re-executes FTS search when CSS-only toggle suffices. Fix: remove `load_and_assign_records` call.
|
||||
2. **StatsLive.Index mount: 10 sync queries** — home page TTFB blocked by 10 sequential queries. `assign_async` already used elsewhere on the same page — extend it to cover heavy queries.
|
||||
3. **Scrobble updates trigger full stream reset** — every 5-minute cron batch causes `reset: true` on all connected clients for both StatsLive and ScrobbledTracksLive.
|
||||
|
||||
**MEDIUM severity (4)**: 4. **Redundant stream_insert + immediate full reload** in ScrobbleRulesLive and OnlineStoreTemplateLive — `stream_insert` is immediately nullified by `load_and_assign_*`. 5. **Full resets on pagination** — `reset: true` on page changes when append could suffice. 6. **Missing optimistic UI** for delete operations — DB waits block UI. 7. **Correlated subqueries** in `ListeningStats.tracks_with_record_info_query` — 3 subqueries per row × 100 tracks.
|
||||
|
||||
**PASS (2)**: 8. All search inputs have `phx-debounce` set. 9. No direct N+1 issues found in LiveView/LiveComponent layer (no direct Repo calls).
|
||||
|
||||
Full report: `docs/backlog/documents/doc-27` — includes file:line references, SQL traces, and fix recommendations for each finding.
|
||||
|
||||
<!-- SECTION:FINAL_SUMMARY:END -->
|
||||
|
||||
Reference in New Issue
Block a user