Skip to content

ci: add epoch boundary time to the PR benchmark comment - #11820

Merged
kbhargava-jump merged 2 commits into
mainfrom
kbhargava/bench-epoch-boundary
Oct 28, 2026
Merged

kbhargava-jump merged 2 commits into
mainfrom
kbhargava/bench-epoch-boundary

Conversation

@kbhargava-jump

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI balanced review requested due to automatic review settings September 30, 2026 22:47
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
 ┌─ ⚡ PERF · 4ac6c5f vs main@f9d782e ─────────────────────────────────
 │ SUITE                               BASELINE          NEW         Δ
 │ replay tps, mainnet               13,219 tps   13,161 tps  ·  -0.44%
 │ bench tps, localnet              669,231 tps  667,379 tps  ·  -0.28%
 │ snapshot load, testnet                4.95 s       4.95 s  ·   0.00%
 │ epoch boundary, mainnet             161.5 ms     160.3 ms  ·  -0.76%
 │ mem total, mainnet                148.35 GiB   148.35 GiB  ·   0.00%
 │ mem total, testnet                 85.66 GiB    85.66 GiB  ·   0.00%
 │ clean compile, firedancer             3.15 s       3.15 s  ·  -0.20%
 │ binary size, firedancer             57.93 MB     57.93 MB  ·   0.00%
 ├─────────────────────────────────────────────────────────────────────
@@ 0 REGRESSIONS · 0 WARNINGS · 0 IMPROVED · 8 NOISE @@
 └─────────────────────────────────────────────────────────────────────
history · 4 pushes
 ┌─ HISTORY · Δ vs main, per push, newest first ──────────────────────────────────
 │ HEAD         TPS    BENCH     SNAP    EPOCH    MEM·M    MEM·T  COMPILE   BINARY
 │ 4ac6c5f   -0.44%   -0.28%    0.00%   -0.76%    0.00%    0.00%   -0.20%    0.00%
 │ fc264e1   -0.15%   -0.31%   -0.05%   -0.65%    0.00%    0.00%   -0.28%    0.00%
+│ a458607   +0.27%   +0.36%   +0.03%  -12.63%    0.00%    0.00%        …    0.00%
+│ 9667125   -0.23%   +0.02%   -6.09%   -1.22%    0.00%    0.00%   +2.84%    0.00%
 └────────────────────────────────────────────────────────────────────────────────

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Epoch results incorrectly use snapshot-run disk skew when classifying benchmark noise.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds epoch-boundary runtime timing to PR benchmark reporting.

Changes:

  • Runs a minimized epoch-crossing ledger benchmark.
  • Parses and displays epoch-boundary timing.
  • Adds the metric to benchmark history.
File Description
.github/​workflows/​on_pull_request.yml Supplies the epoch ledger.
.github/​workflows/​benchmark.yml Runs epoch benchmarks for baseline and PR.
contrib/​ci/​bench/​bench.sh Adds the epoch benchmark mode.
contrib/​ci/​bench/​benchmark_comment.py Parses and renders epoch timing.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread contrib/ci/bench/benchmark_comment.py Outdated
@kbhargava-jump
kbhargava-jump enabled auto-merge (squash) September 30, 2026 22:50
ripatel-fd
ripatel-fd previously approved these changes Sep 30, 2026

@mmcgee-jump mmcgee-jump left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id render the row in milliseconds, not seconds

Copilot AI balanced review requested due to automatic review settings September 30, 2026 22:52
@ripatel-fd

Copy link
Copy Markdown
Contributor

@copilot how long does this bench job take compared to bench runs on other PRs? respond with number of seconds and confidence range

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The new benchmark is consistently wired through collection, parsing, workflow configuration, and reporting.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Copilot AI commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

@copilot how long does this bench job take compared to bench runs on other PRs? respond with number of seconds and confidence range

Measured from GitHub Actions job timings. This PR has two runs (36787575349, 36788019857); I compared them with 16 recent successful On Pull Request runs from other PRs (2026-09-30).

bench-snapshot job, which now also runs the epoch steps:

  • This PR: 101 s and 116 s (mean ~108 s).
  • Other PRs: 77 ± 5 s (mean ± 1 SD, range 66–84 s).
  • Difference: about +35 s, likely between +30 and +45 s. The spread between the two job totals is too wide for a useful confidence interval with n=2, so I based the estimate on the added steps. epoch base + epoch new took 38 s (23 + 15) and 36 s (23 + 13). The snapshot steps took 30–34 s, the same as on other PRs.

