valuelog: add sealed mmap byte budget and deny-reason stats#829
valuelog: add sealed mmap byte budget and deny-reason stats#829snissn wants to merge 6 commits intopr/vlog-mmap-telemetry-splitfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a sealed mmap byte budget (default 64 MiB) alongside the existing sealed-segment-count cap, and splits sealed-map-denied telemetry into count-cap vs bytes-cap reasons.
Changes:
- New
MaxMappedSealedBytesconfig (envTREEDB_VLOG_MAX_MAPPED_SEALED_BYTES, default 64 MiB) that denies sealed lazy mmap when cumulative sealed mapped bytes would exceed the budget - Deny-reason enum (
sealedLazyMmapDenyCountCap/sealedLazyMmapDenyBytesCap) with per-reason atomic counters and aggregated stats - Stats export of
sealed_map_denied.count_capandsealed_map_denied.bytes_capin bothdbandcachinglayers
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
TreeDB/internal/valuelog/reader_mmap.go |
Add bytes budget constant/env var, stat the file before lock, split deny counters |
TreeDB/internal/valuelog/manager.go |
Deny-reason enum, bytes accounting in allowSealedLazyMmapLocked, new SealedMapDeniedByReasonStats |
TreeDB/internal/valuelog/manager_test.go |
Tests for byte-budget fallback and per-reason deny stats |
TreeDB/db/api.go |
Export per-reason denied stats in Stats() |
TreeDB/caching/db.go |
Same stats export + whitespace alignment |
💡 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.
|
Follow-up validation on Runs
Comparison vs earlier #827 evidence (same day)Reference runs:
Observed deltas on this branch:
Telemetry noteThe new TreeDB mmap counters are exported through TreeDB stats surfaces, but celestia’s current forensics pipeline does not emit those counters into |
There was a problem hiding this comment.
Pull request overview
Adds a sealed value-log lazy-mmap byte budget (in addition to the existing sealed segment-count budget) and refines telemetry so operators can distinguish whether sealed lazy-mmap was denied due to segment-count vs byte-cap pressure.
Changes:
- Introduces
TREEDB_VLOG_MAX_MAPPED_SEALED_BYTES(default64MiB) and enforces it for sealed (non-current-writable) lazy mmaps. - Splits sealed mmap deny counters into
count_capvsbytes_cap, and exports both viaStats()(db + caching). - Extends valuelog tests to cover byte-budget fallback and per-reason deny aggregation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TreeDB/internal/valuelog/reader_mmap.go | Adds the new env-configured sealed-mmap byte budget and increments per-reason deny counters when mapping is refused. |
| TreeDB/internal/valuelog/manager.go | Adds deny-reason plumbing, aggregates per-reason deny counters, and enforces the sealed mapped-bytes budget. |
| TreeDB/internal/valuelog/manager_test.go | Adds helper + tests validating byte-budget fallback and deny-reason aggregation. |
| TreeDB/db/api.go | Exports new max-bytes config stat and split deny-reason stats for non-caching DB stats. |
| TreeDB/caching/db.go | Exports new max-bytes config stat and split deny-reason stats for caching DB stats. |
Comments suppressed due to low confidence (1)
TreeDB/internal/valuelog/reader_mmap.go:170
- The sealed mmap budgets can be exceeded under concurrent reads: the manager lock is released before
remapToFileSize(), so multiple goroutines can all passallowSealedLazyMmapLocked(seeing the target and other files as still-unmapped) and then mmap in parallel, overshooting both the segment-count and byte budgets. If the intent is to strictly cap sealed residency, consider reserving segments/bytes underm.mu(and releasing the reservation on mmap failure) or otherwise re-checking/enforcing the budget once the mapping is installed.
m.mu.Lock()
allow, denyReason := m.allowSealedLazyMmapLocked(f, targetSize)
m.mu.Unlock()
if !allow {
switch denyReason {
case sealedLazyMmapDenyCountCap:
f.sealedMapDeniedByCount.Add(1)
case sealedLazyMmapDenyBytesCap:
f.sealedMapDeniedByBytes.Add(1)
default:
f.sealedMapDeniedByCount.Add(1)
}
return false
}
f.remapToFileSize()
data, _ := f.mmapData.Load().([]byte)
💡 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
Adds a bytes-based budget for sealed lazy-mmap in the value log and exposes split deny telemetry (count cap vs bytes cap) via TreeDB stats.
Changes:
- Introduce
TREEDB_VLOG_MAX_MAPPED_SEALED_BYTESand enforce it alongside the existing sealed-segment-count cap. - Split sealed mmap deny counters into
count_capvsbytes_capand aggregate/export them in stats. - Add tests covering the new byte-budget behavior and new deny-reason aggregation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TreeDB/internal/valuelog/reader_mmap.go | Adds env-configurable sealed-mmap byte budget and records deny reason counters. |
| TreeDB/internal/valuelog/manager.go | Enforces byte budget in allowSealedLazyMmapLocked and exposes per-reason deny stats. |
| TreeDB/internal/valuelog/manager_test.go | Adds helpers and tests for byte budget + deny-reason aggregation. |
| TreeDB/db/api.go | Exports new sealed-mmap byte budget and per-reason deny stats in DB stats. |
| TreeDB/caching/db.go | Exports new sealed-mmap byte budget and per-reason deny stats in caching DB stats. |
💡 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8c1cbca67
ℹ️ 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 extends TreeDB’s value-log sealed lazy-mmap policy to include a byte-based budget (in addition to the existing sealed-segment-count budget) and exposes deny-reason telemetry split by count-cap vs bytes-cap via Stats.
Changes:
- Add
TREEDB_VLOG_MAX_MAPPED_SEALED_BYTES/MaxMappedSealedBytes(default 64MiB) and enforce it when deciding whether to lazily mmap sealed segments. - Split sealed lazy-mmap deny counters into
count_capvsbytes_cap, and provide an aggregated total for backward compatibility. - Add/extend internal valuelog tests to cover byte-budget fallback behavior and deny-reason aggregation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| TreeDB/internal/valuelog/reader_mmap.go | Adds sealed mapped-bytes budget config parsing and increments deny counters by reason during sealed lazy-mmap enable attempts. |
| TreeDB/internal/valuelog/manager.go | Implements deny-reason enum, byte-budget enforcement, and deny-reason aggregation APIs. |
| TreeDB/internal/valuelog/manager_test.go | Adds helpers and tests for byte-budget fallback and new deny-reason stats aggregation. |
| TreeDB/db/api.go | Exports new byte-budget setting and split deny-reason stats in DB Stats output. |
| TreeDB/caching/db.go | Exports new byte-budget setting and split deny-reason stats in caching DB Stats output. |
💡 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.
Summary
TREEDB_VLOG_MAX_MAPPED_SEALED_BYTES, default 64MiB) alongside the existing sealed-segment-count budgetcount_capvsbytes_capand export both in TreeDB statsWhy
The telemetry split in #828 showed we can now distinguish current-vs-sealed mmap residency. This step uses that signal to bound sealed residency by bytes, not only segment count, so a few large sealed segments cannot inflate file-backed RSS.
Validation
go test ./TreeDB/internal/valueloggo test ./TreeDB/internal/valuelog -run 'SealedLazyMmap|MmapResidencyStatsAggregatesCounters'go test ./TreeDB/db -run TestDoesNotExist -count=1go test ./TreeDB/caching -run TestDoesNotExist -count=1go vet ./TreeDB/internal/valuelog ./TreeDB/db ./TreeDB/caching