Repository navigation
Fix git-metadata probe CPU storm: lock-free hex + concurrency cap #5400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| import Foundation | ||
|
|
||
| /// Bounds how many git-metadata probes parse index files at the same time. | ||
| /// | ||
| /// Each tracked workspace panel schedules a detached probe that reads and | ||
| /// parses the entire `.git/index`. A burst — a workspace restore, the periodic | ||
| /// fallback refresh, or a flurry of file-system watch events — can schedule one | ||
| /// probe per panel at once. With dozens of panels that spawns dozens of | ||
| /// parallel parses and saturates every core, producing the multi-core CPU | ||
| /// spikes tracked in https://github.com/manaflow-ai/cmux/issues/4639. | ||
| /// | ||
| /// This actor caps how many probes run concurrently. Callers wrap the parse in | ||
| /// ``run(_:)``; work beyond the cap suspends until a slot frees and resumes in | ||
| /// FIFO order. Construct one and share it across every probe: | ||
| /// | ||
| /// ```swift | ||
| /// let limiter = GitProbeConcurrencyLimiter(maxConcurrent: 4) | ||
| /// let snapshot = await limiter.run { await parseIndex() } | ||
| /// ``` | ||
| public actor GitProbeConcurrencyLimiter { | ||
| private let maxConcurrent: Int | ||
| private var available: Int | ||
| private var waiters: [CheckedContinuation<Void, Never>] = [] | ||
|
|
||
| /// Creates a limiter allowing at most `maxConcurrent` probes to run at once. | ||
| /// | ||
| /// - Parameter maxConcurrent: The concurrency ceiling. Values below 1 are | ||
| /// clamped to 1 so the limiter always makes forward progress. | ||
| public init(maxConcurrent: Int) { | ||
| let ceiling = max(1, maxConcurrent) | ||
| self.maxConcurrent = ceiling | ||
| self.available = ceiling | ||
| } | ||
|
|
||
| /// Runs `body` once a slot is free, releasing the slot when it returns. | ||
| /// | ||
| /// While `body` is suspended on its own `await`s the actor is free to admit | ||
| /// other callers up to the ceiling; only the slot accounting is serialized. | ||
| /// | ||
| /// - Parameter body: The probe work to run while holding a slot. | ||
| /// - Returns: Whatever `body` returns. | ||
| public func run<T: Sendable>(_ body: @Sendable () async -> T) async -> T { | ||
| await acquire() | ||
| defer { release() } | ||
| return await body() | ||
| } | ||
|
|
||
| private func acquire() async { | ||
| if available > 0 { | ||
| available -= 1 | ||
| return | ||
| } | ||
| // Resumed by `release()`, which hands off its slot directly without | ||
| // bumping `available`, so the slot count stays balanced. | ||
| await withCheckedContinuation { waiters.append($0) } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Handle cancellation while waiting in Prompt for AI agents |
||
| } | ||
|
Comment on lines
+48
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The fix is to wrap the suspension in
Comment on lines
+48
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff Optional enhancement: task cancellation support. Cancelled tasks currently remain in the await withTaskCancellationHandler {
await withCheckedContinuation { waiters.append($0) }
} onCancel: {
Task { await removeCancelledWaiter() }
}However, this adds complexity (tracking which continuation to remove, ensuring atomicity). The current simple design is acceptable given that git probe work is lightweight and short-lived. 🤖 Prompt for AI Agents |
||
|
|
||
| private func release() { | ||
| if waiters.isEmpty { | ||
| available += 1 | ||
| } else { | ||
| waiters.removeFirst().resume() | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import Foundation | ||
| import Testing | ||
| @testable import CmuxGit | ||
|
|
||
| /// Records how many bodies run between ``enter()`` and ``leave()`` at once so a | ||
| /// test can assert the limiter never exceeds its ceiling. | ||
| private actor ConcurrencyTracker { | ||
| private(set) var current = 0 | ||
| private(set) var peak = 0 | ||
| private(set) var completed = 0 | ||
|
|
||
| func enter() { | ||
| current += 1 | ||
| peak = max(peak, current) | ||
| } | ||
|
|
||
| func leave() { | ||
| current -= 1 | ||
| completed += 1 | ||
| } | ||
| } | ||
|
|
||
| @Suite struct GitProbeConcurrencyLimiterTests { | ||
| /// 60 tasks through a ceiling of 3 must never run more than 3 bodies at | ||
| /// once, and every task must still complete. | ||
| @Test func capsConcurrencyAndCompletesEveryTask() async { | ||
| let limiter = GitProbeConcurrencyLimiter(maxConcurrent: 3) | ||
| let tracker = ConcurrencyTracker() | ||
| await withTaskGroup(of: Void.self) { group in | ||
| for _ in 0..<60 { | ||
| group.addTask { | ||
| await limiter.run { | ||
| await tracker.enter() | ||
| await Task.yield() | ||
| await Task.yield() | ||
| await tracker.leave() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| #expect(await tracker.peak <= 3) | ||
| #expect(await tracker.peak >= 1) | ||
| #expect(await tracker.completed == 60) | ||
| } | ||
|
|
||
| /// A ceiling of 1 strictly serializes: despite yields, no two bodies are | ||
| /// ever in flight together. | ||
| @Test func ceilingOfOneSerializes() async { | ||
| let limiter = GitProbeConcurrencyLimiter(maxConcurrent: 1) | ||
| let tracker = ConcurrencyTracker() | ||
| await withTaskGroup(of: Void.self) { group in | ||
| for _ in 0..<25 { | ||
| group.addTask { | ||
| await limiter.run { | ||
| await tracker.enter() | ||
| await Task.yield() | ||
| await tracker.leave() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| #expect(await tracker.peak == 1) | ||
| #expect(await tracker.completed == 25) | ||
| } | ||
|
|
||
| /// A non-positive ceiling is clamped to 1 rather than deadlocking on zero | ||
| /// available slots. | ||
| @Test func clampsNonPositiveCeilingToOne() async { | ||
| let limiter = GitProbeConcurrencyLimiter(maxConcurrent: 0) | ||
| let tracker = ConcurrencyTracker() | ||
| await withTaskGroup(of: Void.self) { group in | ||
| for _ in 0..<10 { | ||
| group.addTask { | ||
| await limiter.run { | ||
| await tracker.enter() | ||
| await Task.yield() | ||
| await tracker.leave() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| #expect(await tracker.peak == 1) | ||
| #expect(await tracker.completed == 10) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import Foundation | ||
| import Testing | ||
| @testable import CmuxGit | ||
|
|
||
| /// Verifies the table-based hex encoder that replaced per-byte | ||
| /// `String(format: "%02x")` in the git index parser produces byte-identical | ||
| /// output. The replacement removed a process-global locale lock that turned | ||
| /// parallel git probes into a multi-core CPU storm | ||
| /// (https://github.com/manaflow-ai/cmux/issues/4639). | ||
| @Suite struct HexEncodingTests { | ||
| @Test func matchesPrintfFormatForEveryByteValue() { | ||
| for value in UInt8.min...UInt8.max { | ||
| #expect(lowercaseHexString([value]) == String(format: "%02x", value)) | ||
| } | ||
| } | ||
|
|
||
| @Test func matchesPrintfFormatForObjectIDWidthSequence() { | ||
| // 20 bytes is the SHA-1 object-ID width formatted per index entry, | ||
| // the hot path in gitIndexSnapshot. | ||
| let bytes: [UInt8] = (0..<20).map { UInt8(($0 * 37 + 5) & 0xff) } | ||
| #expect(lowercaseHexString(bytes) == bytes.map { String(format: "%02x", $0) }.joined()) | ||
| #expect(lowercaseHexString(bytes).count == 40) | ||
| } | ||
|
|
||
| @Test func encodesEmptyCollectionAsEmptyString() { | ||
| #expect(lowercaseHexString([UInt8]()) == "") | ||
| } | ||
|
|
||
| @Test func matchesPrintfFormatForArraySliceAndDataSuffix() { | ||
| // The real call sites pass an ArraySlice (object ID / checksum) and a | ||
| // Data.suffix (file signature); both must encode identically. | ||
| let bytes: [UInt8] = [0x00, 0x0f, 0xa0, 0xff, 0x10, 0x01, 0x7e] | ||
| let slice = bytes[1..<6] | ||
| #expect(lowercaseHexString(slice) == slice.map { String(format: "%02x", $0) }.joined()) | ||
| let data = Data(bytes) | ||
| #expect( | ||
| lowercaseHexString(data.suffix(4)) | ||
| == data.suffix(4).map { String(format: "%02x", $0) }.joined() | ||
| ) | ||
| } | ||
|
|
||
| @Test func matchesPrintfFormatForUInt64BigEndianHash() { | ||
| // The FNV content signature was %016llx of a UInt64; the replacement | ||
| // encodes the 8 big-endian bytes and must match for full-width values. | ||
| let values: [UInt64] = [0, 1, 0x0123_4567_89ab_cdef, .max, 0xff] | ||
| for value in values { | ||
| let bytes = (0..<8).map { UInt8(truncatingIfNeeded: value >> (8 * (7 - $0))) } | ||
| #expect(lowercaseHexString(bytes) == String(format: "%016llx", CUnsignedLongLong(value))) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxConcurrentis stored as an instance property but is never read afterinit. The live ceiling is tracked entirely byavailable;maxConcurrentis dead state. It can be removed.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!