fix(gix-note): edit notes trees lazily - #2963
Open
Sebastian Thiel (Byron) wants to merge 6 commits into
Open
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d149d746f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
Sebastian Thiel (Byron)
force-pushed
the
gix-notes-perf
branch
from
September 2, 2026 11:08
3945e4d to
a3b1edc
Compare
Exercise `get()` and `replace()` against 65,536-note fanout trees held entirely in `gix_odb::memory::Proxy`, keeping fixture construction and filesystem I/O outside the timed paths. Cover both the worst-case one-to-two-level fanout expansion and the common steady-state two-level replacement. Count each operation as one element so Criterion reports notes per second for GET and PUT. Run the matrix with `cargo bench -p gix-note --bench read-write`.
`replace()` and `remove()` loaded every fanout subtree into a `HashMap`, then reconstructed every logical note path. This made one edit O(total notes). Represent unopened fanout directories as radix-tree leaves carrying their existing tree IDs. Materialize only the matching edit path and subtrees that the fanout heuristic must collapse. Preserve opaque subtrees whenever the current fanout allows it, matching Git's trade-off: fanout after an edit depends on what the lazy trie has materialized instead of a global recount. Retain each opened subtree's original path components so non-note entries keep their spelling, including uppercase fanout-like directories. A regression with 32 root fanout entries proves replacement reads and writes only the root and target subtree, while the Git-produced boundary fixture verifies exact tree IDs. The now-unused `gix-hashtable` dependency is removed. Git baseline: git.git 1630431f326e15fcde608827b5ff38422528eb59, especially `note_tree_search()`, `load_subtree()`, `determine_fanout()`, `for_each_note_helper()`, and `write_notes_tree()` in `notes.c`.
Git keeps `struct notes_tree` alive across lookups and edits, preserving radix nodes and lazily materialized subtree entries. Recreating that structure for every operation repeats tree parsing and entry allocation. Add caller-owned `State` and require it in `get()`, `replace()`, and `remove()`. The state advances after each successful edit. `gix::note::Platform` retains states by root-tree ID so references and aliases sharing a tree also share parsed entries, and clears cached states after plumbing errors. A counting in-memory ODB regression proves repeated operations do not reread materialized trees. Measurements on 65,536-note fixtures were: | Scenario | New state | Reused state | | --- | ---: | ---: | | Common two-level GET | ~16,700 GET/s | ~111,000,000 GET/s | | Common two-level PUT | ~5,470 PUT/s | ~8,000 PUT/s | | One-level fanout GET | ~20,100 GET/s | ~108,000,000 GET/s | | One-level fanout PUT | ~3,310 PUT/s | ~3,980 PUT/s | The reused one-level PUT is primed by one untimed expanding write and therefore measures blob-only replacements after expansion. BREAKING CHANGE: `gix_note::get()`, `gix_note::replace()`, and `gix_note::remove()` now require a mutable `State` initialized with `State::new()`. Git baseline: git.git 1630431f326e15fcde608827b5ff38422528eb59, `struct notes_tree` in `notes.h`, and `init_notes()`, `get_note()`, `add_note()`, `remove_note()`, and `write_notes_tree()` in `notes.c`. `notes-cache.c` builds its cache API on the same persistent tree.
Include Git's `notes.c` directly so the benchmark can construct an already-materialized `struct notes_tree` matching `fanout-expansion-1-level-fanout/replace/reused-state` without reading 65,536 notes. Replace `odb_write_object()` with Git's in-memory object path and remove the filesystem-backed ODB source. Prime the replacement state, alternate note blobs, and assert the expected 258 tree hashes per PUT. | 65,536-note replacement | Time | Throughput | | --- | ---: | ---: | | Git in memory | 149.57 µs/PUT | 6,686 PUT/s | | `gix-note` baseline | 250.45 µs/PUT | 3,993 PUT/s |
`gix_object::Write::write()` serialized an object into the trait buffer, then `write_stream()` copied that buffer again. Override it in the memory proxy so objects are serialized once, then hashed and stored directly. Keep the existing allocation when its content-addressed ID is already present. Valid callers promise that a known ID matches their bytes, so this does not change observable object contents. | `gix-note` fanout replacement | Time | Throughput | | --- | ---: | ---: | | Before | 250.45 µs/PUT | 3,993 PUT/s | | After | 231.43 µs/PUT | 4,321 PUT/s | | Git in-memory baseline | 149.57 µs/PUT | 6,686 PUT/s |
Git streams its already-sorted note traversal directly into tree buffers. Do the same when there are no materialized non-note entries, recursively grouping adjacent path components into trees instead of rebuilding the layout through `gix_object::tree::Editor` and its path-keyed hash map. Keep the editor path for trees containing non-note entries, where its overwrite and conflict handling remain useful. | 65,536-note, 258-tree replacement | Time | Throughput | | --- | ---: | ---: | | Before | 231.43 µs/PUT | 4,321 PUT/s | | After | 137.65 µs/PUT | 7,265 PUT/s | | Git in-memory median | 142.30 µs/PUT | 7,028 PUT/s |
Sebastian Thiel (Byron)
force-pushed
the
gix-notes-perf
branch
from
September 2, 2026 12:03
a3b1edc to
ea4152c
Compare
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.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by Codex GPT-5.
Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Summary
gix-hashtabledependency.Reported issue
Compare this with how Git does it in /Users/byron/dev/github.com/git/git
The
edit()path does a full traversal and reconstruction:gix-note/src/lib.rs:121callscollect()on the root.collect()at line 168 visits every entry and recursively descends into every notes fanout subtree at line 194.HashMapcontains every note; only one entry is changed at lines 130–133.write()at line 226 starts with an empty tree.So the CPU/allocation work is O(number of notes), and the full notes tree is read.
My wording was slightly imprecise: it reconstructs the complete logical tree, but content addressing means unchanged tree objects may hash identically and already exist in the ODB. It does not necessarily add N fresh Git objects on every edit. A path-local editor would avoid the full traversal, but matching Git’s global fanout heuristic makes that less trivial than my earlier answer suggested.
Requested direction
$issue-full-auto Do it like Git does, accepting the trade-off that comes with it.Git baseline
Compared with git.git
1630431f326e15fcde608827b5ff38422528eb59, especiallynotes.cfunctionsnote_tree_search(),load_subtree(),determine_fanout(),for_each_note_helper(), andwrite_notes_tree().Like Git, the implementation keeps untouched subtrees opaque. A fanout decision can therefore change when editing materializes one sparse bucket instead of after a global note recount.
Validation
cargo +1.88 test -p gix-note --all-featurescargo fmt --all -- --checkcargo clippy -p gix-note --all-targets --all-features --no-deps -- -D warningscargo test -p gix --test gix repository::noteCommit:
d149d746f2fa0951c5db5b812e6caa15e28e02e7