From 54a1e5fd040107fa3596823a5829124faaab465e Mon Sep 17 00:00:00 2001 From: Ofer Chen Date: Thu, 7 May 2026 16:45:49 +0300 Subject: [PATCH] docs(audits): Vec vs upstream pool allocator (#1050) Static audit comparing upstream rsync's per-flist slab pool (`lib/pool_alloc.c`) against oc-rsync's `Vec` plus per-entry `PathBuf` and `Box` heap allocations. Lists bumpalo / typed-arena candidates for collapsing the file list into a single slab and the lifetime, Drop, and signal-cleanup risks that gate the pilot. --- docs/audits/vec-fileentry-vs-pool.md | 89 ++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 docs/audits/vec-fileentry-vs-pool.md diff --git a/docs/audits/vec-fileentry-vs-pool.md b/docs/audits/vec-fileentry-vs-pool.md new file mode 100644 index 000000000..0f75f90c7 --- /dev/null +++ b/docs/audits/vec-fileentry-vs-pool.md @@ -0,0 +1,89 @@ +# `Vec` vs upstream pool allocator (#1050) + +Static audit of the file-list allocation strategy. Compares upstream +rsync's slab pool (`lib/pool_alloc.c`) against oc-rsync's +`Vec` plus per-entry heap allocations, and lists the +single-slab arena candidates worth piloting. Ships no code change. + +## 1. Upstream: `pool_alloc` slabs for `struct file_struct` + +Upstream allocates every file-list entry from a per-flist slab pool: + +- `lib/pool_alloc.c:48` `pool_create(SMALL_EXTENT|NORMAL_EXTENT, ...)`, + `:115` `pool_alloc()`, `:259` `pool_free_old()`, `:310` `pool_boundary()`. +- `flist.c:2914` and `:2920` create the pool inside `flist_new()`; sibling + flists reuse `first_flist->file_pool` (`flist.c:2929`). +- `flist.c:1018` packs each entry as a single contiguous record: + `alloc_len = FILE_STRUCT_LEN + extra_len + basename_len + dirname_len + linkname_len`, + filled by one `pool_alloc(pool, alloc_len, "recv_file_entry")` at `:1020` + (sender mirror at `flist.c:1239`+). +- `util2.c` provides only the OOM helper; the slab logic lives in + `lib/pool_alloc.c`. `flist.c:325` calls `pool_boundary(...,8*1024)` so an + flist can be released as one batch via `pool_free_old`. + +Net effect: one allocation per entry, freed in O(1) per flist, no +per-field `malloc` for path or extras. + +## 2. oc-rsync: `Vec` + boxed extras + +`crates/protocol/src/flist/entry/core.rs:32` defines `FileEntry` with +`name: PathBuf`, `dirname: Arc`, and +`extras: Option>` (`:55`). Every populated entry +therefore costs at least: + +| Allocation | Source | Per-entry bytes (glibc, 64-bit) | +|------------|--------|---------------------------------| +| `Vec` slot | inline `FileEntry` (~200 B; see `entry/tests.rs:294`) | 200 | +| `PathBuf` heap (name) | `name: PathBuf` at `core.rs:35` | 32-48 + path len | +| `Arc` heap | shared via `PathInterner` (`core.rs:42`) | amortized 24 / unique dir | +| `Box` | `entry/accessors.rs:14` lazy box | 0 or 64-128 | +| glibc `malloc` chunk overhead | 16 B header per chunk | 32-48 | + +Three to four discrete `malloc`s per non-trivial entry (vec slot, path +buffer, extras box, optional symlink target inside extras) versus one +`pool_alloc` upstream. At 1 M entries that is ~200 MB of fragmented +heap chunks plus glibc bookkeeping, against a handful of slab extents +upstream. + +## 3. Arena candidates + +- `bumpalo` 3.x: `Bump` arena with `bumpalo::collections::Vec` / + `boxed::Box`. Drop-free, per-flist reset matches `pool_free_old`. No + `Drop` recursion - `FileEntry` must be POD-ish or wrapped in + `ManuallyDrop`. +- `typed-arena`: simpler `Arena` with stable references; no + reset, must drop the arena to reclaim. Acceptable for incremental + flists if each segment owns its arena. +- `slab` crate: keyed slab; useful only if we keep handles instead of + pointers. Heavier than `bumpalo` for our access pattern. + +Suggested pilot: arena per `FileList` segment, store `&'arena str` +basenames carved from the same arena, keep the existing `Arc` +dirname interner outside the arena to preserve cross-segment sharing. + +## 4. Risks + +- **Lifetimes.** `FileEntry` currently owns its strings; an arena + flips ownership to a borrow tied to the flist. Every consumer + (`engine`, `transfer`, `cli`) must accept a generic + `FileEntry<'a>` or the arena must be `'static` (which defeats reuse). +- **Drop semantics.** `FileEntryExtras` carries `Vec` symlink + targets and checksum buffers. Bumpalo skips `Drop`, so these would + leak unless we keep `Box`/`Vec` outside the arena or switch to arena + slices. +- **Signal cleanup.** Signal handlers and `RawSyncReceiver` cancel + paths (`crates/transfer/src/receiver.rs`) currently rely on `Drop` to + release transient flist memory. An arena-only flist must be wired + into the cancellation tree explicitly to avoid leaks on SIGINT/abort. +- **INC_RECURSE.** Sibling segments share `first_flist->file_pool` + upstream (`flist.c:2929`). The Rust port must mirror that sharing or + pay re-allocation each segment. + +## 5. Cross-references + +- #1048 - PathBuf overhead per entry; arena work would subsume the + basename allocation savings tracked there. +- `docs/audits/100k-stat-syscall-overhead.md` - related RSS curve at + 100 K+ entries. +- Upstream: `target/interop/upstream-src/rsync-3.4.1/lib/pool_alloc.c`, + `flist.c:1018-1025`, `flist.c:2907-2935`.