Add a late-joiner benchmark and make the harness survive a flaky iteration by xianshijing-lk · Pull Request #1164 · livekit/client-sdk-swift

xianshijing-lk · 2026-10-20T22:44:51Z

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_ms read 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_iterations metric, and print the milestone that was missed.

Timeouts didn't say what timed out. EventLatch reported only that a lifecycle event was missed. Naming the latches turned "a lifecycle event" into data_echo — a different bug from the subscribed one I had assumed.

Nothing recorded when, within an iteration, anything happened. TimelineLogger stamps SDK logs on the benchmark's own clock and Timeline.mark emits 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=1 is one print per iteration and safe for a measured run; LK_BENCH_SDK_LOG=1 is a synchronous print per log line on the paths being timed and is for diagnosis only. (Note LiveKitSDK.setLoggerStandardOutput() is deprecated and sets an OSLogger level, so it never reaches stdout — PrintLogger/TimelineLogger has to be installed explicitly.)

Notes for review

Benchmarks only; no consumer-facing code.
startFeedingFrames moved from BM-SESSION into shared support, and BM-SESSION's publish-to-frame leg was extracted into its own function.
LateJoinObserver uses 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.
Runs on main as-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

…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>

github-actions · 2026-10-20T22:45:01Z

⚠️ This PR does not contain any files in the .changes directory.

devin-ai-integration

Devin Review found 2 potential issues.

devin-ai-integration · 2026-10-20T22:47:32Z

+    var feeders: [Task<Void, Never>] = []
+
+    func teardown() async {
+        for feeder in feeders {
+            feeder.cancel()
+        }
+        for publisher in publishers {
+            await publisher.disconnect()
+        }


🟡 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.

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

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/.

pblazej · 2026-10-29T08:41:35Z

+        samples.record(mFirstFrame, Int(firstFrameAt - subscribedAt))
+        samples.record(mPubToFrame, Int(firstFrameAt - publishStart))
+
+        Timeline.mark(singlePC ? "SinglePC" : "DualPC", [


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).

pblazej · 2026-10-29T08:41:35Z

+    var feeders: [Task<Void, Never>] = []
+
+    func teardown() async {
+        for feeder in feeders {


issue (non-blocking): The feeders are cancelled but not awaited before the publishers disconnect. Add await feeder.value after feeder.cancel().

pblazej · 2026-10-29T08:41:35Z

+        return nil
+    } catch let timeout as BenchmarkTimeout {
+        return timeout.milestone
+    } catch {


issue (non-blocking): A CancellationError counts as a flaky iteration here. Make runTolerantly throwing and rethrow cancellation before the catch-all.

pblazej · 2026-10-29T08:41:35Z

+/// 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)")


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.

pblazej · 2026-10-29T08:41:35Z

+///
+/// Read at registration time, so the variants a run declares match the environment it runs in.
+private func joinPublisherCounts() -> [Int] {
+    let fallback = [1, 5]


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.

pblazej · 2026-10-29T08:41:35Z

+
+                for _ in benchmark.scaledIterations {
+                    // As in BM-SESSION: an iteration contributes all of its samples or none.
+                    let samples = MeasurementBuffer()


polish: This block is duplicated at SessionLifecycleBenchmark.swift:84-94, and both files declare mFailed. One helper in SessionObservers.swift can own both.

pblazej · 2026-10-29T08:41:35Z

+
+/// 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 {


polish: The description argues that 2 s is the correct settle, so a constant is enough here.

pblazej · 2026-10-29T08:41:35Z

+///
+/// 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,


polish: Without the timeline, this extraction is not needed, and the benchmark _: Benchmark parameter on line 103 is unused.

pblazej · 2026-10-29T08:41:35Z

+}
+
+/// Reports frames from one remote video track, so per-track arrival can be distinguished.
+private final class FrameProbe: NSObject, VideoRenderer, @unchecked Sendable {


nitpick: FrameProbe holds only an immutable @Sendable closure, so plain Sendable works instead of @unchecked Sendable.

pblazej · 2026-10-29T08:41:35Z

+/// 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 {


praise: Staging samples and committing them only on success is the right fix for partial sessions.

xianshijing-lk requested review from hiroshihorie and pblazej as code owners October 20, 2026 22:44

devin-ai-integration Bot reviewed Oct 20, 2026

View reviewed changes

xianshijing-lk and others added 2 commits October 28, 2026 16:08

pblazej requested changes Oct 29, 2026

View reviewed changes

Conversation

xianshijing-lk commented Oct 20, 2026

BM-JOIN

The harness changes matter more than the new scenario

Notes for review

Uh oh!

github-actions Bot commented Oct 20, 2026

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration Bot Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pblazej left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

devin-ai-integration Bot Oct 20, 2026 •

edited

Loading