Skip to content

test(net): make TCP loopback benchmarks comparable - #1146

Merged
Coldwings merged 5 commits into
mainfrom
fix/1145-fair-tcp-benchmark
Aug 24, 2026
Merged

test(net): make TCP loopback benchmarks comparable#1146
Coldwings merged 5 commits into
mainfrom
fix/1145-fair-tcp-benchmark

Conversation

@Coldwings

Copy link
Copy Markdown
Owner

Description

Replace the inequivalent TCP loopback adapters with one fixed-work wire
protocol and separately attributable client-side and server-side comparisons.
This prevents batching, overlapping composed writes, synthetic IOPS, or
per-runtime peer differences from being presented as runtime performance.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation (changes to documentation, comments, or examples)
  • Tests (adding or modifying tests)
  • Build/CI (changes to build system, CI configuration, or dependencies)

Related Issues

Closes #1145

Changes Made

Core Changes

  • Define shared latency, message-rate, and bulk records with fixed work,
    drained warmup, exact sequence/payload validation, and one logical write at
    a time.
  • Compare Elio, libuv, and Asio clients against one POSIX reference server,
    and compare their servers with one POSIX reference client. A representative
    3x3 cross-runtime matrix remains a conformance check only.
  • Emit versioned client/driver JSON plus server-observed connection evidence,
    validate both against the scheduled-work manifest, and preserve commands,
    logs, binary hashes, revision, and environment metadata.
  • Add a dedicated-host comparison runner with seeded balanced blocks, unique
    trial/evidence linkage, paired bootstrap intervals, reference-peer headroom,
    and fail-closed publication qualification for both attribution axes.
  • Make GitHub-hosted workflows performance-neutral: CI validates protocol and
    accounting but publishes no ranking or percentage-level performance gate.

API Changes

None. These are optional example/benchmark targets and supporting CI tooling.

Testing

Unit Tests

  • Added new tests for the changes
  • Updated existing tests if needed
  • All focused tests pass locally

Integration Tests

  • Built all eight client, server, and reference targets with developer
    warnings treated as errors
  • Passed all 63 client/reference, server/reference, and cross-runtime
    conformance cases
  • Passed two independent 36-trial controlled-runner smoke matrices; every
    raw result and server evidence record linked by one unique wire trial ID

Sanitizer Testing

  • Tested with ASAN (AddressSanitizer)
  • Tested with TSAN (ThreadSanitizer)
  • No new warnings or errors in the focused TSAN reference driver run

Test Results

[bench][tcp]: 15 test cases, 80 assertions passed
Conformance matrix: 63/63 cases passed
Evidence: 63 performance/driver records and 63 server-observed records
Comparison smoke: 36/36 trials and 36/36 linked server evidence records
TSAN: POSIX reference client against Asio server, 100 warmup + 5000 measured, clean

Checklist

Code Quality

  • My code follows the project's code style
  • I have added/updated comments only for non-obvious constraints
  • I have removed debug code, TODOs, and commented-out code
  • My changes generate no new warnings

Documentation

  • I have updated the README, wiki, and changelog
  • I have documented both client-runtime and server-runtime comparisons
  • I have documented the controlled-runner requirements

Testing

  • I have added tests that prove the fix is effective
  • New focused unit tests pass locally
  • I have run focused TSAN validation

Compatibility

  • My changes are backward compatible
  • I have considered the impact on existing users
  • I have updated CHANGELOG.md

Performance

  • I have considered the performance and measurement impact
  • The replacement benchmark measures both runtime sides without
    attributing self-pair costs to one side

Additional Notes

The timings produced by the public conformance workflow are deliberately not
ranked. The separate comparison runner publishes ratios only when the run has
at least 18 balanced blocks, sufficiently long measured phases, a clean and
matched build revision, disjoint non-SMT CPU affinity, an explicit dedicated
host assertion, and unsaturated reference-peer evidence.

Reviewer Guidance

Please focus on the timing boundary, single-writer/credit invariants, unique
server evidence reconciliation, reference-peer headroom qualification, paired
statistics, and the distinction between adapter and driver counter scopes.

Copilot AI balanced review requested due to automatic review settings August 24, 2026 05:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Elio-only polling can contaminate comparisons, and several publication-qualification guarantees are not consistently enforced or documented.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Reworks TCP loopback benchmarks into fixed-work, separately attributable client/server comparisons.

Changes:

  • Adds shared protocol validation and reference peers.
  • Adds conformance and controlled-performance runners.
  • Updates tests, CI, and performance documentation.
