valuelog: reuse dst on ReadAppend ReadAt fallback#839
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves hot-path allocation behavior in the value-log reader by ensuring ReadAppend’s ReadAt fallback can reuse caller-provided dst, and adds supporting memory/telemetry and pooling changes across TreeDB components to reduce retained heap and improve observability.
Changes:
- Route
valuelog.File.ReadAppendfallback throughReadAtWithDictToto reusedstand eliminate decode-allocate-copy behavior; add focused tests + fallback benchmark. - Add value-log mmap “sealed lazy mmap” budgets, residency/deny telemetry, and expose new stats/expvar selection for process/cache/mmap-related metrics.
- Reduce memory churn across the system via additional pooling/scratch reuse (zipper apply scratch, leaf build pages), geometric scratch growth, and capping returned
Getbuffers tolen==cap.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
TreeDB/internal/valuelog/manager.go |
Implements ReadAppend fallback dst reuse and adds sealed-lazy-mmap controls + residency/deny stats aggregation. |
TreeDB/internal/valuelog/reader.go |
Adjusts scratch/header pooling to reduce allocs; updates decode helpers accordingly. |
TreeDB/internal/valuelog/reader_mmap.go |
Adds sealed-lazy-mmap enable/refresh logic and new env-configurable budgets. |
TreeDB/internal/valuelog/manager_test.go |
Adds coverage for fallback dst reuse and sealed-lazy-mmap budget behavior. |
TreeDB/internal/valuelog/read_append_fallback_bench_test.go |
Adds a microbenchmark targeting the fallback path allocation profile. |
TreeDB/internal/valuelog/buf.go + buf_test.go |
Adds grow-buffer counters and validates snapshot deltas. |
TreeDB/zipper/zipper.go + zipper_test.go |
Introduces reusable apply/merge scratch and pooled outer-leaf build pages; updates tests for new signatures and adds scratch lifecycle tests. |
TreeDB/node/leaf.go + key_scratch_test.go |
Changes key scratch growth to geometric expansion and adds regression test. |
TreeDB/tree/tree.go + TreeDB/caching/snapshot.go + TreeDB/api_alloc_test.go |
Caps returned Get slices to len==cap to prevent callers from extending into spare capacity; updates tests. |
TreeDB/internal/memtable/* |
Adds value-borrowing support and reset policies (including “hard” reset) with expanded test coverage. |
TreeDB/batch/batch.go + TreeDB/batch/batch_arena_test.go |
Improves arena chunk retention and geometric growth behavior; adds tests. |
TreeDB/caching/* (multiple files) |
Adds/extends memory-pressure behavior and telemetry tests; updates expvar stat selection; adds iterator/rotation regression tests. |
TreeDB/db/api.go |
Exposes new value-log decode-buffer grow + mmap residency/budget stats via DB.Stats(). |
cmd/unified_bench/main.go |
Includes newly exported stats keys in unified benchmark output set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6926c2002
ℹ️ 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 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 address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR primarily targets value-log read performance and heap behavior by ensuring File.ReadAppend’s ReadAt fallback can reuse caller-provided dst, while also introducing broader memory/telemetry and buffer-reuse improvements across TreeDB internals.
Changes:
- Route
ReadAppendfallback reads throughReadAtWithDictToto reusedst, removing decode-allocate-copy fallback behavior and adding focused benchmark/test coverage. - Add/extend mmap residency controls and telemetry (sealed-segment lazy mmap budgets, residency/deny stats), plus additional expvar-eligible stats.
- Reduce retained heap growth via stricter slice capacity semantics (
Get), scratch/buffer pooling improvements, and append-only/batch arena retention/growth policy updates.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/unified_bench/main.go | Adds additional cache/process stat keys to unified benchmark output. |
| TreeDB/zipper/zipper.go | Introduces apply-lifetime scratch reuse (split key arena + page caches), pooled builders, and adjusts merge recursion to pass scratch. |
| TreeDB/zipper/zipper_test.go | Updates zipper tests for new signatures and adds coverage for scratch reuse/trim behavior. |
| TreeDB/tree/tree.go | Caps returned slice capacity in Tree.Get to avoid retaining append-growth capacity. |
| TreeDB/node/leaf.go | Makes ensureKeyScratch grow geometrically to avoid repeated small allocations. |
| TreeDB/node/key_scratch_test.go | Adds test coverage for geometric scratch growth behavior. |
| TreeDB/internal/valuelog/reader_mmap.go | Adds sealed-segment lazy mmap enablement, budgets, out-of-range refresh, and dead-mapped byte accounting. |
| TreeDB/internal/valuelog/reader.go | Reworks decode scratch pooling (holder-based) and pools record header scratch; updates decode path signatures. |
| TreeDB/internal/valuelog/manager.go | Implements ReadAppend fallback dst reuse via ReadAtWithDictTo; adds sealed lazy mmap bookkeeping and aggregated residency/deny stats. |
| TreeDB/internal/valuelog/manager_test.go | Adds tests for fallback dst reuse, lazy mmap budgets, residency stats aggregation, and avoids async remap races in tests. |
| TreeDB/internal/valuelog/read_append_fallback_bench_test.go | Adds benchmark to validate allocation-free fallback behavior (plain + grouped_compressed). |
| TreeDB/internal/valuelog/decode_scratch_pool_test.go | Adds allocation regression test for small scratch reuse after warm-up. |
| TreeDB/internal/valuelog/buf.go | Adds grow-buffer stats counters and snapshot helper for telemetry. |
| TreeDB/internal/valuelog/buf_test.go | Adds tests validating grow-buffer stats snapshots track expected deltas. |
| TreeDB/internal/memtable/memtable.go | Adds ValueBorrower interface for memtables that can retain caller-owned values. |
| TreeDB/internal/memtable/append_only.go | Adds borrow-value write path, hard reset variant, revised growth policy, and improved reset retention/shrink logic. |
| TreeDB/internal/memtable/append_only_test.go | Updates tests for new growth policy and adds test ensuring early growth skips intermediate steps. |
| TreeDB/internal/memtable/append_only_pool_test.go | Adds test ensuring borrowed values aren’t mutated via pooling after reset. |
| TreeDB/internal/memtable/append_only_reset_shrink_test.go | Adds test verifying warm reset retains observed entries within bound. |
| TreeDB/internal/memtable/append_only_reset_hard_test.go | Adds coverage for hard reset dropping observed spike buffers. |
| TreeDB/db/api.go | Exposes decode-buffer grow stats and new vlog mmap residency/budget/deny stats via DB.Stats(). |
| TreeDB/caching/snapshot.go | Caps returned slice capacity in Snapshot.Get for non-empty values. |
| TreeDB/caching/vlog_generation_scheduler_test.go | Adds regression test ensuring forced GC still runs when rewrite planning is canceled. |
| TreeDB/caching/stats_test.go | Adds coverage for adaptive memtable decision override (memtableAdaptiveBTreeMinIters). |
| TreeDB/caching/retained_arena_trim_test.go | Adds tests for trimming retained append-only arenas/leases after flush/checkpoint paths. |
| TreeDB/caching/pool_pressure_test.go | Adds tests for pool-pressure scaling, trimming behavior, and critical-pressure retention skips. |
| TreeDB/caching/memtable_view_telemetry_test.go | Updates tests for deferred telemetry API changes (adds bytes tracking). |
| TreeDB/caching/memtable_view_refs_test.go | Adds test ensuring append-only mem lease cap is enforced. |
| TreeDB/caching/memory_stats_test.go | Adds tests ensuring process memory stats include required breakdown keys and are parseable. |
| TreeDB/caching/iterator_rotation_flush_test.go | Adds tests for iterator-triggered rotations and queue behavior with sharded memtables. |
| TreeDB/caching/iterator_lease_test.go | Extends iterator lease tests to validate deferred-bytes telemetry. |
| TreeDB/caching/expvar_stats.go | Adds expvar publishing/selection logic for filtered/coerced TreeDB stats. |
| TreeDB/caching/expvar_stats_test.go | Adds tests validating expvar stat filtering and type coercion. |
| TreeDB/caching/debug_memtable_rotate.go | Adds gated debug logging for memtable rotations with budget control. |
| TreeDB/caching/debug_memtable_rotate_test.go | Adds tests for debug memtable mode labeling. |
| TreeDB/caching/batch_arena_sizing_test.go | Adds tests for batch arena in-flight bytes and pressure-based clamps. |
| TreeDB/caching/batch_arena_retained_max_test.go | Adds tests validating retained-bytes max accounting helpers. |
| TreeDB/caching/batch_arena_ownership_test.go | Expands ownership/lease behavior tests incl. deferred-pressure steal suppression and hard-cap gating. |
| TreeDB/caching/batch_arena_budget_test.go | Adds extensive tests around hard caps, preflight blocking, draining, and global leased-byte accounting. |
| TreeDB/caching/backpressure_component_test.go | Renames/extends flush test to assert empty units are skipped and non-empty are removed. |
| TreeDB/caching/append_only_hint_test.go | Adds tests for append-only entry hint decay, overflow safety, and pressure-scaled capacity clamping. |
| TreeDB/caching/append_only_direct_writer_pool_test.go | Adds comprehensive tests for append-only direct-writer arena retention/reuse and pressure-aware limits. |
| TreeDB/batch/batch.go | Updates arena chunk reset behavior (keep largest reusable) and implements geometric arena chunk growth. |
| TreeDB/batch/batch_arena_test.go | Adds tests for arena chunk retention and geometric growth. |
| TreeDB/api_alloc_test.go | Extends Get safe-copy test to assert cap == len on returned values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR primarily reduces allocations and improves buffer reuse in value-log read fallback paths, while expanding memory/mmap residency telemetry and adding pooling/pressure-aware behaviors across TreeDB components.
Changes:
- Route
File.ReadAppendfallback reads throughReadAtWithDictToto reuse caller-provideddst, adding targeted tests/benchmarks. - Introduce sealed-segment lazy mmap with configurable budgets and richer mmap residency counters/stats.
- Add/adjust pooling, scratch reuse, and memory-pressure telemetry (batch arena, entry slices, zipper merge scratch, expvar stats).
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/unified_bench/main.go | Adds new metric keys so unified bench output can surface new memory/pool stats. |
| TreeDB/zipper/zipper.go | Introduces apply-lifetime scratch reuse (split keys, page buffers) and pooled builders/pages to cut allocations. |
| TreeDB/zipper/zipper_test.go | Updates call sites for new zipper APIs and adds tests for scratch reuse/trimming. |
| TreeDB/tree/tree.go | Caps Get return slice capacity to length to avoid retaining excess heap. |
| TreeDB/node/leaf.go | Makes keyScratch growth geometric to avoid repeated small allocations. |
| TreeDB/node/key_scratch_test.go | Adds coverage for geometric scratch growth behavior. |
| TreeDB/internal/valuelog/reader.go | Reworks decode scratch/header pooling and routes decoding-to-dst via pooled header scratch. |
| TreeDB/internal/valuelog/reader_mmap.go | Adds sealed lazy mmap enablement, out-of-range refresh, and budgets via env vars. |
| TreeDB/internal/valuelog/manager.go | Adds sealed mmap budgeting logic, residency/deny stats, ReadAppend ReadAt-to-dst fallback helper, and grouped cache defaults changes. |
| TreeDB/internal/valuelog/manager_test.go | Adds tests for residency stats aggregation, lazy mmap budgets, ReadAppend dst reuse, and removes async-remap close races. |
| TreeDB/internal/valuelog/read_append_fallback_bench_test.go | Adds benchmark focused on ReadAppend ReadAt fallback allocation behavior. |
| TreeDB/internal/valuelog/decode_scratch_pool_test.go | Adds allocation test ensuring small scratch reuse is alloc-free after warmup. |
| TreeDB/internal/valuelog/buf.go | Adds atomic instrumentation for grow() to expose buffer growth behavior in stats. |
| TreeDB/internal/valuelog/buf_test.go | Adds tests validating GrowBufferStatsSnapshot deltas. |
| TreeDB/internal/memtable/memtable.go | Adds ValueBorrower interface for memtables that can retain caller-owned values. |
| TreeDB/internal/memtable/append_only.go | Adds borrowed-value write path, changes growth policy, and adds hard reset + retention policy logic. |
| TreeDB/internal/memtable/append_only_test.go | Updates capacity growth tests and adds inline growth behavior test. |
| TreeDB/internal/memtable/append_only_reset_shrink_test.go | Adds test ensuring warm reset retains observed entries within bounds. |
| TreeDB/internal/memtable/append_only_reset_hard_test.go | Adds test for hard reset dropping spikes and clearing retained arena. |
| TreeDB/internal/memtable/append_only_pool_test.go | Adds test ensuring borrowed values aren’t reused/mutated via pooling. |
| TreeDB/db/api.go | Exposes new grow/mmap residency/deny stats through DB Stats(). |
| TreeDB/api_alloc_test.go | Extends Get safe-copy test to assert capacity is capped to length. |
| TreeDB/caching/snapshot.go | Caps Snapshot.Get returned slice capacity to length. |
| TreeDB/caching/snapshot_test.go | Updates test to use new memtable mode storage helper. |
| TreeDB/caching/reverse_iterator_parity_test.go | Updates test to use new memtable mode storage helper. |
| TreeDB/caching/stats_test.go | Updates adaptive stats DB initialization and adds BTree min-iterator override test. |
| TreeDB/caching/vlog_generation_scheduler_test.go | Adds test asserting forced GC still runs after rewrite-plan cancellation. |
| TreeDB/caching/retained_arena_trim_test.go | Adds tests validating retained arena trimming behavior and pool returns. |
| TreeDB/caching/pool_pressure_test.go | Adds extensive tests for pressure-aware budgets, trimming, and threshold scaling. |
| TreeDB/caching/memtable_view_telemetry_test.go | Updates deferred enter signature and adds assertions for deferred-bytes telemetry. |
| TreeDB/caching/memtable_view_refs_test.go | Adds test ensuring append-only mem-lease pool cap is enforced. |
| TreeDB/caching/memory_stats_test.go | Adds coverage ensuring runtime/process memory stats are exported and parseable. |
| TreeDB/caching/iterator_rotation_flush_test.go | Adds tests for iterator-triggered rotations enqueuing/triggering flush correctly. |
| TreeDB/caching/iterator_lease_test.go | Extends iterator lease tests to include deferred-bytes telemetry. |
| TreeDB/caching/expvar_stats.go | Adds expvar publishing/selection with coercion and env toggle. |
| TreeDB/caching/expvar_stats_test.go | Adds tests for expvar stat selection/filtering/coercion. |
| TreeDB/caching/debug_memtable_rotate.go | Adds env-driven debug logging for memtable rotation behavior. |
| TreeDB/caching/debug_memtable_rotate_test.go | Adds tests for memtable mode label mapping. |
| TreeDB/caching/batch_arena_sizing_test.go | Adds tests for in-flight bytes tracking and pressure clamping of arena sizing/growth. |
| TreeDB/caching/batch_arena_retained_max_test.go | Adds tests for retained-bytes max tracking helpers. |
| TreeDB/caching/batch_arena_ownership_test.go | Updates ownership tests for borrowed semantics, adds hard-cap and deferred-pressure suppression tests. |
| TreeDB/caching/batch_arena_budget_test.go | Adds tests for pool draining, hard-cap enforcement, tail compaction, and global leased-bytes tracking. |
| TreeDB/caching/backpressure_component_test.go | Renames/extends test to validate empty rotates don’t leave queue units; non-empty rotates do. |
| TreeDB/caching/append_only_hint_test.go | Adds tests for append-only entry hint decay, overflow safety, and pressure-scaled thresholds. |
| TreeDB/caching/append_only_direct_writer_pool_test.go | Adds large suite of tests for append-only direct-writer arena retention/reuse/pressure behavior. |
| TreeDB/batch/batch.go | Improves arena reset to keep the largest reusable chunk and adds geometric growth for arena chunks. |
| TreeDB/batch/batch_arena_test.go | Adds tests for arena reset retention and geometric growth behavior. |
Comments suppressed due to low confidence (2)
TreeDB/zipper/zipper.go:1
- When
cap(s.outerLeafBuildPages)exceeds the keep cap,reset()drops the entire slice without returning the already-cached*outerLeafBuildPageentries toouterLeafBuildPagePool. This prevents those page buffers from being recycled and can increase allocation/GC pressure after a transient spike. A concrete fix is to iterate overs.outerLeafBuildPages(up to its currentlen) andputOuterLeafBuildPageeach non-nil page before replacing the slice.
TreeDB/internal/valuelog/buf.go:1 - This adds multiple atomic increments to every
grow()call. Ifgrow()is on a hot path (e.g., decode/append operations at high QPS), this can measurably degrade throughput due to contended atomics and cache-line bouncing. Consider making this instrumentation conditional (e.g., behind a package-level enabled flag with a fast check, build tag, or sampling) so production deployments can disable it when not actively diagnosing buffer growth behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bc328253d
ℹ️ 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 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 address that feedback".
Summary
File.ReadAppendReadAt fallback throughReadAtWithDictToso caller-provided dst can be reusedValidation
go test ./TreeDB/internal/valuelog -run 'TestFileReadAppend_CountsGroupedFallbackReadAt|TestFileReadAppend_FallbackReusesDst|TestValueLogManager_MmapReadAppend|TestValueLogManager_MmapReadAppendCompressedGroupedCache' -count=1go test ./TreeDB/internal/valuelog -bench '^BenchmarkFileReadAppend_Fallback$' -benchmem -run '^$' -count=1Benchmark signal
BenchmarkFileReadAppend_Fallback/grouped_compressed: now 0 alloc/op on this fallback pathBenchmarkFileReadAppend_Fallback/plain: remains allocation-free (neutral)