Skip to content

test(benchmark): resolve smaller effects in memory.sh - #453

Merged
rmanibus merged 1 commit into
mainfrom
claude/clever-matsumoto-33c070
Aug 4, 2026
Merged

test(benchmark): resolve smaller effects in memory.sh#453
rmanibus merged 1 commit into
mainfrom
claude/clever-matsumoto-33c070

Conversation

@rmanibus

@rmanibus rmanibus commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Take SAMPLES repetitions per (operation, size) point (default 3, overridable like SIZES/OUT/KEEP) and report the median with its min–max band, so a noisy point is visible instead of hidden behind one measurement.
  • Report cumulative allocated bytes alongside peak RSS. Peak RSS is a high-water mark and is blind to allocation the collector reclaims; a new -memstats <path> flag (cmd/cloudstic/profiling.go) dumps runtime.MemStats.TotalAlloc from inside the measured process — exact and free, unlike a sampled -alloc_space heap profile, which would land a small delta inside its own sampling error and perturb the RSS measured in the same pass.
  • internal/benchreport grows an alloc_mb CSV column and a Stat (median/min/max) reduction shared by both harnesses; render() gets a second "Total allocated" table for the scaling report and an Allocated column for the single-tool comparison table. cloudstic.sh's multi-tool comparison is unaffected — it never populates the column, so nothing renders for it.
  • memory.sh gains BENCH_CLOUDSTIC_BIN to point the sweep at a prebuilt binary, since comparing two commits means measuring with one harness rather than checking out the other commit (which would also swap the harness doing the measuring).

Closes nothing on its own; motivated by the evidence in #449, where the memory sweep reported peak RSS moving 5 MB against a measured ±60 MB run-to-run spread for a change that provably removed 36 MB of allocation from CompactCatalog.

Verification