File summaries
File Description
.github/workflows/benchmark.yml Runs conformance-only benchmarks.
.github/workflows/ci.yml Builds and validates benchmark targets.
CHANGELOG.md Records the benchmark redesign.
README.md Documents benchmark workflows.
examples/CMakeLists.txt Adds client, server, and reference targets.
examples/bench_tcp_asio.cpp Implements the Asio client adapter.
examples/bench_tcp_asio_server.cpp Adds the Asio server adapter.
examples/bench_tcp_common.hpp Defines protocol, counters, and output.
examples/bench_tcp_elio.cpp Implements the Elio client adapter.
examples/bench_tcp_elio_server.cpp Adds the Elio server adapter.
examples/bench_tcp_libuv.cpp Implements the libuv client adapter.
examples/bench_tcp_libuv_server.cpp Adds the libuv server adapter.
examples/bench_tcp_reference.cpp Adds the POSIX reference server.
examples/bench_tcp_reference_client.cpp Adds the POSIX reference client.
examples/bench_tcp_server_protocol.hpp Shares server validation and evidence.
tests/unit/test_bench_tcp_common.cpp Expands protocol and invariant tests.
tools/run-tcp-benchmark-conformance.py Runs the conformance matrix.
tools/run-tcp-performance-comparison.py Runs controlled paired comparisons.
wiki/Performance-Tuning.md Documents controlled benchmarking.
Review details
  • Files reviewed: 19/19 changed files
  • Comments generated: 6
  • Review effort level: Balanced

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

Comment thread examples/bench_tcp_elio.cpp Outdated
Comment thread README.md Outdated
Comment thread tools/run-tcp-benchmark-conformance.py Outdated
Comment thread examples/bench_tcp_elio_server.cpp Outdated
Comment thread wiki/Performance-Tuning.md Outdated
Comment thread README.md Outdated
@Coldwings

Copy link
Copy Markdown
Owner Author

@copilot please re-review only; do not make direct code modifications.

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot please re-review only; do not make direct code modifications.

Re-reviewed the current branch without making code changes.

I still see the same issues in the current tip (8236aaa):

  • Elio client/server examples still use polling sleep loops in examples/bench_tcp_elio.cpp and examples/bench_tcp_elio_server.cpp.
  • --build-metadata examples still point to a repo-root path in README.md and wiki/Performance-Tuning.md.
  • Per-case JSONL output in tools/run-tcp-benchmark-conformance.py is still written before performance_eligible=false is added.
  • README still states a fixed 250 ms floor while the runner accepts configurable minimum measured duration.

@Coldwings

Copy link
Copy Markdown
Owner Author

@copilot please re-review only; do not make direct code modifications.

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot please re-review only; do not make direct code modifications.

Re-reviewed only (no direct code changes from this pass).

Current tip (5463e32) appears to address the previously flagged points: polling loops were replaced with atomic wait/notify, metadata examples now point outside the worktree, per-case JSONL is rewritten with enriched ineligible records, and the README now states the 250 ms floor is hard (config can only raise it).

@Coldwings

Copy link
Copy Markdown
Owner Author

@copilot please re-review only; do not make direct code modifications.

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot please re-review only; do not make direct code modifications.

Re-reviewed only; no direct code modifications in this pass.

Current tip (c0233c3) still looks consistent with the requested fixes and with the prior re-review: main-thread polling removed in Elio client/server benchmarks, metadata paths are outside the worktree in docs, conformance output copies are enriched/checked for performance_eligible=false, and the 250 ms publication floor is enforced.

@Coldwings

Copy link
Copy Markdown
Owner Author

@copilot please re-review only; do not make direct code modifications.

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@copilot please re-review only; do not make direct code modifications.

Re-reviewed only; no direct code modifications in this pass.

Current tip (30aa8eb) still aligns with the review fixes: completion is observable in Elio client/server benchmarks without polling, docs keep metadata outside the worktree, conformance artifacts are enriched and checked for performance_eligible=false, and the 250 ms publication floor remains enforced.

@Coldwings

Copy link
Copy Markdown
Owner Author

Additional validation for the review fixes:

  • Elio client/server benchmark targets rebuild successfully.
  • The deterministic scheduler admission-rejection regression passes (12 assertions).
  • Client connection failure and server bind failure both terminate with status 1 without hanging.
  • The full 63-case conformance matrix passes with 63 linked server-evidence records; all 189 retained result copies are explicitly performance-ineligible.
  • A 36-trial two-axis comparison smoke run has unique linked server evidence and suppresses medians, ratios, and confidence intervals while unqualified.
  • An independent read-only review found no remaining blocker.

@Coldwings
Coldwings merged commit 0fd0b74 into main Aug 24, 2026
11 checks passed
@Coldwings
Coldwings deleted the fix/1145-fair-tcp-benchmark branch August 24, 2026 06:04
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.

[Tests] TCP loopback benchmark compares non-equivalent workloads

3 participants