perf(library): patch what changed instead of reloading the list
A retag named the one file it rewrote and the store threw the whole list away anyway: 20.5 MB across the IPC at 26 138 tracks to pick up one new title. A soft rescan of an unchanged library did the same, though its metrics say zero added, zero updated and zero removed. - `TrackMetadataChanged` with a `filePath` re-reads that one row through `trackCache` and splices it in, replacing the array so the memoized filter and sort caches notice. The summaries are still refetched: a retag can move a track between albums. A failed patch falls back to the full reload, because the answer that cannot be wrong is the one to keep. - A batch write names no paths (it can be thousands) and still reloads. - `LibraryScanComplete` reads its own metrics and reloads only when the scan changed something; a payload of an unknown shape is treated as a change, so nothing can leave a stale list on screen. Closes #282
This commit is contained in:
1 parent
bce8d27370
commit
5b236ca5af
2 files changed
+205
-5
No files matched your search
@@ -9,6 +9,7 @@ import {
|
||||
} from '@go/library/library.js';
|
||||
import type * as library from '@go/library/models.js';
|
||||
import { list } from '@utils/binding';
|
||||
import { trackCache } from './track-cache';
|
||||
import { decodeTrackTable, type ListTrack } from '@utils/track-table';
|
||||
import { Events } from '../events';
|
||||
|
||||
@@ -90,8 +91,8 @@ class LibraryStore {
|
||||
private cacheGen = 0;
|
||||
|
||||
constructor() {
|
||||
EventsOn(Events.LibraryScanComplete, () => {
|
||||
this.invalidate();
|
||||
EventsOn(Events.LibraryScanComplete, (payload: unknown) => {
|
||||
this.applyScanComplete(payload);
|
||||
});
|
||||
EventsOn(Events.LibraryRemoved, () => {
|
||||
this.libraries = null;
|
||||
@@ -107,8 +108,8 @@ class LibraryStore {
|
||||
this.changeGen++;
|
||||
this.notify();
|
||||
});
|
||||
EventsOn(Events.TrackMetadataChanged, () => {
|
||||
this.invalidate();
|
||||
EventsOn(Events.TrackMetadataChanged, (payload: unknown) => {
|
||||
this.applyMetadataChanged(payload);
|
||||
});
|
||||
EventsOn(Events.TrackPlayCountChanged, (payload: unknown) => {
|
||||
this.applyPlayCount(payload);
|
||||
@@ -554,6 +555,116 @@ class LibraryStore {
|
||||
// INVALIDATION
|
||||
// ===================================================================
|
||||
|
||||
/**
|
||||
* A scan finished. Reload only if it changed something (#282).
|
||||
*
|
||||
* `ScanMetrics` says how many files were added, updated and removed,
|
||||
* and a soft rescan of an unchanged library reports zero of each —
|
||||
* yet every loaded collection was thrown away and refetched, which
|
||||
* at 26 138 tracks is 20.5 MB across the IPC for a scan that found
|
||||
* nothing. Anything non-zero still reloads everything: a scan can
|
||||
* change a tag, an album name or a genre on any file it touched, and
|
||||
* it reports counts rather than paths.
|
||||
*
|
||||
* A payload that says nothing at all is treated as a change, not as
|
||||
* a no-op: an unknown shape must not be able to leave a stale list
|
||||
* on screen.
|
||||
*/
|
||||
private applyScanComplete(payload: unknown): void {
|
||||
const m = payload as {
|
||||
added?: number;
|
||||
updated?: number;
|
||||
removed?: number;
|
||||
} | null;
|
||||
|
||||
if (!m) {
|
||||
this.invalidate();
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
const changed = (m.added ?? 0) + (m.updated ?? 0) + (m.removed ?? 0);
|
||||
|
||||
if (changed === 0) return;
|
||||
|
||||
this.invalidate();
|
||||
}
|
||||
|
||||
/**
|
||||
* Tags were rewritten on disk. A single file is patched; a batch is
|
||||
* a reload (#282).
|
||||
*
|
||||
* The event names the one file it rewrote, and that file's row is
|
||||
* the only row that changed — so re-reading the whole list to pick
|
||||
* up one new title is 20.5 MB at 26 138 tracks. A batch write
|
||||
* carries no paths (it can be thousands of files), and the summaries
|
||||
* really do change with it, so that one still reloads.
|
||||
*/
|
||||
private applyMetadataChanged(payload: unknown): void {
|
||||
const p = payload as { filePath?: string; batch?: boolean } | null;
|
||||
|
||||
if (!p?.filePath || p.batch) {
|
||||
this.invalidate();
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
this.patchOneTrack(p.filePath);
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-read one file's row and splice it in.
|
||||
*
|
||||
* The summaries are dropped and refetched, because a retag can move
|
||||
* a track between albums and change a genre — they are the small
|
||||
* collections, and they are what makes the album and artist views
|
||||
* agree with the row that was just patched.
|
||||
*/
|
||||
private patchOneTrack(filePath: string): void {
|
||||
const loaded = this.loadedCollections();
|
||||
|
||||
void trackCache
|
||||
.refresh([filePath])
|
||||
.then(([fresh]) => {
|
||||
if (!fresh || this.tracks === null) return;
|
||||
|
||||
const idx = this.tracks.findIndex(
|
||||
(t) => t.FilePath === filePath,
|
||||
);
|
||||
|
||||
// Not in this view's list (another library's file, or
|
||||
// gone): the list is right as it stands.
|
||||
if (idx === -1) return;
|
||||
|
||||
this.tracks = [
|
||||
...this.tracks.slice(0, idx),
|
||||
fresh,
|
||||
...this.tracks.slice(idx + 1),
|
||||
];
|
||||
this.changeGen++;
|
||||
this.notify();
|
||||
})
|
||||
.catch((err: unknown) => {
|
||||
// The patch failed, so the list may now be stale. Fall
|
||||
// back to the answer that cannot be wrong.
|
||||
console.error('library: could not re-read a retagged track', err);
|
||||
this.invalidate();
|
||||
});
|
||||
|
||||
this.albums = null;
|
||||
this.artists = null;
|
||||
this.genres = null;
|
||||
this.cacheGen++;
|
||||
this.inFlight.delete('albums');
|
||||
this.inFlight.delete('artists');
|
||||
this.inFlight.delete('genres');
|
||||
this.refetchLoaded({
|
||||
albums: loaded.albums,
|
||||
artists: loaded.artists,
|
||||
genres: loaded.genres,
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Patch one track's play statistics in place.
|
||||
*
|
||||
|
||||
@@ -21,6 +21,12 @@ import {
|
||||
} from '@test/support/harness';
|
||||
import { trackTable } from '@test/support/track-table';
|
||||
|
||||
const RETAGGED = {
|
||||
TrackName: 'Retagged',
|
||||
FilePath: '/a.mp3',
|
||||
PlayCount: 0,
|
||||
};
|
||||
|
||||
const TRACKS = [
|
||||
{ TrackName: 'One', FilePath: '/a.mp3', PlayCount: 0 },
|
||||
{ TrackName: 'Two', FilePath: '/b.mp3', PlayCount: 4 },
|
||||
@@ -112,10 +118,93 @@ describe('library store: caching', () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it('refetches when a track is retagged', async () => {
|
||||
/*
|
||||
* #282: a retag names the one file it rewrote, so the whole list does
|
||||
* not have to come back — 20.5 MB at 26 138 tracks to pick up one new
|
||||
* title. The summaries are still refetched, because a retag can move
|
||||
* a track between albums.
|
||||
*/
|
||||
it('re-reads the one file a retag names, not the whole list', async () => {
|
||||
stub('library.Library.GetTracksByPaths', [RETAGGED]);
|
||||
|
||||
emit(Events.TrackMetadataChanged, { filePath: '/a.mp3' });
|
||||
await flush();
|
||||
|
||||
expect(calls('library.Library.GetTrackTable')).toHaveLength(0);
|
||||
expect(lastArgs('library.Library.GetTracksByPaths')).toEqual([['/a.mp3']]);
|
||||
expect(calls().map((c) => c.path).sort()).toEqual([
|
||||
'library.Library.GetAlbums',
|
||||
'library.Library.GetArtists',
|
||||
'library.Library.GetGenres',
|
||||
'library.Library.GetTracksByPaths',
|
||||
]);
|
||||
});
|
||||
|
||||
it('splices the re-read row in, so consumers notice', async () => {
|
||||
stub('library.Library.GetTracksByPaths', [RETAGGED]);
|
||||
|
||||
const before = libraryStore.getCachedTracks();
|
||||
|
||||
emit(Events.TrackMetadataChanged, { filePath: '/a.mp3' });
|
||||
await flush();
|
||||
|
||||
const after = libraryStore.getCachedTracks();
|
||||
|
||||
expect(after?.map((t) => t.TrackName)).toEqual(['Retagged', 'Two']);
|
||||
expect(after, 'a new array, which is what memoized consumers key on').not.toBe(
|
||||
before,
|
||||
);
|
||||
});
|
||||
|
||||
it('falls back to a full reload when the patch cannot be read', async () => {
|
||||
stubFailure('library.Library.GetTracksByPaths', 'sql: database is locked');
|
||||
|
||||
emit(Events.TrackMetadataChanged, { filePath: '/a.mp3' });
|
||||
await flush();
|
||||
|
||||
expect(calls('library.Library.GetTrackTable')).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('reloads everything for a batch write, which names no paths', async () => {
|
||||
emit(Events.TrackMetadataChanged, { batch: true, total: 40 });
|
||||
await flush();
|
||||
|
||||
expect(calls('library.Library.GetTrackTable')).toHaveLength(1);
|
||||
expect(calls('library.Library.GetTracksByPaths')).toHaveLength(0);
|
||||
});
|
||||
|
||||
/*
|
||||
* #282, the other half: a soft rescan of an unchanged library reports
|
||||
* zero added, updated and removed, and every loaded collection was
|
||||
* thrown away and refetched anyway.
|
||||
*/
|
||||
describe('a scan that changed nothing', () => {
|
||||
beforeEach(async () => {
|
||||
emit(Events.LibraryScanComplete, {
|
||||
added: 0,
|
||||
updated: 0,
|
||||
removed: 0,
|
||||
skipped: 31,
|
||||
});
|
||||
await flush();
|
||||
});
|
||||
|
||||
it('refetches nothing', () => {
|
||||
expect(calls()).toEqual([]);
|
||||
});
|
||||
|
||||
it('keeps the data it already had', () => {
|
||||
expect(libraryStore.getCachedTracks()?.map((t) => t.FilePath)).toEqual([
|
||||
'/a.mp3',
|
||||
'/b.mp3',
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
it('reloads when a scan reports a change', async () => {
|
||||
emit(Events.LibraryScanComplete, { added: 1, updated: 0, removed: 0 });
|
||||
await flush();
|
||||
|
||||
expect(calls('library.Library.GetTrackTable')).toHaveLength(1);
|
||||
});
|
||||
|
||||
|
||||
Reference in new issue
Block a user