perf(local): only re-read the files that changed (#411) - #619
Draft
TheZupZup wants to merge 3 commits into
Draft
Conversation
A Linux local scan spends nearly all of its time opening files and parsing
their metadata blocks. On a routine rescan almost none of those files changed,
and it parsed every one of them anyway.
Now each candidate is stat-ed for its size and mtime, which is one syscall. If
the catalog already holds that path with an identical stamp, the stored track
is reused verbatim and the file is never opened. Only new files and changed
files are parsed.
On a synthetic library with a fixed 200 microsecond per-file parse cost
(test/benchmarks/local_incremental_scan_bench.dart, numbers from this machine):
files 2000 full 635 ms rescan 55 ms parsed 2000 -> 0
files 20000 full 4803 ms rescan 151 ms parsed 20000 -> 0
files 100000 full 23327 ms rescan 655 ms parsed 100000 -> 0
Modules:
* lib/core/models/local_file_stamp.dart is the stamp (size + mtime) and the
StampedTrack pairing. It documents what size and mtime miss, because that is
what the full-rescan path exists for.
* lib/core/sources/local/local_file_stat.dart is the stat seam, beside the
scanner and metadata-reader seams and for the same reason. The unsupported
implementation answers nothing, so every file is parsed: that is what Android
gets (its local library comes from the content resolver, so there is nothing
to stat) and what any caller that has not opted in gets.
* lib/core/repositories/stamped_catalog_writer.dart is the optional repository
capability, in the same shape as SourceCatalogReader and
IncrementalCatalogWriter. Falling back to a plain upsert is always correct
and only slower, which is what lets this be optional at all.
* LocalMusicSource does the comparison; LocalLibraryScanner carries stamps
through the merge, including for retained tracks (an offline folder keeps
its stamps, so plugging the drive back in does not re-parse it).
Schema v5 adds two nullable columns to `tracks`. They are on the row rather
than in a side table on purpose: the row *is* the record of "this path was
parsed into this track", so one write and one transaction, and there is no
window where a stamp says "parsed" for a track the catalog does not have.
A sidecar index would avoid the migration and buy exactly that hazard. The
migration is purely additive: existing rows keep every value, read back with
null stamps, and a null stamp means "parse it", which is the pre-v5 behaviour.
Nothing to backfill, nothing lost if the upgrade is interrupted.
Recovery is preserved. scanFolders(full: true) ignores every stamp and rebuilds
the slice from the files themselves, through the same code path (it hands the
scan an empty index rather than taking a second route, so the two cannot
drift). Forgetting the source and re-selecting does the same thing, since there
is then nothing indexed to compare against.
The tests count parser calls, not results. Asserting that two scans produce the
same catalog proves nothing about whether the second one re-read everything,
which is the entire point: the catalog was already correct, it was just
expensive to arrive at. There are round-trip tests over a real in-memory SQLite
database too, because a stamp that does not survive the schema is not a stamp.
Two existing suites needed a real fix. The deferred-scanner concurrency tests
assumed the walk had already started when a scan future was created, which
stopped being true once the scan reads the indexed slice first; they now wait
for the walk to genuinely start before superseding it, which is what they meant
to assert. And the Library screen widget test now stubs the stat seam for the
same documented reason it already stubs the tag reader: real filesystem I/O on
fictional paths never completes against pumpAndSettle's fake clock.
Tests: 52 new, 4933 passing overall.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lq4ZJtwtdVCMBuZLjFbyfV
Merges main, which brought in tool/linux_local_library_smoke.dart (#447, landed as #610). That tool calls LocalLibraryScanner directly, and this PR changed what the scanner hands back: LocalLibraryScan.tracks is now a list of StampedTrack rather than Track, so the smoke stopped compiling the moment the two met. Nothing conflicted textually, which is why it took a merge to notice. The smoke's assertions are all about the tracks, not about where their bytes were, so they read LocalLibraryScan.plainTracks. The one previously-indexed row it feeds to retention now arrives as a StampedTrack with no stamp, which is exactly the shape of a row written before schema v5 and the case retention has to keep working for. The tool still passes no stat reader, so it parses every file, and what it proves about a granted Flatpak folder is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lq4ZJtwtdVCMBuZLjFbyfV
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #411. Part of #376.
Second of four PRs on Linux local-library reliability. Independent of the others, based on latest
main.What changes
A local scan spends nearly all of its time opening files and parsing their metadata blocks. On a routine rescan almost none of those files changed, and it parsed every one of them anyway.
Now each candidate file is
stat-ed for its size and last-modified time, which is one syscall. If the catalog already holds that path with an identical stamp, the stored track is reused verbatim and the file is never opened. Only new files and changed files get parsed.Numbers
test/benchmarks/local_incremental_scan_bench.dart, on a synthetic tree with a fixed 200 microsecond per-file parse cost (this machine, so treat these as shape rather than truth):The parse cost is pinned rather than measured on purpose: a real tag read is dominated by I/O that varies by an order of magnitude between an SSD and a spinning USB drive, and by page cache state, so pinning it is what makes the two columns comparable. 200 microseconds is well under a cold FLAC tag read, so the saving shown is a floor.
Files
lib/core/models/local_file_stamp.dartis the stamp (size + mtime) and theStampedTrackpairing. It documents what size and mtime miss, because that is what the full-rescan path exists for.lib/core/sources/local/local_file_stat.dartis the stat seam, beside the scanner and metadata-reader seams and for the same reason. The unsupported implementation answers nothing, so every file is parsed: that is what Android gets (its local library comes from the content resolver, so there is nothing tostat), and what any caller that has not opted in gets.lib/core/repositories/stamped_catalog_writer.dartis the optional repository capability, in the same shape asSourceCatalogReaderandIncrementalCatalogWriter. Falling back to a plain upsert is always correct and only slower, which is the property that lets it be optional.LocalMusicSourcedoes the comparison.LocalLibraryScannercarries stamps through the merge, including for retained tracks: an offline folder keeps its stamps, so plugging the drive back in does not re-parse it.The schema migration, and why
Schema v5 adds two nullable columns to
tracks:file_size_bytesandfile_modified_at_ms.The issue asks not to migrate without clear value, so here is the case. The stamp has to live somewhere that cannot disagree with the catalog. On the row, it is one write in one transaction, and there is no window in which a stamp says "already parsed" for a track the catalog does not actually have. A sidecar store would avoid the migration and buy exactly that hazard: an interrupted or partially-restored index would make a later scan skip files it never indexed, and the symptom would be a silently short library.
It is purely additive. Both columns are nullable with no default, so SQLite's
ALTER TABLE … ADD COLUMNrewrites no rows: existing rows keep every value and read back with null stamps, and a null stamp means "parse this file", i.e. exactly the pre-v5 behaviour. Nothing to backfill, and nothing lost if the upgrade is interrupted. Tests cover v4 → v5, v2 → v5 in one go, and a fresh install.Recovery
scanFolders(full: true)ignores every stamp and rebuilds the slice from the files themselves. It goes through the same code path (it hands the scan an empty index rather than taking a second route) so the two cannot drift apart. Forgetting the source and re-selecting the folders does the same thing, since there is then nothing indexed to compare against. A full rescan still records fresh stamps, so recovery does not cost a full parse on the next scan too.Tests
They count parser calls, not results. Asserting that two scans produce the same catalog proves nothing about whether the second one re-read every file, which is the entire point: the catalog was already correct before this change, it was just expensive to arrive at.
test/core/sources/local/local_incremental_scan_test.dart: a second scan of an unchanged library reads zero tags and stats each file once; a moved mtime, a changed size with an unchanged mtime, a new file, and a file that cannot be stat-ed each re-parse and nothing else does; a full rescan re-parses everything and still records fresh stamps; no stat reader means the old behaviour.test/features/library/incremental_library_scan_test.dart: the same through the real Drift repository over in-memory SQLite, because a stamp that does not survive the schema is not a stamp. Covers multiple roots, a changed file in one root not re-parsing the other, and a drive going offline and coming back without re-parsing.test/data/database/linthra_database_migration_test.dart: the v5 migration cases above.Real-device / real-filesystem limits
Documented in
docs/local-music.md:touch -rafter an in-place edit. Hashing would catch those, at the cost of reading every byte of the library on every scan, which is the thing being avoided. The full rescan is the answer.FileStatreports portably), so two writes to one file inside the same millisecond that leave the size unchanged look identical. Vanishingly unlikely for audio files, and the full rescan covers it.Checks
dart format,flutter analyze(clean),flutter test: 52 new tests, 4933 passing. Drift output regenerated withdart run build_runner build --delete-conflicting-outputs.Two existing suites needed a real fix rather than a nudge. The deferred-scanner concurrency tests assumed the filesystem walk had already started the moment a scan future was created; that stopped being true once the scan reads the indexed slice first, so they now wait for the walk to genuinely start before superseding it, which is what they meant to assert. And the Library screen widget test now stubs the stat seam for the same reason it already stubs the tag reader, with the same comment: real filesystem I/O on fictional paths never completes against
pumpAndSettle's fake clock.Relationship to the other three
library_controller.dart, where both want the previous slice available on every scan, and in two concurrency tests that both need the same restructure for the same reason. Expect a small conflict there, whichever lands second.🤖 Generated with Claude Code
https://claude.ai/code/session_01Lq4ZJtwtdVCMBuZLjFbyfV
Generated by Claude Code