I built main (1ca7e68) and its parent (ee61760, before #449) and pointed the sweep at each via BENCH_CLOUDSTIC_BIN. At 5,000–20,000 files most operations already show a tight band (peak RSS spread under ~10 MB, allocation spread under ~2 MB across 3–5 samples). For prune specifically at 20,000 files, 5 samples gave:

peak RSS allocated
before (ee61760) 163.1–171.8 MB 340.2 MB (all 5 samples identical to 0.1 MB)
after (1ca7e68) 154.8–163.8 MB 324.9–325.0 MB

The allocated column resolves a clean ~15 MB signal with an essentially zero noise band, where peak RSS alone (spread ~9 MB against a ~16 MB delta) is borderline. I also isolated the CompactCatalog delta itself with go tool pprof -alloc_space -diff_base against byte-identical repository copies built with each binary, which attributed −11.08 MB (the removed catalog copy) and −5.43 MB (the removed extra marshal) to that function specifically, confirming the mechanism the PR describes.

One caveat worth recording: at 50,000 files, prune's total allocation is dominated by PackStore.Repack's mark-phase pack-cache behavior, whose cost varies by up to several GB between runs — traced to Go's randomized map iteration order feeding a bounded pack cache, unrelated to this PR's code path. That's a pre-existing, independent source of noise at that scale; I didn't attempt to fix it here, and picked 20,000 files for the demonstration above because it isn't dominated by that effect.

env GOCACHE=/tmp/cloudstic-gocache go test -race -count=1 ./internal/benchreport/... ./internal/cmd/benchreport/... ./cmd/cloudstic/...
env GOCACHE=/tmp/cloudstic-gocache GOLANGCI_LINT_CACHE=/tmp/cloudstic-golangci-lint golangci-lint run ./internal/benchreport/... ./internal/cmd/benchreport/... ./cmd/cloudstic/...
shellcheck -x scripts/benchmark/memory.sh
SIZES="5000 20000" SAMPLES=3 ./scripts/benchmark/memory.sh   # ran clean under /bin/bash (3.2) and the Homebrew bash (5.3)
go run ./internal/cmd/benchreport render -in benchmark-results/memory.csv -title "Peak memory vs repository size"

Summary by CodeRabbit

  • New Features

    • Benchmark reports now include cumulative memory allocation metrics alongside peak memory measurements.
    • Added statistical aggregation with median, minimum, and maximum values for benchmark samples.
    • Extended profiling support for detailed memory statistics collection.
  • Documentation

    • Enhanced benchmark documentation explaining allocation measurement methods and their interpretation.

Peak RSS alone couldn't tell PR #449's 36 MB CompactCatalog reduction
from noise: it moved peak RSS by 5 MB against a measured ±60 MB
run-to-run spread. Two changes fix that.

- Take SAMPLES repetitions per (operation, size) point (default 3,
  overridable) and report the median with its min-max band, so a noisy
  point is visible rather than hidden.
- Report cumulative allocated bytes alongside peak RSS, via a new
  -memstats flag that dumps runtime.MemStats.TotalAlloc from inside the
  measured process. Peak RSS is a high-water mark and can't see churn
  the collector reclaims; allocation is the column that moves when an
  operation stops allocating something it didn't need.

benchreport's CSV/report layer grows the alloc_mb column and a Stat
(median/min/max) aggregation shared by both harnesses; cloudstic.sh's
comparison table is unaffected since it never populates the new column.
memory.sh also gains BENCH_CLOUDSTIC_BIN to point the sweep at a
prebuilt binary, needed to compare two commits without swapping out the
harness doing the measuring.

Verified against #449 by building main and its parent and pointing the
sweep at each: at 20,000 files prune's allocated total is 340.2 MB
(main) vs 324.9-325.0 MB (with the fix) across 5 samples with an
essentially zero band, a clean 15 MB signal that peak RSS alone left
borderline.
@rmanibus rmanibus added test area/cli Core CLI command surface and UX benchmark Run the memory-scaling benchmark on this PR labels Aug 4, 2026
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d6d5fdd2-7c1c-452f-a4c7-8312a9e4e2dc

📥 Commits

Reviewing files that changed from the base of the PR and between 219ebab and e159ab0.

📒 Files selected for processing (10)
  • AGENTS.md
  • cmd/cloudstic/main.go
  • cmd/cloudstic/profiling.go
  • internal/benchreport/csv.go
  • internal/benchreport/measure.go
  • internal/benchreport/render.go
  • internal/benchreport/report.go
  • internal/benchreport/report_test.go
  • internal/cmd/benchreport/main.go
  • scripts/benchmark/memory.sh

📝 Walkthrough

Walkthrough

The benchmark pipeline now records cumulative allocation with peak RSS, supports repeated samples, aggregates median and min–max statistics, and renders allocation data in CSV, tables, and charts.

Changes

Memory benchmark metrics

Layer / File(s) Summary
Profiling and allocation collection
cmd/cloudstic/..., internal/cmd/benchreport/main.go, internal/benchreport/measure.go
Added -memstats and -alloc-from. The commands write, read, validate, and report cumulative allocation values.
Sample schema and aggregation
internal/benchreport/report.go, internal/benchreport/csv.go, internal/benchreport/report_test.go
Benchmark rows and CSV records now retain optional allocation values. Repeated samples aggregate into median, minimum, maximum, and count statistics.
Report and chart rendering
internal/benchreport/render.go, internal/benchreport/report_test.go
Tables and charts use median values and display allocation metrics when available.
Repeated benchmark execution and documentation
scripts/benchmark/memory.sh, AGENTS.md
The memory benchmark supports validated repeated samples, per-sample allocation collection, prebuilt executables, and updated usage documentation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Cloudstic
  participant MemstatsFile
  participant Benchreport
  participant Report
  Cloudstic->>MemstatsFile: Write runtime.MemStats
  Benchreport->>MemstatsFile: Read total_alloc_bytes
  Benchreport->>Report: Append AllocMB to benchmark row
  Report->>Report: Aggregate repeated samples
  Report->>Report: Render median and min–max values
Loading
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/clever-matsumoto-33c070

Comment @coderabbitai help to get the list of available commands.

@rmanibus
rmanibus merged commit 2f487c3 into main Aug 4, 2026
19 of 24 checks passed
@rmanibus
rmanibus deleted the claude/clever-matsumoto-33c070 branch August 4, 2026 15:06
@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli Core CLI command surface and UX benchmark Run the memory-scaling benchmark on this PR test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant