perf: make small-stream access and path lookups linear in the number of entries - #81
Merged
mdsteele merged 5 commits intoAug 28, 2026
Merged
Conversation
Every access to a mini sector went through seek_within_mini_sector, which opened the mini stream's chain from its start sector and walked the whole FAT chain to build the sector list, then used one entry of it. The same happened in set_minifat for the MiniFAT's chain, and append_mini_sector and allocate_mini_sector walked their chains again to extend them. With n small streams the mini stream is O(n) sectors long, so reading or writing them all cost O(n^2): 500 streams of 300 bytes took 35x the time of 50 (issue mdsteele#57). MiniAllocator now walks each chain once, keeps the sector IDs, extends them in place as the chains grow (extend_chain accepts any sector of a chain, so extending from the cached last sector is O(1)), and drops the cache if the chain itself is freed. Mini-sector seeks index the cached list directly. Chain::into_subsector, which existed for the rewalk, is gone. Two tests exercise growth, freeing, regrowth and the mini-stream cutoff in both directions, on the file being written and on it reopened.
…tree The sibling tree under a storage is a plain binary search tree that is never rebalanced (insert_dir_entry appends where the comparison lands), so a writer that creates entries in sorted order — the common case — leaves a chain, and every path lookup walks it: with n storages under the root a lookup costs O(n), and reading them all O(n^2). Measured per lookup: 0.8 us at 50 storages, 5.3 us at 500, 15.5 us at 2000. Directory now keeps a map from (parent stream ID, name key) to stream ID, built once when the file is opened and kept current by insert and remove — including a removed entry's slot being refilled by its in-order predecessor, whose children (if it is a storage) move with it — and stream_id_for_name_chain resolves each path component through it. The key is what compare_names treats as equal: the UTF-16 length and the CFB-uppercased characters. The tree on disk is unchanged, so files written by this crate are byte-identical to before. Tests walk every entry of a 600-storage file and resolve each in its own and another case after removals and re-additions, on the file being written and reopened, and cover a storage moved into a removed sibling's slot keeping its children reachable.
mdsteele
reviewed
Aug 26, 2026
| /// as encoded in UTF-16 (i.e. [shortlex | ||
| /// order](https://en.wikipedia.org/wiki/Shortlex_order), rather than | ||
| /// dictionary order). | ||
| /// The key under which a name is indexed: two names get the same key |
Owner
There was a problem hiding this comment.
It looks like this method and its doc comment accidentally got inserted below the doc comment of the following method.
Contributor
Author
There was a problem hiding this comment.
Well spotted, thanks - name_key and its doc had landed between compare_names and its doc comment. Moved name_key above the doc block so each method sits under its own comment; no code change.
name_key and its doc had been inserted between compare_names and its doc comment (review feedback). Move name_key above the doc block so each method sits under its own comment. No code change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Under edition 2018 a panic message with no arguments is a plain string,
so the `{i}` placeholders in the new tests' assert! messages were never
formatted (clippy: non_fmt_panics). Pass the values explicitly, and
write a fixed-size fill as an array rather than a vec.
Contributor
Author
|
Pushed 7f28f50 for the linters job: the new tests' assert messages relied on inline captures, which a panic message doesn't format under edition 2018 (clippy |
Owner
|
Thanks! |
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
2 participants
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.
Two quadratic terms surfaced when we (embedded-society/altium-designer-mcp, which stores one storage with a few small streams per PCB component) measured reading and writing files with hundreds of storages. They are the mechanism behind #57, which you diagnosed there as
Chain::newbeing rebuilt on every access.1.
seek_within_mini_sectorrewalked the mini stream's FAT chain on every access (open_chain→Chain::new), andset_minifatdid the same for the MiniFAT chain;append_mini_sector/allocate_mini_sectorwalked them again to extend. With n small streams the mini stream is O(n) sectors long, so every read or write of every small stream cost O(n): 500 streams of 300 bytes took 36× the time of 50 (open_stream+read_to_endover all of them: 1.9 ms → 69 ms; creation 3.9 ms → 105 ms).MiniAllocatornow walks each chain once, keeps the sector IDs, extends them in place (extend_chainaccepts any sector of a chain, so extending from the cached last sector is O(1)) and drops the cache if the chain itself is freed.Chain::into_subsector, which existed only for the rewalk, is gone.2. Path lookups walked an unbalanced sibling tree.
insert_dir_entrynever rebalances (theTODOthere), so entries created in sorted order — the common case — form a chain, andstream_id_for_name_chaincosts O(siblings) per component: 0.8 µs per lookup at 50 storages, 5.3 µs at 500, 15.5 µs at 2000.Directorynow keeps(parent stream ID, name key) → stream ID, built once on open and kept current by insert and remove, including the case where a removed entry's slot is refilled by its in-order predecessor (whose children, if it is a storage, move with it). The key is exactly whatcompare_namestreats as equal (UTF-16 length + CFB-uppercased characters), so behaviour is unchanged. The on-disk tree is not touched: files written by the crate are byte-identical to before, which matters for us because our output must match Altium's own byte for byte.Result on our workload (500 storages × 4 small streams, release build): open 135 ms → 20 ms, create 180 ms → 87 ms; scaling ratios for 10× the entries go from 35 / 20 to 8 / 13.
Tests:
tests/mini_streams.rs(grow, free, regrow, cross the mini-stream cutoff both ways, reopened),tests/name_index.rs(every walked entry of a 600-storage file resolves in its own and another case after removals and re-additions, reopened) andtests/name_index_moves.rs(the predecessor-moves-a-storage case). Existing suite passes;cargo fmtperrustfmt.toml.Happy to split this into two PRs if you'd prefer to take them separately.