feat(bench): add concurrent initialization benchmark harness - #239
feat(bench): add concurrent initialization benchmark harness#239om7057 wants to merge 6 commits into
Conversation
Adds a benchmark suite to measure and analyze CUDA initialization latency in HAMi-core's multiprocess device-sharing layer. Addresses issue #1662. Tools included: - bench_init: Concurrent fork+exec benchmark across N worker processes, measuring cuInit() latency distribution and percentiles (p50, p90, p99). - phase_probe: Breaks down set_task_pid() cost into phases (nvmlInit, snapshots, primary context retain/release). - nested_retain: Tests whether holding a context across multiple retains is amortizable (~1µs per retain once context exists vs ~31ms cold). - warm_holder: Background context holder for testing whether other processes' context probe cost changes when a context is already held. - abi_check: Verifies struct layout consistency (sizeof/offsetof) to ensure Go-side mirror struct wire format stays compatible. Also includes: - Makefile: Builds all tools with configurable CUDA_HOME (default: /home/muon/cuda-headers). - run_benchmarks.sh: Orchestrates the full benchmark suite and produces the measurements from #1662. - README.md: Documents each tool's purpose, usage, and expected results. Binaries are .gitignore'd; source files carry Apache 2.0 license headers. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: om7057 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesThe PR adds a CUDA benchmark suite with configurable builds, concurrent initialization timing, context and NVML probes, an automated runner, ABI checks, and documentation. CUDA benchmark suite
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Runner as run_benchmarks.sh
participant Phase as phase_probe
participant Retain as nested_retain
participant Holder as warm_holder
participant Init as bench_init
participant ABI as abi_check
Runner->>Phase: measure CUDA and NVML phases
Runner->>Retain: measure nested primary-context retains
Runner->>Holder: start persistent context holder
Runner->>Init: run concurrent initialization at process counts
Runner->>Holder: terminate background holder
Runner->>ABI: print shared-region layout
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: om7057 <kulkarniom7057@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/abi_check.c`:
- Around line 24-29: Update bench/abi_check.c in main to compare
sizeof(shared_region_t), the limit, sm_limit, and procs offsets, and limit[0]
size against the generated or declared Go-mirror expectations; report mismatches
and return a nonzero status when any comparison fails. Update bench/README.md
lines 42-43 to accurately describe the automated cross-language ABI comparison
and failure behavior.
In `@bench/bench_init.c`:
- Around line 99-105: Update the index calculation in pct to use nearest-rank
semantics: compute ceil(p * n / 100.0) - 1, then clamp the result to the range
[0, n - 1] before indexing sorted. Preserve the existing empty-input behavior.
- Around line 225-231: Align the percentile contract across the benchmark output
and documentation: in bench/bench_init.c lines 225-231, choose and consistently
emit either p90 or p95; in bench/run_benchmarks.sh line 51, filter for that
emitted percentile; and in bench/README.md line 14, document the same percentile
set. Preserve the existing JSON field naming and percentile calculation
structure.
- Around line 153-154: Update the worker-slot initialization around sh->ready
and the startup/readiness logic to set each slot’s result to a non-success
sentinel, detect and reap children that exit before signaling readiness, and
enforce a bounded wait instead of spinning indefinitely. During final
aggregation, include only workers that exited successfully and wrote a valid
result, excluding failed or prematurely terminated children from success counts
and timing totals.
In `@bench/Makefile`:
- Around line 1-4: Update the CUDA_HOME definition in the Makefile so it no
longer defaults to the author-specific /home/muon/cuda-headers path; use a
conventional CUDA installation default such as /usr/local/cuda or require and
validate CUDA_HOME explicitly, while preserving the existing INCLUDES and LIBS
references.
- Around line 10-12: Expose the Makefile’s NVML availability result as a
runner-consumable optional-target capability while retaining the existing
OPTIONAL_TARGETS behavior; update bench/Makefile lines 10-12 accordingly. In
bench/run_benchmarks.sh lines 11-32, validate every required binary before
running benchmarks, use the capability to avoid rebuilding unavailable optional
targets, and skip phase_probe cleanly when it was not built while continuing to
run the remaining benchmarks.
- Around line 2-4: Declare the benchmark’s C11/GNU11 toolchain requirement:
update bench/Makefile lines 2-4 to compile with -std=gnu11, and update
bench/README.md lines 56-59 to require a C11/GNU11-compatible compiler.
In `@bench/phase_probe.c`:
- Around line 67-81: Validate every CUDA/NVML return status in
bench/phase_probe.c:67-81, bench/ctx_experiment.c:63-73, and
bench/nested_retain.c:51-73 before recording timings, emitting JSON, or
asserting state; abort or report the failure instead of producing benchmark
output. In the phase_probe process-query flow, handle
NVML_ERROR_INSUFFICIENT_SIZE by updating the required count and reissuing
nvmlDeviceGetComputeRunningProcesses rather than accepting COUNT_UNKNOWN, and
ensure device-handle, retain, and release failures are checked consistently at
each site.
In `@bench/README.md`:
- Around line 70-74: Update the documented workflow around the listed benchmark
commands to include nested_retain after warm_holder starts and before the
concurrency sweep, then include abi_check after the sweep, preserving the
existing command order.
In `@bench/run_benchmarks.sh`:
- Line 5: Update the benchmark script’s shell options to enable errexit,
nounset, and pipefail, then add EXIT and signal cleanup traps that kill and wait
for ${HOLDER_PID:-} when set. Ensure benchmark failures propagate through
pipelines while warm_holder is always stopped before the script exits.
- Line 17: Update the LD_PRELOAD setup in run_benchmarks.sh to verify that the
libvgpu.so path is readable and fail immediately with a clear error if it is
unavailable; only export LD_PRELOAD after this validation so benchmarks cannot
silently run against native CUDA.
In `@bench/warm_holder.c`:
- Around line 54-63: Update the mode selection around the nonprimary branch and
cuDevicePrimaryCtxRetain to explicitly accept only "primary" and "nonprimary";
reject any other mode with an error message and nonzero return before creating
or retaining a CUDA context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f600c82-95eb-49ad-bbb0-cd412ea4e46c
📒 Files selected for processing (10)
bench/.gitignorebench/Makefilebench/README.mdbench/abi_check.cbench/bench_init.cbench/ctx_experiment.cbench/nested_retain.cbench/phase_probe.cbench/run_benchmarks.shbench/warm_holder.c
Addresses review feedback on the benchmark harness: - Makefile: default CUDA_HOME to /usr/local/cuda instead of an author-local path. Add check-cuda-home, which fails fast with a clear message and the override syntax if cuda.h isn't found. Add print-targets so run_benchmarks.sh can ask make which targets were actually built (phase_probe is conditional on NVML) instead of assuming it's always present. - bench_init.c: fix percentile calculation (was truncating instead of taking the ceiling, which is wrong for small N). Zero-initialize every slot's rc to failure before workers run, since a worker that dies before writing its result would otherwise read back rc=0 from memset and look like a 0ms success. Add a timeout to the ready-barrier wait loop so a dead worker can't hang the benchmark forever; reap early exits as they happen instead of only after the timeout fires. - abi_check.c: hardcode the expected sizeof/offsetof values (captured from the Go mirror struct) and exit nonzero on mismatch, instead of just printing numbers a human has to compare by eye. - phase_probe.c, nested_retain.c, ctx_experiment.c, warm_holder.c: check the return value of every CUDA/NVML call instead of ignoring it, so a driver-level failure shows up as an error message instead of a garbage timing number. warm_holder now validates its mode argument instead of treating anything other than "nonprimary" as "primary" silently. phase_probe retries nvmlDeviceGetComputeRunningProcesses with a right-sized buffer on NVML_ERROR_INSUFFICIENT_SIZE instead of silently returning a truncated process list. - README.md, run_benchmarks.sh: reflect the above (percentile label p95 instead of p90 to match what bench_init actually reports, document the abi_check pass/fail behavior, use print-targets to detect whether phase_probe was built, fail with a clear message if libvgpu.so is missing instead of silently running unpreloaded and mislabeling native CUDA numbers as HAMi-core numbers). Rebuilt and re-ran the full suite after these changes: abi_check passes, bench_init and nested_retain reproduce the same numbers as before (cold retain ~34.5ms vs nested retain ~0.002ms; N=4 concurrent init completes with 0 failures). Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
|
Pushed. Makefile now defaults CUDA_HOME to /usr/local/cuda, and check-cuda-home fails fast with the override syntax if cuda.h isn't found there. Also added print-targets so run_benchmarks.sh can detect whether phase_probe was actually built instead of assuming it. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bench/ctx_experiment.c (1)
50-50: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEscape or validate
labelbefore emitting JSON.
argv[1]is interpolated directly into"cond":"%s". A label containing",\, a newline, or another control character produces malformed JSON and can break benchmark aggregation. Use a JSON-escaping helper or enforce a strict safe-label grammar before printing.Also applies to: 89-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bench/ctx_experiment.c` at line 50, Validate or JSON-escape the label selected in the argc/argv label initialization before it is interpolated into the JSON output. Update the corresponding emission near the later cond output so quotes, backslashes, newlines, and control characters cannot produce malformed JSON; either reuse an existing escaping helper or enforce a strict safe-label grammar.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/Makefile`:
- Around line 16-18: Make the CUDA preflight target check-cuda-home an
order-only prerequisite for every binary target in the Makefile, not just for
all. Ensure direct builds such as bench_init also run check-cuda-home first,
while preserving the existing all target dependency structure.
- Around line 9-14: Replace the HAVE_NVML ldconfig check with a compile/link
probe using $(CUDA_HOME)/include and the same NVML library search and
-lnvidia-ml flags used by phase_probe. Set HAVE_NVML only when the probe
succeeds, so TARGETS includes OPTIONAL_TARGETS exclusively when nvml.h and the
library are actually usable.
---
Outside diff comments:
In `@bench/ctx_experiment.c`:
- Line 50: Validate or JSON-escape the label selected in the argc/argv label
initialization before it is interpolated into the JSON output. Update the
corresponding emission near the later cond output so quotes, backslashes,
newlines, and control characters cannot produce malformed JSON; either reuse an
existing escaping helper or enforce a strict safe-label grammar.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1844c821-12bc-4292-823e-d19e5bf3b7a1
📒 Files selected for processing (9)
bench/Makefilebench/README.mdbench/abi_check.cbench/bench_init.cbench/ctx_experiment.cbench/nested_retain.cbench/phase_probe.cbench/run_benchmarks.shbench/warm_holder.c
🚧 Files skipped from review as they are similar to previous changes (7)
- bench/README.md
- bench/abi_check.c
- bench/nested_retain.c
- bench/warm_holder.c
- bench/phase_probe.c
- bench/run_benchmarks.sh
- bench/bench_init.c
ldconfig -p only confirms the runtime libnvidia-ml.so.1 (from the driver package) is present. It says nothing about whether nvml.h or the dev symlink libnvidia-ml.so are installed, both of which phase_probe's actual build needs (-lnvidia-ml at link time, nvml.h at compile time). A driver-only install would pass the old ldconfig check, get selected into TARGETS, and then fail to build. Replaced it with a real compile+link probe using the same INCLUDES and LIBS phase_probe itself is built with, so HAVE_NVML now means "phase_probe will actually build here," not just "some NVML library exists somewhere." Verified both directions: with the real CUDA_HOME it detects NVML and selects phase_probe; overriding LIBS with a nonexistent library name correctly drops phase_probe from the target list instead of crashing the build. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
all: check-cuda-home \$(TARGETS) only orders check-cuda-home ahead of compilation when make processes prerequisites serially. Under make -j it's not guaranteed to finish first, and make bench_init (building a single target directly) never mentions check-cuda-home at all, so both paths could start compiling against a missing/wrong CUDA_HOME before the check has a chance to fail loudly. Moved the dependency onto the targets themselves as an order-only prerequisite (\$(CORE_TARGETS) \$(OPTIONAL_TARGETS): | check-cuda-home), so it runs before any of them build regardless of how make was invoked. Verified: `make bench_init` with a bad CUDA_HOME now fails on check-cuda-home before attempting to compile. `make -j8` with a bad CUDA_HOME fails clean with no partial build artifacts. `make -j8` with a valid CUDA_HOME still builds all 5 binaries correctly. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/Makefile`:
- Around line 14-15: Add $(CFLAGS) before the -x c - arguments in the HAVE_NVML
probe, keeping the existing $(INCLUDES), $(LIBS), and NVML linkage unchanged so
detection matches phase_probe’s build flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a49850ff-b9bf-4a35-a8f5-9fc284f17333
📒 Files selected for processing (1)
bench/Makefile
The probe omitted \$(CFLAGS) while phase_probe's real build rule includes it (gcc \$(CFLAGS) \$(INCLUDES) \$< -o \$@ \$(LIBS) -lnvidia-ml), so any external -std/-march/--sysroot flags reflected in CFLAGS weren't exercised by the probe, risking a false positive on setups where those flags matter for whether the real build succeeds. Verified: HAVE_NVML still detects correctly with CUDA_HOME set to the real toolkit, and phase_probe still builds. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
|
@archlitchi @chaunceyjiang please go through this once, I've addressed the CodeRabbit comments. |
|
Thanks for putting this benchmark together — really useful tooling for the |
Sure, please go through my approach once @maverick123123 |
|
@om7057 ran this unchanged at 9c23395 on 8x A100 80GB, driver 580.65.06: three full run_benchmarks.sh passes plus bench_init at 240 and 300, zero failed workers, N=128 p99 78.75/78.28/78.09 s. been using it unchanged as the baseline protocol in #1662 since. full json, commands, checksums: https://gist.github.com/iemAnshuman/42dfda66804796ff4b906f5607ea3653. one note: driver-only images ship just libnvidia-ml.so.1, so the compile+link probe skips phase_probe until you add a symlink, maybe worth a README line |
3 participants
Supports reproducible benchmarking for issue Project-HAMi/HAMi#1662 and provides performance analysis tools for HAMi-core's multiprocess device-sharing layer.
Summary
Adds a benchmark suite measuring CUDA initialization latency in concurrent scenarios, with tools for phase-breakdown analysis, context retention amortization testing, and struct-layout verification.
Tools
Files
bench/*.c: Benchmark source files with Apache 2.0 license headersMakefile: Builds all tools; CUDA_HOME configurable, NVML optionalrun_benchmarks.sh: Orchestrates full suite to reproduce [Performance] High latency (~1 minute) when hundreds of processes initialize libvgpu.so concurrently HAMi#1662 findingsREADME.md: Documents each tool and expected results.gitignore: Excludes binary artifactsBuilding
Running
Expected output: linear scaling from ~61.5ms per-process, with ~80% time in CUDA context operations vs ~20% in NVML.
Binaries are excluded from git (only source and build config are tracked).
Summary by CodeRabbit