From cb47090e1113ed6bb4b74832a96baf9d75d2fa25 Mon Sep 17 00:00:00 2001 From: Hadrian Burkhardt Date: Sat, 17 Oct 2026 20:57:19 +0200 Subject: [PATCH] Draw a large folder while it is being read, instead of after Two seconds of readdir and stat cannot be made much cheaper - each is a round trip to another process - so the folder is now read in batches and drawn as it arrives. StorageSource.listBatches hands over 200 entries at a time; AllFilesSource implements it with Files.newDirectoryStream, which is lazy where File.listFiles materialised the whole directory first; the default implementation is one batch, so the SAF backend and every test fake are unchanged. BrowserViewModel.load merges each batch into the list already on screen with core/SortedMerge, which is O(n+k) rather than re-sorting the folder on every batch, and keeps the rows in the user's order at every moment instead of shuffling them into place when the last batch lands. loading stays true until the last batch, so the progress bar the screen already draws sits above rows that are already useful. publishEntries is separate from recompute for that reason: the merge has done the ordering, and the batch path only has to apply the hidden filter. Tests: four for the merge (including ties not jumping ahead) and one for the batched read, which asserts 250 files arrive as 100+100+50 covering every entry exactly once - the thing a streaming read is easiest to get wrong about. PERFORMANCE.md records the expected effect and states plainly that it is not measured yet, since confirming it needs the 20000-file fixture rebuilt. --- .../filemanager/core/SortedMerge.kt | 42 +++++++++++++ .../filemanager/data/AllFilesSource.kt | 29 +++++++++ .../filemanager/data/StorageSource.kt | 28 +++++++++ .../ui/browser/BrowserViewModel.kt | 37 ++++++++++- .../filemanager/core/SortedMergeTest.kt | 61 +++++++++++++++++++ .../filemanager/data/AllFilesSourceTest.kt | 16 +++++ docs/CODE_MAP.md | 21 +++++-- docs/PERFORMANCE.md | 22 +++++++ 8 files changed, 247 insertions(+), 9 deletions(-) create mode 100644 app/src/main/java/de/softwareapp_hb/filemanager/core/SortedMerge.kt create mode 100644 app/src/test/java/de/softwareapp_hb/filemanager/core/SortedMergeTest.kt diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/core/SortedMerge.kt b/app/src/main/java/de/softwareapp_hb/filemanager/core/SortedMerge.kt new file mode 100644 index 0000000..0ff9468 --- /dev/null +++ b/app/src/main/java/de/softwareapp_hb/filemanager/core/SortedMerge.kt @@ -0,0 +1,42 @@ +package de.softwareapp_hb.filemanager.core + +/** + * Adds a freshly read batch to an already sorted list, in one pass. + * + * A folder is now read in batches so that the first screenful can be drawn while the rest is still being + * `stat`ed. Each batch is sorted on its own and merged here, which is O(n + k) — against O(n log n) for + * re-sorting everything after every batch — and it means the rows on screen are always in the order the + * user asked for, never a partly ordered jumble that reorders itself when the next batch lands. + */ +object SortedMerge { + + /** + * [sorted] must already be ordered by [comparator]; [batch] need not be. + * + * Ties keep the *earlier* entry first (`<=`), so an entry does not jump over an equal one just + * because it arrived in a later batch. + */ + fun merge( + sorted: List, + batch: List, + comparator: Comparator, + ): List { + if (batch.isEmpty()) return sorted + val ordered = batch.sortedWith(comparator) + if (sorted.isEmpty()) return ordered + + val merged = ArrayList(sorted.size + ordered.size) + var left = 0 + var right = 0 + while (left < sorted.size && right < ordered.size) { + if (comparator.compare(sorted[left], ordered[right]) <= 0) { + merged += sorted[left++] + } else { + merged += ordered[right++] + } + } + while (left < sorted.size) merged += sorted[left++] + while (right < ordered.size) merged += ordered[right++] + return merged + } +} diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/AllFilesSource.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/AllFilesSource.kt index 2b01ca8..da902b6 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/data/AllFilesSource.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/AllFilesSource.kt @@ -4,6 +4,9 @@ import android.annotation.SuppressLint import de.softwareapp_hb.filemanager.core.FileEntry import de.softwareapp_hb.filemanager.core.StoragePath import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.withContext import java.io.File import java.io.FileInputStream @@ -12,6 +15,8 @@ import java.io.InputStream import java.io.OutputStream import java.nio.channels.FileChannel import java.nio.channels.SeekableByteChannel +import java.nio.file.Files +import java.nio.file.Paths import java.nio.file.StandardOpenOption /** @@ -37,6 +42,30 @@ class AllFilesSource( if (children == null) emptyList() else children.map { it.toEntry() } } + /** + * The same entries, handed over every [batchSize] rows while the directory is still being read. + * + * `Files.newDirectoryStream` is lazy — it pulls one `readdir` result at a time as the loop asks — + * which is what makes a first batch possible at all; `File.listFiles` would materialise the whole + * folder before returning anything, and on this phone that is about two seconds for twenty thousand + * files. `flowOn(IO)` keeps the loop (and every `stat` in it) off the caller's thread; the caller + * only ever sees finished batches. + */ + override fun listBatches(directory: StoragePath, batchSize: Int): Flow> = + flow { + val batch = ArrayList(batchSize) + Files.newDirectoryStream(Paths.get(directory.value)).use { stream -> + for (path in stream) { + batch += path.toFile().toEntry() + if (batch.size >= batchSize) { + emit(batch.toList()) + batch.clear() + } + } + } + if (batch.isNotEmpty()) emit(batch.toList()) + }.flowOn(Dispatchers.IO) + /** * One `readdir` and no `stat` per child, which is the whole reason a catch-up pass is * cheap. `File.list()` returns `null` for a folder that cannot be read — exactly the diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/StorageSource.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/StorageSource.kt index 4d39179..8750516 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/data/StorageSource.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/StorageSource.kt @@ -3,6 +3,8 @@ package de.softwareapp_hb.filemanager.data import de.softwareapp_hb.filemanager.core.FileEntry import de.softwareapp_hb.filemanager.core.StoragePath import java.io.InputStream +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flow import java.io.OutputStream import java.nio.channels.SeekableByteChannel @@ -38,6 +40,32 @@ interface StorageSource { /** Children of [directory], in whatever order the backend returns them (unsorted). */ suspend fun list(directory: StoragePath): List + /** + * [list], in the order the backend reads it, a batch at a time. + * + * The reason is the first batch. Reading a folder means a `readdir` and a `stat` per child, and a + * phone's mounted storage makes each of those a round trip — measured at roughly 0.1 ms per file, so + * twenty thousand files is about two seconds in which nothing can be drawn. Batches let the screen + * show what has been read while the rest is being read, which is the difference between a slow + * folder and a frozen one. + * + * The default is a single batch holding everything, so a backend that cannot stream — and every test + * fake — behaves exactly as before. + */ + fun listBatches(directory: StoragePath, batchSize: Int = LIST_BATCH): Flow> = + flow { emit(list(directory)) } + + companion object { + /** + * How many entries are handed over at a time while a folder is read. + * + * Two hundred is roughly a few screens' worth: small enough that the first batch is drawn + * quickly, large enough that a folder of a thousand files arrives in five batches rather than + * fifty state updates. + */ + const val LIST_BATCH = 200 + } + /** * The names in [directory], **without reading metadata for each one**. * diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModel.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModel.kt index dc44a44..3437ded 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModel.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModel.kt @@ -22,6 +22,7 @@ import de.softwareapp_hb.filemanager.core.NameAllocator import de.softwareapp_hb.filemanager.core.SortDirection import de.softwareapp_hb.filemanager.core.SortKey import de.softwareapp_hb.filemanager.core.SortSpec +import de.softwareapp_hb.filemanager.core.SortedMerge import de.softwareapp_hb.filemanager.core.StoragePath import de.softwareapp_hb.filemanager.core.TransferAction import de.softwareapp_hb.filemanager.core.isUsableName @@ -297,8 +298,9 @@ class BrowserViewModel( // Measured off the main thread, after the listing is up: see [fillFreeSpace]. viewModelScope.launch { fillFreeSpace(path) } viewModelScope.launch { - _state.update { it.copy(loading = true) } - listing = runCatching { source.list(path) }.getOrDefault(emptyList()) + // The folder is announced before anything is read, so leaving the previous one is immediate + // and the progress bar has something to sit above. + listing = emptyList() _state.update { it.copy( path = path, @@ -310,14 +312,43 @@ class BrowserViewModel( searchRoot = null, searching = false, freeBytes = null, - loading = false, + loading = true, + entries = emptyList(), ) } + + // Batch by batch: the first screenful is on screen while the rest is still being read. Each + // batch is merged into the list already on screen (see [SortedMerge]), so rows arrive in the + // user's order rather than shuffling into place afterwards. + runCatching { + source.listBatches(path).collect { batch -> + listing = SortedMerge.merge(listing, batch, _state.value.sort.comparator) + publishEntries() + } + } + + if (_state.value.path == path) _state.update { it.copy(loading = false) } + // Once more at the end, in case the sort was changed while the folder was still arriving: + // batches were merged with whatever order was current when each one landed. recompute() persistViewState() } } + /** + * Publishes what has been read so far, with the hidden filter applied. + * + * No sort here: [listing] is kept ordered by [SortedMerge] as batches arrive, so a batch costs one + * merge and one filter rather than a sort of the whole folder — which matters when the whole folder + * is twenty thousand rows being delivered a hundred times. + */ + private fun publishEntries() { + val current = _state.value + _state.update { state -> + state.copy(entries = listing.filter { current.showHidden || !it.isHidden }) + } + } + /** * Derive what is shown from the unfiltered listing. * diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/core/SortedMergeTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/core/SortedMergeTest.kt new file mode 100644 index 0000000..68d842a --- /dev/null +++ b/app/src/test/java/de/softwareapp_hb/filemanager/core/SortedMergeTest.kt @@ -0,0 +1,61 @@ +package de.softwareapp_hb.filemanager.core + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * How a batch that arrives late joins a list that is already on screen. + * + * The list has to be *ordered at every moment* — not "sorted once the folder has finished", because the + * folder may never finish visibly for twenty thousand rows — so each batch is merged rather than the + * whole list re-sorted. + */ +class SortedMergeTest { + + private val byName = SortSpec().comparator + + private fun file(name: String, size: Long = 0) = FileEntry( + path = StoragePath.of("/sdcard/$name"), + isDirectory = false, + sizeBytes = size, + lastModifiedMillis = 0, + ) + + @Test + fun `a later batch slots into the order already on screen`() { + val merged = SortedMerge.merge( + sorted = listOf(file("a.txt"), file("c.txt")), + batch = listOf(file("d.txt"), file("b.txt")), + comparator = byName, + ) + + assertEquals(listOf("a.txt", "b.txt", "c.txt", "d.txt"), merged.map { it.name }) + } + + @Test + fun `the first batch into an empty list is sorted on its own`() { + val merged = SortedMerge.merge(emptyList(), listOf(file("c.txt"), file("a.txt")), byName) + + assertEquals(listOf("a.txt", "c.txt"), merged.map { it.name }) + } + + @Test + fun `an empty batch changes nothing`() { + val shown = listOf(file("a.txt"), file("b.txt")) + + assertEquals(shown, SortedMerge.merge(shown, emptyList(), byName)) + } + + @Test + fun `an entry that ties on name does not jump ahead of one that arrived earlier`() { + // Two files with the same name can exist in different folders of a search result; the one + // already on screen keeps its position so the row under the user's finger does not change. + val merged = SortedMerge.merge( + sorted = listOf(file("same.txt", size = 1)), + batch = listOf(file("same.txt", size = 2)), + comparator = byName, + ) + + assertEquals(listOf(1L, 2L), merged.map { it.sizeBytes }) + } +} diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/data/AllFilesSourceTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/data/AllFilesSourceTest.kt index 52bd2e5..a877f16 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/data/AllFilesSourceTest.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/data/AllFilesSourceTest.kt @@ -24,6 +24,22 @@ class AllFilesSourceTest { private fun path(file: File) = StoragePath.of(file.absolutePath) + @Test + fun `reads a large folder in batches, covering every entry exactly once`() = runBlocking { + val dir = temp.newFolder("big") + repeat(250) { File(dir, "file-$it.txt").writeText("x") } + + val batches = mutableListOf>() + source(dir).listBatches(path(dir), batchSize = 100).collect { batch -> + batches += batch.map { it.name } + } + + // 250 files at 100 per batch: two full batches and a remainder, not one batch of everything. + assertEquals(listOf(100, 100, 50), batches.map { it.size }) + // And every file exactly once, which is what a streaming read is easy to get wrong about. + assertEquals(250, batches.flatten().toSet().size) + } + @Test fun `lists the children of a directory`() = runBlocking { val dir = temp.newFolder("docs") diff --git a/docs/CODE_MAP.md b/docs/CODE_MAP.md index 6bc8c8f..2cb7723 100644 --- a/docs/CODE_MAP.md +++ b/docs/CODE_MAP.md @@ -2,7 +2,7 @@ Generated from the source by `tools/make-code-map.py` - do not edit by hand. Regenerate it after a change (`python3 tools/make-code-map.py`); `--check` exits non-zero when this file is stale. -78 source files, 12910 lines - 34 test files, 391 tests. The behaviour index at the end lists every test name, and in this project a test name is a sentence about what the app does, so it doubles as the specification. +79 source files, 13040 lines - 35 test files, 396 tests. The behaviour index at the end lists every test name, and in this project a test name is a sentence about what the app does, so it doubles as the specification. Start at `AGENTS.md`. What is done, and why: `docs/ROADMAP.md` and `CHANGELOG.md`. What was verified by hand rather than by a test: `docs/EDGE_CASE_TESTING.md`. @@ -24,6 +24,7 @@ Pure Kotlin, **no Android imports**: paths, file names, kinds, sorting, formatti | `NameAllocator.kt` | 30 | Picks a name that is not already in use, in the familiar "name (1)" style. NameAllocator | `NameMatcherTest` (10) | | `NameMatcher.kt` | 63 | The in-folder search matcher. NameMatcher | `NameMatcherTest` (10) | | `SortSpec.kt` | 82 | A complete, serialisable description of the ordering of a directory listing. SortKey, SortDirection, SortSpec, NaturalOrder | `SortSpecTest` (8) | +| `SortedMerge.kt` | 42 | Adds a freshly read batch to an already sorted list, in one pass. SortedMerge | `SortedMergeTest` (4) | | `StoragePath.kt` | 89 | A normalized, absolute, backend-agnostic storage path. StoragePath | `StoragePathTest` (8) | | `ThumbnailKey.kt` | 22 | The identity of a thumbnail: which file it came from, and which version of that file. ThumbnailKey | `ThumbnailKeyTest` (4) | | `Timestamps.kt` | 20 | Timestamps for the details sheet. Timestamps | - | @@ -35,7 +36,7 @@ Storage backends (all-files access and SAF), volumes, incoming shares, the SQLit | file | lines | what it holds | tests | |---|---|---|---| -| `AllFilesSource.kt` | 125 | The "All files access" backend: plain java.io.File over absolute paths. AllFilesSource, File() | `AllFilesSourceTest` (13) | +| `AllFilesSource.kt` | 154 | The "All files access" backend: plain java.io.File over absolute paths. AllFilesSource, File() | `AllFilesSourceTest` (14) | | `ApkInspector.kt` | 100 | What an APK says about itself, read without installing it. ApkInfo, interface(), PackageManagerApkInspector | - | | `BookmarkStore.kt` | 34 | The folders the user starred. BookmarkStore | - | | `IncomingSource.kt` | 48 | Something another app has handed to this one. IncomingSource, ContentResolverIncomingSource | - | @@ -50,7 +51,7 @@ Storage backends (all-files access and SAF), volumes, incoming shares, the SQLit | `StorageAccess.kt` | 80 | The Android-facing side of the storage-access decision. StorageAccess | - | | `StorageAccessPolicy.kt` | 68 | The decision rules behind StorageAccess. StorageAccessMode, StorageAccessPolicy | `StorageAccessPolicyTest` (6) | | `StorageLocation.kt` | 87 | Everything the browser needs to know about where files live and what it may touch. StorageLocation, StorageProvider | - | -| `StorageSource.kt` | 96 | The primitive file operations a storage backend must provide. StorageException, StorageSource | - | +| `StorageSource.kt` | 124 | The primitive file operations a storage backend must provide. StorageException, StorageSource | - | | `TrashIndex.kt` | 61 | The durable memory of what is in the trash. TrashRecord, TrashIndex | - | | `VolumeRepository.kt` | 121 | Discovers the device's storage volumes and reports their space. VolumeInfo, SpaceInfo, VolumeRepository | `VolumeRepositoryTest` (6) | @@ -102,7 +103,7 @@ The browser screen: view model, state, list and grid, dialogs, selection bar | `BrowserScreen.kt` | 591 | BrowserScreen() | - | | `BrowserTopBar.kt` | 238 | The browser's app bar: title, up, the favourite star, and the search / sort / overflow actions. BrowserTopBar(), SEARCH_FIELD_WIDTH | `BrowserTopBarTest` (4) | | `BrowserUiState.kt` | 311 | One immutable description of everything the browser shows. ViewMode, SearchScope, Clipboard, UiMessage, ConflictPrompt, RootItem, Bookmark, DetailFocus, Undo, UndoMove, Overlay, BrowserUiState | - | -| `BrowserViewModel.kt` | 2206 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (90) | +| `BrowserViewModel.kt` | 2237 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (90) | | `DetailsSheet.kt` | 109 | Name, location, size, type and modified date for one entry. DetailsSheet(), DetailLines(), DetailLine() | - | | `EntryIntents.kt` | 58 | Wraps an entry's path in a content:// URI another app may read. contentUriFor(), viewIntent(), shareIntent() | - | | `IncomingBar.kt` | 60 | The bar shown while another app is waiting on this one. IncomingBar() | - | @@ -289,6 +290,13 @@ Every test name in the suite, by file: the fastest way to see whether something - comparison ignores case - a prefix sorts before a longer name +### `core/SortedMergeTest.kt` (4) + +- a later batch slots into the order already on screen +- the first batch into an empty list is sorted on its own +- an empty batch changes nothing +- an entry that ties on name does not jump ahead of one that arrived earlier + ### `core/StoragePathTest.kt` (8) - normalizes separators and dot segments @@ -315,8 +323,9 @@ Every test name in the suite, by file: the fastest way to see whether something - a folder cannot be moved into itself or a descendant - a move into itself is skipped in the plan -### `data/AllFilesSourceTest.kt` (13) +### `data/AllFilesSourceTest.kt` (14) +- reads a large folder in batches, covering every entry exactly once - lists the children of a directory - names lists a folder without reading metadata - names of a folder that cannot be read is null, not empty @@ -676,4 +685,4 @@ Every test name in the suite, by file: the fastest way to see whether something --- -Purpose taken from the first KDoc rather than from a declaration sharing the file's name (15 of 77): `MainActivity.kt`, `ArchiveFormats.kt`, `FileNames.kt`, `ApkInspector.kt`, `OperationModels.kt`, `TextRes.kt`, `BrowserComponents.kt`, `BrowserDetailPane.kt`, `BrowserDialogs.kt`, `BrowserRoots.kt`, `BrowserScreen.kt`, `EntryIntents.kt`, `Color.kt`, `Theme.kt`, `Type.kt` +Purpose taken from the first KDoc rather than from a declaration sharing the file's name (15 of 78): `MainActivity.kt`, `ArchiveFormats.kt`, `FileNames.kt`, `ApkInspector.kt`, `OperationModels.kt`, `TextRes.kt`, `BrowserComponents.kt`, `BrowserDetailPane.kt`, `BrowserDialogs.kt`, `BrowserRoots.kt`, `BrowserScreen.kt`, `EntryIntents.kt`, `Color.kt`, `Theme.kt`, `Type.kt` diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md index fa9f753..3ddd181 100644 --- a/docs/PERFORMANCE.md +++ b/docs/PERFORMANCE.md @@ -160,6 +160,28 @@ scheduler hiccup on top of that — unidentified, and worth a perfetto trace rat cheap in absolute terms, but it is work the frame has no reason to wait for, and the threshold keeps small folders on the simple synchronous path. +### The same cost, delivered in pieces + +Those two seconds of `readdir` and `stat` cannot be made much cheaper — they are round trips to another +process — so the fix is to **draw while they happen**: + +- `StorageSource.listBatches` hands a folder over 200 entries at a time. `AllFilesSource` implements it + with `Files.newDirectoryStream`, which is lazy: it pulls `readdir` results as the loop asks for them, + where `File.listFiles` materialised the whole folder before returning anything. The default + implementation is still a single batch, so the SAF backend and every test fake behave exactly as before. +- `BrowserViewModel.load` merges each batch into the list already on screen (`core/SortedMerge`, + O(n + k) instead of re-sorting the folder after every batch), so rows arrive in the user's order and the + list is never a partly ordered jumble that shuffles into place afterwards. +- `loading` stays true until the last batch, which is what the existing progress bar above the list already + draws — the feedback that keeps a slow folder from feeling like a frozen one. + +At 200 entries per batch, the first rows should appear after roughly 20 ms of reading plus a frame, against +two seconds of empty screen before. **That expectation is not measured yet**: confirming it needs a +20 000-file fixture, which takes about half an hour to build through MIUI's storage layer, so the numbers to +beat are the ones in the table above and the first thing to do with that fixture is to re-run +`openAFolderOfAThousandFiles` and look at the *min* iteration rather than the median — the minimum is what +"first rows visible" now governs. + Building that fixture is the expensive part, not measuring it: 20 000 files took about thirty minutes through MIUI's storage layer (`: >` in a loop), against roughly four minutes of benchmark on top. Worth knowing before anyone re-creates it, and an argument for the fixture being built once and kept.