Repository navigation
Add a late-joiner benchmark and make the harness survive a flaky iteration - #1164
xianshijing-lk wants to merge 3 commits into
Conversation
…ation BM-SESSION measures a track published into an established session. The opposite arrangement — a participant joining a room that is already full — is the one a late joiner actually experiences, and the one preallocated receive sections are meant to serve, so it needs its own measurement. BM-JOIN seeds a room with N publishers, lets it reach steady state, then measures the joiner from `connect()` through every track being subscribed and rendering. N is swept so a preallocation constant can be chosen from data rather than taste. The harness changes matter more than the new scenario. Three defects, each found by it producing a wrong answer: - A thrown iteration was treated as a failed benchmark and discarded the variant whole, so one flaky run cost all 15 samples. The arm that occasionally failed then contributed no row at all rather than a visibly worse one, and its surviving percentiles read as if nothing had gone wrong. Failures now cost only their own sample and are counted. - `EventLatch` timeouts said only that *a* milestone was missed. Naming them turned "a lifecycle event" into `data_echo`, which is a different bug from the `subscribed` one it was assumed to be. - Nothing recorded *when* within an iteration anything happened, so a tail could not be decomposed. `TimelineLogger` stamps SDK logs on the benchmark's own clock and `Timeline.mark` emits per-iteration milestones, which is also what lets percentiles be computed over pooled raw samples instead of by averaging each run's percentiles — not a percentile of anything once the runs differ. The two instrumentation flags are deliberately separate: `LK_BENCH_TIMELINE` is one print per iteration and safe for a measured run, `LK_BENCH_SDK_LOG` is a synchronous print per log line on the paths being timed and is for diagnosis only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| var feeders: [Task<Void, Never>] = [] | ||
|
|
||
| func teardown() async { | ||
| for feeder in feeders { | ||
| feeder.cancel() | ||
| } | ||
| for publisher in publishers { | ||
| await publisher.disconnect() | ||
| } |
There was a problem hiding this comment.
🟡 Publisher feeders bypass task lifecycle rule
SeededRoom stores feeder tasks and cancels them manually. The repository requires cancellable task ownership instead, so feeders lack automatic cancellation if their owner exits without teardown.
Learn more
Each seeded publisher starts an unstructured frame-feeding task that runs until cancelled. SeededRoom owns the task handles and cancels them only in its explicit teardown() method. The repository's concurrency rule requires AnyTaskCancellable instead of manually managed Task handles, so ownership itself must drive cancellation.
Example: If a seeded-room owner exits without reaching teardown(), a feeder keeps capturing frames because dropping its Task handle does not cancel the detached task.
Recommended fix: Use a cancellable owner for the feeder tasks, exposing an equivalent benchmark-facing wrapper if the SDK's internal AnyTaskCancellable cannot be imported. Retain explicit teardown for prompt shutdown, but make owner release cancel any remaining feeders.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
note: AnyTaskCancellable is internal to LiveKit and not visible from the Benchmarks package, and every exit of seedRoom and runLateJoin calls teardown(), so no leak is reachable. The real gap is the missing await on the feeders (see line 148), so this thread can be resolved.
The synthetic source filled every plane with a constant, so consecutive frames were byte-identical. The encoder correctly drops unchanged content: it emitted one keyframe and then nothing, measured as `framesEncoded=2` over a 1.6s session fed at 15fps. Nothing downstream could decode until the SFU asked the publisher for a fresh keyframe, so `subscribed → first frame` timed a keyframe round trip rather than media latency, and the variance in that round trip read as a bimodal stall — 6-22% of iterations several hundred milliseconds slow, in both transport modes. Chasing it through the SDK found nothing, because nothing in the SDK was wrong. With a block sweeping across the luma plane the encoder produces a continuous stream, `framesEncoded` rises, and the stall disappears entirely: 150 samples, largest gap between consecutive sorted values 5ms. `createBufferTrack` also defaults to `.screenShareVideo`, which selects screen-share encoder presets, content-type hints and an uncapped start bitrate. A call publishes a camera, and the rust and JS harnesses use camera-type sources, so both benchmarks now pass `source: .camera` explicitly. Also drops a claim in BM-JOIN's header that measurement has since contradicted: single PC settles the subscription earlier, but on staging that does not convert into an earlier first frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Milestones are measured as they happen, so a session that failed at a late one had already recorded the earlier ones. Tolerating the failure instead of throwing kept those samples, which mixed sessions that went wrong into the percentiles of every metric preceding the failure and left each metric with a different sample count. A `data_echo` timeout, for instance, still contributed a `connect_ms` and a `pub_to_sub_ms`. `MeasurementBuffer` holds an iteration's samples and hands them over only on the success path, so an iteration is all-or-nothing — which is what the percentiles assume. `failed_iterations` is still recorded either way, so the failure rate remains visible. Also adds the .changes entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pblazej
left a comment
pblazej
left a comment
There was a problem hiding this comment.
Requesting changes for the timeline samples. MeasurementBuffer fixes the bias the description names, and the failed CI jobs are unrelated because CI does not build Benchmarks/.
| samples.record(mFirstFrame, Int(firstFrameAt - subscribedAt)) | ||
| samples.record(mPubToFrame, Int(firstFrameAt - publishStart)) | ||
|
|
||
| Timeline.mark(singlePC ? "SinglePC" : "DualPC", [ |
There was a problem hiding this comment.
issue (blocking): This prints before the data_echo wait can fail, and Timeline.mark has no warmup flag, so pooled timeline samples include iterations the metrics discard.
Print only after success with an iteration id and a warmup flag, or move TimelineLogger and Timeline to a separate PR (preferred).
| var feeders: [Task<Void, Never>] = [] | ||
|
|
||
| func teardown() async { | ||
| for feeder in feeders { |
There was a problem hiding this comment.
issue (non-blocking): The feeders are cancelled but not awaited before the publishers disconnect. Add await feeder.value after feeder.cancel().
| return nil | ||
| } catch let timeout as BenchmarkTimeout { | ||
| return timeout.milestone | ||
| } catch { |
There was a problem hiding this comment.
issue (non-blocking): A CancellationError counts as a flaky iteration here. Make runTolerantly throwing and rethrow cancellation before the catch-all.
| /// be known. Count the lines to get the rate. | ||
| func reportFailure(_ milestone: String?, variant: String) { | ||
| guard let milestone else { return } | ||
| print("[\(variant)] iteration failed, never reached: \(milestone)") |
There was a problem hiding this comment.
nitpick: For a non-timeout error this prints "never reached: ", which is not a milestone. BenchmarkTimeout.description already names the milestone, so one iteration failed: <error> message covers both.
| /// | ||
| /// Read at registration time, so the variants a run declares match the environment it runs in. | ||
| private func joinPublisherCounts() -> [Int] { | ||
| let fallback = [1, 5] |
There was a problem hiding this comment.
suggestion (non-blocking): Add N=3 and N=4 to find the boundary around three preallocated sections, and report the failure rate next to the success-only percentiles.
|
|
||
| for _ in benchmark.scaledIterations { | ||
| // As in BM-SESSION: an iteration contributes all of its samples or none. | ||
| let samples = MeasurementBuffer() |
There was a problem hiding this comment.
polish: This block is duplicated at SessionLifecycleBenchmark.swift:84-94, and both files declare mFailed. One helper in SessionObservers.swift can own both.
|
|
||
| /// How long the seeded room is left publishing before the joiner arrives, from | ||
| /// `LK_BENCH_JOIN_SETTLE_MS`. See the call site for why this is not a token delay. | ||
| private func joinSettleMilliseconds() -> Int { |
There was a problem hiding this comment.
polish: The description argues that 2 s is the correct settle, so a constant is enough here.
| /// | ||
| /// Split out from ``runSession(benchmark:config:tokenGen:singlePC:)`` because the publish-to-frame | ||
| /// leg is the one under investigation and reads better whole. | ||
| private func measurePublishToFirstFrame(samples: MeasurementBuffer, |
There was a problem hiding this comment.
polish: Without the timeline, this extraction is not needed, and the benchmark _: Benchmark parameter on line 103 is unused.
| } | ||
|
|
||
| /// Reports frames from one remote video track, so per-track arrival can be distinguished. | ||
| private final class FrameProbe: NSObject, VideoRenderer, @unchecked Sendable { |
There was a problem hiding this comment.
nitpick: FrameProbe holds only an immutable @Sendable closure, so plain Sendable works instead of @unchecked Sendable.
| /// samples from sessions that went wrong into the percentiles of the metrics that came before | ||
| /// the failure — and leave each metric with a different sample count. Buffering keeps an | ||
| /// iteration all-or-nothing, which is what the percentiles assume. | ||
| final class MeasurementBuffer { |
There was a problem hiding this comment.
praise: Staging samples and committing them only on success is the right fix for partial sessions.
BM-JOIN
BM-SESSION measures a track published into an established session. The opposite arrangement — a participant joining a room that is already full — is what a late joiner actually experiences, and the case preallocated receive sections are meant to serve, so it needs its own measurement.
Each iteration seeds a room with N publishers, lets it reach steady state, then measures the joiner from
connect()through every track being subscribed and rendering. N is swept (default 1 and 5) so a preallocation constant can be chosen from data.The settle before the joiner arrives is not a token delay. At 500ms,
first_frame_msread higher with one publisher than with five — more load, earlier frame — because seeding five takes longer and leaves the first one further along, so the measurement was gated on the publisher's encoder warm-up rather than on the join. 2s removes that, and matches the real arrangement: an agent, or the first party to a 1:1, has been publishing for seconds before anyone joins.The harness changes matter more than the new scenario
Three defects, each found by the harness producing a wrong answer:
A flaky iteration discarded the whole variant. The framework treats a thrown iteration as a failed benchmark, so one bad run cost all 15 samples and the arm vanished from the results table. Every casualty I saw was a dual-PC arm — which meant dual's worst runs deleted themselves while single PC's stayed in, silently tilting every comparison. Failures now cost only their own sample, are counted in a
failed_iterationsmetric, and print the milestone that was missed.Timeouts didn't say what timed out.
EventLatchreported only that a lifecycle event was missed. Naming the latches turned "a lifecycle event" intodata_echo— a different bug from thesubscribedone I had assumed.Nothing recorded when, within an iteration, anything happened.
TimelineLoggerstamps SDK logs on the benchmark's own clock andTimeline.markemits per-iteration milestones. This is also what makes correct aggregation possible: percentiles can be computed over pooled raw samples instead of by averaging each run's percentiles, which is not a percentile of anything once the runs differ.The two flags are deliberately separate:
LK_BENCH_TIMELINE=1is one print per iteration and safe for a measured run;LK_BENCH_SDK_LOG=1is a synchronous print per log line on the paths being timed and is for diagnosis only. (NoteLiveKitSDK.setLoggerStandardOutput()is deprecated and sets an OSLogger level, so it never reaches stdout —PrintLogger/TimelineLoggerhas to be installed explicitly.)Notes for review
startFeedingFramesmoved from BM-SESSION into shared support, and BM-SESSION's publish-to-frame leg was extracted into its own function.LateJoinObserveruses one renderer probe per track, because a shared renderer can't tell which track a frame came from — "all frames" would otherwise fire on the first track delivering N frames.mainas-is. It is built to evaluate the preallocation in Mid-based track resolution + preallocated receive media sections #1112, but does not depend on it.🤖 Generated with Claude Code