Skip to content

refactor(benchmark): make Benchmark::sync fallible#1414

Open
dcvz wants to merge 3 commits into
tracel-ai:mainfrom
oxiglade:refactor/fallible-benchmark-sync
Open

refactor(benchmark): make Benchmark::sync fallible#1414
dcvz wants to merge 3 commits into
tracel-ai:mainfrom
oxiglade:refactor/fallible-benchmark-sync

Conversation

@dcvz

@dcvz dcvz commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

On asynchronous backends, kernel launches are fire-and-forget: compilation/validation errors and GPU faults often only become observable at synchronization. Since sync() returned (), an implementor's only option was .unwrap(). That means a single benchmark whose kernel legitimately can't run on the current device (e.g. requesting 40 KiB of shared memory against Apple's 32 KiB limit) aborts the entire benchmark binary instead of failing that one bench. In practice this made full cubek benchmark sweeps impossible to complete on macOS.

sync now returns Result<(), String>, propagated through profile_full and run(). Two run() changes close holes the signature alone wouldn't fix:

  • warmup errors are propagated instead of discarded. Previously a queued JIT-compilation error was consumed invisibly by a warmup sync, and the benchmark reported timings for a kernel that never ran;
  • the stream is drained after the samples. Previously, under device timing (where profile() overrides bypass the trait's sync), a fault from the final samples surfaced at the next benchmark's leading sync and failed the wrong one.

The first commit fixes Metal fault recording during sync/profiling (faults recorded by completion handlers during the fence wait were dropped or misattributed). Standalone fixes, but without them the trait's guarantee wouldn't hold on Metal.

Migration is one line per impl: block_on(client.sync()).unwrap() → block_on(client.sync()).map_err(|e| format!("{e}")). The getting-started examples are updated.

@dcvz
dcvz force-pushed the refactor/fallible-benchmark-sync branch from 3883282 to dca752a Compare July 9, 2026 18:46

@nathanielsimard nathanielsimard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's a good improvement. Is there a way to use the ServerError struct ins

let error_sink = stream.errors.clone();

Box::pin(async move {
MetalStreamBackend::wait_event_sync(fence)?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If wait_event_sync fails, the recorded faults stay in the sink and poison a later unrelated call. The drain should happen regardless of the wait's outcome.

// profile instead of leaking into the next call on the stream.
let errors = self.flush_errors(stream_id);
if !errors.is_empty() {
return Err(ProfileError::Unknown {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

errors is a Vec<ServerError>, and ProfileError::Server(#[from] Box<ServerError>) exists at base.rs:71 for exactly this. It should be ProfileError::Server(Box::new(ServerError::ServerUnhealthy { errors, backtrace })). The pre-existing flush_errors at :160 does the same stringify, so both could be fixed together.

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.

2 participants