Skip to content

Remove swift-collections dependency#948

Draft
dfed wants to merge 7 commits intolivekit:mainfrom
dfed:patch-1
Draft

Remove swift-collections dependency#948
dfed wants to merge 7 commits intolivekit:mainfrom
dfed:patch-1

Conversation

@dfed
Copy link
Contributor

@dfed dfed commented Mar 12, 2026

Summary

  • Replace DequeModule and OrderedCollections imports with minimal internal implementations (Deque, OrderedDictionary, OrderedSet) in Sources/LiveKit/Support/DataStructures/
  • Remove swift-collections from Package.swift, Package@swift-6.0.swift, and LiveKitClient.podspec
  • Unblocks consumers from being pinned to old swift-collections versions due to library evolution mode / XCFramework compatibility constraints

Details

The actual API surface used was tiny — 3 types, ~14 call sites, all internal. The new implementations are ~180 lines total, matching the existing pattern of lightweight data structures in the repo (RingBuffer, MapTable, TTLDictionary).

Test plan

  • xcodebuild build -scheme LiveKit -destination 'platform=macOS' succeeds
  • xcodebuild test -scheme LiveKit -only-testing LiveKitCoreTests -destination 'platform=macOS' — all non-E2E tests pass (E2E tests require local server)
  • CI passes

🤖 Generated with Claude Code

@hiroshihorie hiroshihorie requested a review from pblazej March 17, 2026 10:28
@pblazej
Copy link
Contributor

pblazej commented Mar 17, 2026

Could you inspect the CI build errors? @dfed

Hint: the key is binary distribution (it was downgraded for a reason); the closest comment I could find was apple/swift-collections#546 (comment)

Haven't tested it tho

@pblazej
Copy link
Contributor

pblazej commented Mar 17, 2026

After a short test, I see 2 obstacles:

  • newer versions do not officially support Swift 5.x toolchain (that won't be an issue soon)
  • more importantly, they rely on experimental features and will break library evolution mode, which is crucial for building a xcframework on top of LK - this is a strict requirement for us

I'm happy to merge it once we resolve the latter.

@dfed
Copy link
Contributor Author

dfed commented Mar 17, 2026

Got it. @pblazej would y'all be interested in a PR that copy/pasted the existing implementation of Deque, OrderedDictionary, and OrderedSet from Swift Collections as internal types such that y'all can remove swift-collections as a dependency? Ideally depending on LiveKit wouldn't restrict adopting apps from using the latest libs from Apple. Happy to update this PR

Replace DequeModule and OrderedCollections with minimal internal
implementations covering only the APIs actually used (~14 call sites).
This unblocks consumers from being pinned to old swift-collections
versions due to library evolution mode compatibility constraints.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dfed dfed marked this pull request as draft March 17, 2026 17:56
@dfed dfed changed the title Update swift-collections package version range Remove swift-collections dependency Mar 17, 2026
dfed and others added 2 commits March 17, 2026 10:59
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@@ -0,0 +1,43 @@
/*
* Copyright 2026 LiveKit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

less sure about this. happy to put whatever you want up here

Copy link
Contributor

Choose a reason for hiding this comment

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

just run swiftformat over it, I think it's fine (checked on CI)

Array.removeFirst() is O(n), making the processSendQueue drain
loop O(n²). Switch to a circular buffer backed by [Element?] with
head pointer tracking for O(1) amortized append and removeFirst.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
/// A double-ended queue backed by a circular buffer.
/// Provides O(1) amortized `append` and `removeFirst`.
struct Deque<Element>: ExpressibleByArrayLiteral {
private var buffer: [Element?] = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minimal internal Deque using a circular buffer ([Element?] + head pointer). Provides O(1) amortized append and removeFirst, which matters because processSendQueue in DataChannelPair drains this in a tight while loop — an Array-backed implementation would make that O(n²).

Only implements the 4 APIs used: isEmpty, first, append, removeFirst, plus ExpressibleByArrayLiteral for = [] initialization.

Copy link
Contributor

Choose a reason for hiding this comment

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

For removeFirst you can leverage the % thing mentioned below, it was >40% faster in my case.

@pblazej
Copy link
Contributor

pblazej commented Mar 18, 2026

@dfed thank you for going the extra mile here!

The problem with binary distribution is pretty common across Apple libs e.g. apple/swift-nio#2897 and already happened to use with swift-logging that we dropped vs bare OSLog.

I think porting the minimal API is better than carrying "a big part of the lib" under Apache as our usage is pretty narrow, the impact of future optimizations (leveraging ownership etc.) is probably negligible.

2 things:

  • please rebase on main to include Package@swift-6.2
  • you can run https://github.com/apple/swift-collections-benchmark against them to see if there are any bottlenecks vs the reference impl
    • or I'll do it as a part of review - I'm talking mostly about DataChannelPair <- Deque

@pblazej
Copy link
Contributor

pblazej commented Mar 18, 2026

bench-deque

collection-bench.zip

Attaching my results - nothing to worry about 🎉

The rest is micro-optimization like:

  // Before (integer division)
  let tail = (head + count_) % buffer.count

  // After (bitwise AND — same result when buffer.count is power of 2)
  let tail = (head + count_) & (buffer.count - 1)

@discardableResult
mutating func removeFirst() -> Element {
guard count_ > 0 else {
fatalError("Cannot removeFirst from an empty Deque")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fatalError("Cannot removeFirst from an empty Deque")
preconditionFailure("Cannot removeFirst from an empty Deque")

matches Array

}
}

init(uniqueKeysWithValues keysAndValues: some Sequence<(Key, Value)>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original does precondition here, I'm fine with in-place update as well, e.g.

  init(uniqueKeysWithValues keysAndValues: some Sequence<(Key, Value)>) {
      for (key, value) in keysAndValues {
          if let i = index[key] {
              pairs[i] = (key, value)
          } else {
              index[key] = pairs.count
              pairs.append((key, value))
          }
      }
  }

if count_ == buffer.count {
grow()
}
let tail = (head + count_) % buffer.count
Copy link
Contributor

@pblazej pblazej Mar 18, 2026

Choose a reason for hiding this comment

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

Since capacity is always a power of two (starts at 4, doubles)...

Suggested change
let tail = (head + count_) % buffer.count
let tail = (head + count_) & (buffer.count - 1)

}
let element = buffer[head]!
buffer[head] = nil
head = (head + 1) % buffer.count
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
head = (head + 1) % buffer.count
head = (head + 1) & (buffer.count - 1)

Comment on lines +63 to +67
let newCapacity = max(buffer.count * 2, 4)
var newBuffer = [Element?](repeating: nil, count: newCapacity)
for i in 0 ..< count_ {
newBuffer[i] = buffer[(head + i) % buffer.count]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let newCapacity = max(buffer.count * 2, 4)
var newBuffer = [Element?](repeating: nil, count: newCapacity)
for i in 0 ..< count_ {
newBuffer[i] = buffer[(head + i) % buffer.count]
}
let newCapacity = max(buffer.count * 2, 4)
var newBuffer = [Element?](repeating: nil, count: newCapacity)
let mask = buffer.count - 1
for i in 0 ..< count_ {
newBuffer[i] = buffer[(head + i) & mask]

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