perf(library): hand the batch dialog the rows the view already has
Fetching whole tracks back for a dialog that reads only fields the rows in hand already carry cost 3 s and 700 ms of blocked main thread on a "select all" over 50 000 tracks — the regression #281 introduced by dropping four fields from the list. `showBatchTrackDetails` takes the rows; the Tracks view passes its own, and the views that hold no library rows (Explore, the queue, playlists) keep the path form over `trackCache`. The batch view's merged-fields extractors read only list fields already, and a cover tier the list does not carry falls back to the one it does. Measured on 50 000 tracks: 50.4 ms, from 87.9 ms before #281. Refs #281
This commit is contained in:
1 parent
7a1412383f
commit
3d9828b847
3 files changed
+70
-34
No files matched your search
@@ -24,6 +24,7 @@ type TrackMBIDs = library.TrackMBIDs;
|
|||||||
import { ImageFilePicker, ReadFile } from '@go/frontendutil/frontendutil.js';
|
import { ImageFilePicker, ReadFile } from '@go/frontendutil/frontendutil.js';
|
||||||
import { trackCache } from '../../store/track-cache';
|
import { trackCache } from '../../store/track-cache';
|
||||||
import { batchCoverArt } from '@utils/track-details-opener.js';
|
import { batchCoverArt } from '@utils/track-details-opener.js';
|
||||||
|
import type { ListTrack } from '@utils/track-table';
|
||||||
import { EventsOn } from '@runtime/runtime';
|
import { EventsOn } from '@runtime/runtime';
|
||||||
import { Events } from '../../events';
|
import { Events } from '../../events';
|
||||||
|
|
||||||
@@ -88,7 +89,11 @@ export class TrackDetails extends LitElement {
|
|||||||
|
|
||||||
// -- Batch-specific state --
|
// -- Batch-specific state --
|
||||||
@state() private batchMode = false;
|
@state() private batchMode = false;
|
||||||
@state() private batchTracks: library.Track[] = [];
|
// `ListTrack`, not `Track`: every field the merged-values view reads
|
||||||
|
// is on the list's rows, so a caller that is already showing them
|
||||||
|
// can open this dialog without fetching anything (#281). The
|
||||||
|
// openers that hold no rows ask `trackCache` instead.
|
||||||
|
@state() private batchTracks: ListTrack[] = [];
|
||||||
@state() private batchFilePaths: string[] = [];
|
@state() private batchFilePaths: string[] = [];
|
||||||
@state() private batchCoverArtMixed = false;
|
@state() private batchCoverArtMixed = false;
|
||||||
@state() private batchProgress: {
|
@state() private batchProgress: {
|
||||||
@@ -136,12 +141,12 @@ export class TrackDetails extends LitElement {
|
|||||||
|
|
||||||
/** Open the dialog for batch editing multiple tracks. */
|
/** Open the dialog for batch editing multiple tracks. */
|
||||||
showBatch(
|
showBatch(
|
||||||
tracks: library.Track[],
|
tracks: readonly ListTrack[],
|
||||||
coverArt: CoverArtUrls | null,
|
coverArt: CoverArtUrls | null,
|
||||||
coverArtMixed: boolean,
|
coverArtMixed: boolean,
|
||||||
): void {
|
): void {
|
||||||
this.batchMode = true;
|
this.batchMode = true;
|
||||||
this.batchTracks = tracks;
|
this.batchTracks = [...tracks];
|
||||||
this.batchFilePaths = tracks.map(
|
this.batchFilePaths = tracks.map(
|
||||||
(t) => t.FilePath,
|
(t) => t.FilePath,
|
||||||
);
|
);
|
||||||
@@ -2133,7 +2138,7 @@ export class TrackDetails extends LitElement {
|
|||||||
key: string;
|
key: string;
|
||||||
label: string;
|
label: string;
|
||||||
type: 'text' | 'number';
|
type: 'text' | 'number';
|
||||||
extract: (t: library.Track) => string;
|
extract: (t: ListTrack) => string;
|
||||||
}> = [
|
}> = [
|
||||||
{
|
{
|
||||||
key: 'title',
|
key: 'title',
|
||||||
@@ -2217,7 +2222,7 @@ export class TrackDetails extends LitElement {
|
|||||||
private countDistinctValues(key: string): number {
|
private countDistinctValues(key: string): number {
|
||||||
const extractMap: Record<
|
const extractMap: Record<
|
||||||
string,
|
string,
|
||||||
(t: library.Track) => string
|
(t: ListTrack) => string
|
||||||
> = {
|
> = {
|
||||||
title: (t) => t.TrackName ?? '',
|
title: (t) => t.TrackName ?? '',
|
||||||
artist: (t) => t.ArtistName ?? '',
|
artist: (t) => t.ArtistName ?? '',
|
||||||
@@ -2241,7 +2246,7 @@ export class TrackDetails extends LitElement {
|
|||||||
if (!extract) return 0;
|
if (!extract) return 0;
|
||||||
|
|
||||||
const unique = new Set(
|
const unique = new Set(
|
||||||
this.batchTracks.map(extract),
|
this.batchTracks.map((t) => extract(t)),
|
||||||
);
|
);
|
||||||
|
|
||||||
return unique.size;
|
return unique.size;
|
||||||
|
|||||||
@@ -80,8 +80,8 @@ import { describeError } from '@utils/describe-error';
|
|||||||
import { notificationStore } from '@store/notification-store';
|
import { notificationStore } from '@store/notification-store';
|
||||||
import { confirmAction } from '@components/confirm-dialog/confirm-dialog';
|
import { confirmAction } from '@components/confirm-dialog/confirm-dialog';
|
||||||
import { RemoveFromLibrary } from '@go/library/library.js';
|
import { RemoveFromLibrary } from '@go/library/library.js';
|
||||||
import { showTrackDetailsForPath, showBatchTrackDetailsForPaths } from '@utils/track-details-opener.js';
|
import { showTrackDetailsForPath, showBatchTrackDetails } from '@utils/track-details-opener.js';
|
||||||
import { tracksByFilePath } from '@utils/track-index.js';
|
import { tracksByFilePath, tracksForPaths } from '@utils/track-index.js';
|
||||||
import '@components/playlist-picker/playlist-picker.js';
|
import '@components/playlist-picker/playlist-picker.js';
|
||||||
import type { TrackDetails } from '@components/track-details/track-details.js';
|
import type { TrackDetails } from '@components/track-details/track-details.js';
|
||||||
import {
|
import {
|
||||||
@@ -2141,9 +2141,11 @@ export class TrackList
|
|||||||
private async openBatchTrackDetails(
|
private async openBatchTrackDetails(
|
||||||
filePaths: string[],
|
filePaths: string[],
|
||||||
) {
|
) {
|
||||||
await showBatchTrackDetailsForPaths(
|
// The rows in hand are what the dialog reads, so a "select all"
|
||||||
|
// on 50 000 tracks opens it without asking the backend for them.
|
||||||
|
await showBatchTrackDetails(
|
||||||
() => this.trackDetailsDialog,
|
() => this.trackDetailsDialog,
|
||||||
filePaths,
|
tracksForPaths(this.tracks, filePaths),
|
||||||
() => void this.openBatchTrackDetails(filePaths),
|
() => void this.openBatchTrackDetails(filePaths),
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,40 +1,54 @@
|
|||||||
/**
|
/**
|
||||||
* Open `<track-details>` for a file path.
|
* Open `<track-details>` for a file path.
|
||||||
*
|
*
|
||||||
* The five library-side hosts already hold the `library.Track` the
|
* Explore's rows carry no library metadata at all: a tracklist row is
|
||||||
* dialog wants — they render it. Explore's rows do not: a tracklist row
|
* an `MBTrack`/`LBTopRecording` from the catalog, and all it can say
|
||||||
* is an `MBTrack`/`LBTopRecording` from the catalog, and all it can say
|
|
||||||
* about the library is *which file is behind it*. So the path is the
|
* about the library is *which file is behind it*. So the path is the
|
||||||
* one key both sides share, and turning it back into a track is the
|
* one key both sides share, and turning it back into a track is the
|
||||||
* work this does.
|
* work this does. The queue, playlists and smart playlists are the same
|
||||||
|
* shape — they render their own row type.
|
||||||
*
|
*
|
||||||
* The queue, playlists and smart playlists are in the same position:
|
* All of them used to find the track in `libraryStore`'s whole-library
|
||||||
* they render their own row type, not a `library.Track`. All of them
|
* array, which meant that array had to be loaded for details to open —
|
||||||
* used to find the track in `libraryStore`'s whole-library array, which
|
* and the ones that read it synchronously silently did nothing when it
|
||||||
* meant that array had to be loaded for details to open — and the ones
|
* was not (#279). They ask `trackCache` for the paths in hand instead:
|
||||||
* that read it synchronously silently did nothing when it was not
|
* one small call, coalesced, answered from cache the second time.
|
||||||
* (#279). They ask `trackCache` for the paths in hand instead: one
|
*
|
||||||
* small call, coalesced, answered from cache the second time.
|
* A caller that *is* showing the rows passes them
|
||||||
|
* (`showBatchTrackDetails`), because fetching back what is already in
|
||||||
|
* hand is how a "select all" over 50 000 tracks came to cost 3 s.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import type * as library from '@go/library/models.js';
|
|
||||||
import type {
|
import type {
|
||||||
CoverArtUrls,
|
CoverArtUrls,
|
||||||
TrackDetails,
|
TrackDetails,
|
||||||
} from '@components/track-details/track-details.js';
|
} from '@components/track-details/track-details.js';
|
||||||
import { trackCache } from '@store/track-cache.js';
|
import { trackCache } from '@store/track-cache.js';
|
||||||
import { loadTrackDetails } from '@utils/lazy-track-details.js';
|
import { loadTrackDetails } from '@utils/lazy-track-details.js';
|
||||||
|
import type * as library from '@go/library/models.js';
|
||||||
|
import type { ListTrack } from '@utils/track-table';
|
||||||
|
|
||||||
/** The cover art the dialog shows, or nothing when the track has none. */
|
/**
|
||||||
function coverArtOf(track: library.Track): CoverArtUrls | undefined {
|
* The cover art the dialog shows, or nothing when the track has none.
|
||||||
return track.CoverArtPath
|
*
|
||||||
? {
|
* A whole track carries every tier; the list's rows carry only the
|
||||||
coverArtPath: track.CoverArtPath,
|
* small one (#281), and every tier is derived from the same file — so a
|
||||||
coverArtSmall: track.CoverArtSmall,
|
* missing one falls back to the tier that is there rather than the
|
||||||
coverArtMedium: track.CoverArtMedium,
|
* dialog showing nothing.
|
||||||
coverArtLarge: track.CoverArtLarge,
|
*/
|
||||||
}
|
function coverArtOf(track: ListTrack): CoverArtUrls | undefined {
|
||||||
: undefined;
|
const small = track.CoverArtSmall;
|
||||||
|
|
||||||
|
if (!small) return undefined;
|
||||||
|
|
||||||
|
const tiers = track as Partial<library.Track>;
|
||||||
|
|
||||||
|
return {
|
||||||
|
coverArtPath: tiers.CoverArtPath ?? small,
|
||||||
|
coverArtSmall: small,
|
||||||
|
coverArtMedium: tiers.CoverArtMedium ?? small,
|
||||||
|
coverArtLarge: tiers.CoverArtLarge ?? small,
|
||||||
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -42,7 +56,7 @@ function coverArtOf(track: library.Track): CoverArtUrls | undefined {
|
|||||||
* one album; none and `mixed` when they span several.
|
* one album; none and `mixed` when they span several.
|
||||||
*/
|
*/
|
||||||
export function batchCoverArt(
|
export function batchCoverArt(
|
||||||
tracks: readonly library.Track[],
|
tracks: readonly ListTrack[],
|
||||||
): { coverArt: CoverArtUrls | null; mixed: boolean } {
|
): { coverArt: CoverArtUrls | null; mixed: boolean } {
|
||||||
const albums = new Set(tracks.map((t) => t.Album));
|
const albums = new Set(tracks.map((t) => t.Album));
|
||||||
|
|
||||||
@@ -101,8 +115,23 @@ export async function showBatchTrackDetailsForPaths(
|
|||||||
filePaths: readonly string[],
|
filePaths: readonly string[],
|
||||||
retry: () => void,
|
retry: () => void,
|
||||||
): Promise<TrackDetailsOutcome> {
|
): Promise<TrackDetailsOutcome> {
|
||||||
const tracks = await trackCache.get(filePaths);
|
return showBatchTrackDetails(dialog, await trackCache.get(filePaths), retry);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Show the batch details dialog for tracks the caller already holds.
|
||||||
|
*
|
||||||
|
* The Tracks view is why this exists rather than only the path form: a
|
||||||
|
* selection there can be the whole library, and fetching every track of
|
||||||
|
* it back to read fields the rows in hand already carry cost 3 s and
|
||||||
|
* 700 ms of blocked main thread on 50 000 tracks (#281's own regression,
|
||||||
|
* measured).
|
||||||
|
*/
|
||||||
|
export async function showBatchTrackDetails(
|
||||||
|
dialog: () => TrackDetails | undefined,
|
||||||
|
tracks: readonly ListTrack[],
|
||||||
|
retry: () => void,
|
||||||
|
): Promise<TrackDetailsOutcome> {
|
||||||
if (tracks.length === 0) return 'not-in-library';
|
if (tracks.length === 0) return 'not-in-library';
|
||||||
|
|
||||||
const ready = await loadTrackDetails(retry);
|
const ready = await loadTrackDetails(retry);
|
||||||
|
|||||||
Reference in new issue
Block a user