Total benchmark time (the slowest of the three parallel jobs):

  • This PR: 101 s and 116 s.
  • Other PRs: 89 ± 3.5 s (range 83–94 s).
  • Difference: about +20 s, likely between +12 and +27 s. Before this PR, bench-replay (83 ± 7 s) and bench-tps (86 ± 7 s) were the slowest jobs. bench-snapshot is now the slowest, so only part of its added time lengthens the total.

The benchmark jobs have a 360 s timeout (6 min), so ~116 s leaves plenty of headroom.

@kbhargava-jump

kbhargava-jump commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

@copilot how would the time differ if we remove download time for the actual new epoch boundary ledger that was added in this PR

1 similar comment
@ripatel-fd

Copy link
Copy Markdown
Contributor

@copilot how would the time differ if we remove download time for the actual new epoch boundary ledger that was added in this PR

Copilot AI commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

@copilot how would the time differ if we remove download time for the actual new epoch boundary ledger that was added in this PR

Without the download, the epoch steps cost about 27 s instead of about 37 s. I split the epoch base step logs from both runs; they ran on runners ci1 and ci2, and neither had the ledger yet:

part of epoch base run 1 (ci1) run 2 (ci2) one-time?
quiesce (drop_caches, pre-read, fstrim) 8.6 s 9.1 s no
gcloud storage cat + untar of mainnet-epoch-450144000-minimized.tar.gz 8.0 s 7.9 s yes
build blockstore2shredcap + rocksdb→shredcap conversion 1.7 s 1.7 s yes
backtest (1.0 s snapshot load + 2.8–3.3 s replay) 4.3 s 4.8 s no
step total 22.6 s 23.6 s

epoch new never downloads (10.8 s + 4.1 s and 8.1 s + 4.5 s).

The ledger is kept in ../dump and reused if it matches its manifest; the converted shreds.pcapng.zst is kept too. So on each runner, only the first bench-snapshot job pays for the download and conversion. That was 9.7 s ± 0.1 s in both runs.

Without that one-time cost:

  • epoch base + epoch new: about 27 s (26.5 s and 26.6 s), instead of 36–38 s. Both steps take about 13 s; roughly 9 s of each is quiesce, not replay.
  • bench-snapshot job: about 104 s, likely 97–112 s, compared with 77 ± 5 s on other PRs, so about +27 s. That is 77 ± 5 s plus the 27 s of epoch steps. Subtracting the download from the two measured runs gives 91 s and 106 s. In run 2, the hugepages step took 11 s versus 0 s in run 1, as it sometimes does on other PRs too.
  • Total benchmark time (slowest job): about +15 s, likely +8 to +23 s, compared with 89 ± 3.5 s today. bench-snapshot would still be the slowest job, just behind bench-tps (86 ± 7 s).

The download is repeated whenever a runner's ../dump is wiped or the ledger name changes. The 9.7 s is the price each time.

@copilot how would the time differ if we remove download time for the actual new epoch boundary ledger that was added in this PR

Answered in the reply to the identical question just below, with the full breakdown. In short: the download plus rocksdb→shredcap conversion costs about 9.7 s, and only once per runner, because the ledger stays cached in ../dump. Without it, the epoch steps add about 27 s to bench-snapshot (about 104 s versus 77 ± 5 s) and about 15 s (likely +8 to +23 s) to the total benchmark time.

ripatel-fd
ripatel-fd previously approved these changes Oct 5, 2026
Times fd_runtime_process_new_epoch() on mainnet-epoch-boundary-461376000,
a minimized mainnet ledger crossing the epoch 1067 -> 1068 boundary at
slot 461376000, in the snapshot job.
@kbhargava-jump
kbhargava-jump force-pushed the kbhargava/bench-epoch-boundary branch from a458607 to fc264e1 Compare October 28, 2026 21:55
Copilot AI balanced review requested due to automatic review settings October 28, 2026 21:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new epoch-ledger input can select ledgers beyond the fixed replay ceiling, but neither workflow entry point allows that ceiling to be adjusted, causing later-epoch benchmarks to fail with no slots to replay.

1 open issue

Review effort: Balanced

Comment thread contrib/ci/bench/bench.sh Outdated
A fixed end slot only fits the default ledger; one around a later
boundary has no slots below it.  Every epoch ledger ends a few slots
past its boundary, so replaying all of it costs ~1 s.
Copilot AI balanced review requested due to automatic review settings October 28, 2026 22:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues are identified for the epoch-boundary benchmark reporting changes.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@kbhargava-jump
kbhargava-jump merged commit ac69ffd into main Oct 28, 2026
19 checks passed
@kbhargava-jump
kbhargava-jump deleted the kbhargava/bench-epoch-boundary branch October 28, 2026 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants