From 79b921aeb9c30b18018cf81837219d9aa12156fc Mon Sep 17 00:00:00 2001 From: jack Date: Fri, 10 Jul 2026 15:19:56 -0400 Subject: [PATCH] Avoid index rebuilds at conversation cap --- bitchat/App/ConversationStore.swift | 66 +-- bitchatTests/ConversationStoreTests.swift | 420 ++++++++++++++++++ .../PerformanceBaselineTests.swift | 56 +++ bitchatTests/Performance/perf-floors.json | 7 +- 4 files changed, 523 insertions(+), 26 deletions(-) diff --git a/bitchat/App/ConversationStore.swift b/bitchat/App/ConversationStore.swift index 8128cd38..7631c495 100644 --- a/bitchat/App/ConversationStore.swift +++ b/bitchat/App/ConversationStore.swift @@ -39,15 +39,17 @@ final class Conversation: ObservableObject, Identifiable { @Published private(set) var messages: [BitchatMessage] = [] @Published private(set) var isUnread: Bool = false - /// Incrementally-maintained message-ID → index map for O(1) dedup and - /// delivery-status lookup. Kept in sync on every mutation: - /// - tail append: single insert - /// - out-of-order insert: suffix reindex from the insertion point - /// - trim: full rebuild — `removeFirst(k)` is already O(n), so the - /// rebuild does not change the asymptotics, and trim only happens once - /// the cap (1337) is reached. Simple and correct beats the - /// offset-tracking alternative here. + /// Incrementally-maintained message-ID → logical-index map for O(1) + /// dedup and delivery-status lookup. Logical indexes are physical array + /// indexes plus `indexOffset`; trimming from the head advances the offset + /// instead of rewriting every surviving dictionary entry. This matters + /// after the 1337-message cap is reached, when every steady-state tail + /// append evicts one old row. + /// + /// Out-of-order inserts and middle removals still reindex only the + /// affected suffix. Full filtering resets the offset while rebuilding. private var indexByMessageID: [String: Int] = [:] + private var indexOffset = 0 fileprivate init(id: ConversationID, cap: Int) { self.id = id @@ -61,7 +63,7 @@ final class Conversation: ObservableObject, Identifiable { } func message(withID messageID: String) -> BitchatMessage? { - guard let index = indexByMessageID[messageID] else { return nil } + guard let index = physicalIndex(forMessageID: messageID) else { return nil } return messages[index] } @@ -101,7 +103,7 @@ final class Conversation: ObservableObject, Identifiable { reindex(from: index) } else { messages.append(message) - indexByMessageID[message.id] = messages.count - 1 + indexByMessageID[message.id] = indexOffset + messages.count - 1 } return InsertResult(inserted: true, trimmedMessageIDs: trimIfNeeded()) @@ -111,7 +113,7 @@ final class Conversation: ObservableObject, Identifiable { /// timeline position (in-place updates like media progress reuse the /// original timestamp); a new message goes through ordered insertion. fileprivate func upsert(_ message: BitchatMessage) -> UpsertOutcome { - if let index = indexByMessageID[message.id] { + if let index = physicalIndex(forMessageID: message.id) { messages[index] = message return .updated } @@ -125,7 +127,7 @@ final class Conversation: ObservableObject, Identifiable { /// `.read` is never downgraded to `.delivered` or `.sent`. /// Returns `true` when the status was applied. fileprivate func applyDeliveryStatus(_ status: DeliveryStatus, forMessageID messageID: String) -> Bool { - guard let index = indexByMessageID[messageID] else { return false } + guard let index = physicalIndex(forMessageID: messageID) else { return false } let message = messages[index] guard !Self.shouldSkipStatusUpdate(current: message.deliveryStatus, new: status) else { return false } @@ -142,7 +144,7 @@ final class Conversation: ObservableObject, Identifiable { /// observers still need an @Published emission to re-render. @discardableResult fileprivate func republishMessage(withID messageID: String) -> Bool { - guard let index = indexByMessageID[messageID] else { return false } + guard let index = physicalIndex(forMessageID: messageID) else { return false } messages[index] = messages[index] return true } @@ -157,10 +159,14 @@ final class Conversation: ObservableObject, Identifiable { /// Removes a single message by ID. Returns the removed message, or /// `nil` when no message with that ID exists. fileprivate func remove(messageID: String) -> BitchatMessage? { - guard let index = indexByMessageID[messageID] else { return nil } + guard let index = physicalIndex(forMessageID: messageID) else { return nil } let removed = messages.remove(at: index) indexByMessageID.removeValue(forKey: messageID) - reindex(from: index) + if index == 0 { + indexOffset += 1 + } else { + reindex(from: index) + } return removed } @@ -177,6 +183,7 @@ final class Conversation: ObservableObject, Identifiable { for id in removedIDs { indexByMessageID.removeValue(forKey: id) } + indexOffset = 0 reindex(from: 0) return removedIDs } @@ -184,6 +191,7 @@ final class Conversation: ObservableObject, Identifiable { fileprivate func clearMessages() { messages.removeAll() indexByMessageID.removeAll() + indexOffset = 0 } // MARK: Diagnostics @@ -205,9 +213,10 @@ final class Conversation: ObservableObject, Identifiable { let message = messages[position] // Count equality + every message resolving to its own position // proves the index is exactly the inverse map (no stale extras). - if let index = indexByMessageID[message.id] { - if index != position { - violations.append("\(label): message \(message.id.prefix(8))… at \(position) indexed at \(index)") + if let logicalIndex = indexByMessageID[message.id] { + let expectedIndex = indexOffset + position + if logicalIndex != expectedIndex { + violations.append("\(label): message \(message.id.prefix(8))… at \(position) indexed at \(logicalIndex - indexOffset)") } } else { violations.append("\(label): message \(message.id.prefix(8))… at \(position) missing from index") @@ -269,10 +278,17 @@ final class Conversation: ObservableObject, Identifiable { private func reindex(from start: Int) { for index in start.. Int? { + guard let logicalIndex = indexByMessageID[messageID] else { return nil } + let index = logicalIndex - indexOffset + guard messages.indices.contains(index) else { return nil } + return index + } + /// Trims oldest messages over the cap; returns the trimmed message IDs. private func trimIfNeeded() -> [String] { guard messages.count > cap else { return [] } @@ -282,7 +298,7 @@ final class Conversation: ObservableObject, Identifiable { indexByMessageID.removeValue(forKey: id) } messages.removeFirst(overflow) - reindex(from: 0) + indexOffset += overflow return trimmedIDs } } @@ -844,8 +860,8 @@ extension Conversation { /// (positions 0 and 1 swap their index entries). Requires >= 2 messages. func _testCorruptIndexEntries() { guard messages.count >= 2 else { return } - indexByMessageID[messages[0].id] = 1 - indexByMessageID[messages[1].id] = 0 + indexByMessageID[messages[0].id] = indexOffset + 1 + indexByMessageID[messages[1].id] = indexOffset } /// Drops a message's index entry entirely (count mismatch + missing). @@ -859,8 +875,8 @@ extension Conversation { func _testCorruptOrderingPreservingIndex() { guard messages.count >= 2 else { return } messages.swapAt(0, messages.count - 1) - indexByMessageID[messages[0].id] = 0 - indexByMessageID[messages[messages.count - 1].id] = messages.count - 1 + indexByMessageID[messages[0].id] = indexOffset + indexByMessageID[messages[messages.count - 1].id] = indexOffset + messages.count - 1 } } @@ -900,7 +916,7 @@ extension ConversationStore { extension Conversation { fileprivate func _testAppendBypassingTrim(_ message: BitchatMessage) { messages.append(message) - indexByMessageID[message.id] = messages.count - 1 + indexByMessageID[message.id] = indexOffset + messages.count - 1 } } #endif diff --git a/bitchatTests/ConversationStoreTests.swift b/bitchatTests/ConversationStoreTests.swift index 3c9ef6fd..ba56c1e7 100644 --- a/bitchatTests/ConversationStoreTests.swift +++ b/bitchatTests/ConversationStoreTests.swift @@ -45,6 +45,158 @@ private func makeDirectConversationID(_ suffix: String) -> ConversationID { )) } +/// Deliberately simple O(n) model used to differentially test the store's +/// optimized logical-index bookkeeping. It models observable behavior only; +/// it has no offset or ID index and therefore cannot reproduce the same bug. +private struct ReferenceConversationTimeline { + struct Message: Equatable { + let id: String + let timestamp: Date + let content: String + var deliveryStatus: DeliveryStatus? + + init(_ message: BitchatMessage) { + id = message.id + timestamp = message.timestamp + content = message.content + deliveryStatus = message.deliveryStatus + } + } + + struct AppendResult { + let inserted: Bool + let trimmedCount: Int + } + + let cap: Int + private(set) var messages: [Message] = [] + + func contains(_ id: String) -> Bool { + messages.contains { $0.id == id } + } + + func message(withID id: String) -> Message? { + messages.first { $0.id == id } + } + + mutating func append(_ message: BitchatMessage) -> AppendResult { + guard !contains(message.id) else { + return AppendResult(inserted: false, trimmedCount: 0) + } + + let snapshot = Message(message) + var low = 0 + var high = messages.count + while low < high { + let mid = (low + high) / 2 + if messages[mid].timestamp <= snapshot.timestamp { + low = mid + 1 + } else { + high = mid + } + } + messages.insert(snapshot, at: low) + + let overflow = max(0, messages.count - cap) + if overflow > 0 { + messages.removeFirst(overflow) + } + return AppendResult(inserted: true, trimmedCount: overflow) + } + + mutating func upsert(_ message: BitchatMessage) -> Int { + if let index = messages.firstIndex(where: { $0.id == message.id }) { + messages[index] = Message(message) + return 0 + } + return append(message).trimmedCount + } + + mutating func applyDeliveryStatus(_ status: DeliveryStatus, to id: String) -> Bool { + guard let index = messages.firstIndex(where: { $0.id == id }), + messages[index].deliveryStatus != status else { + return false + } + // The differential stream uses only unique `.delivered` values (or + // an exact repeat), so no-downgrade policy is intentionally outside + // this index-focused reference model. + messages[index].deliveryStatus = status + return true + } + + mutating func remove(at index: Int) -> Message { + messages.remove(at: index) + } + + mutating func removeAll(where predicate: (Message) -> Bool) { + messages.removeAll(where: predicate) + } + + mutating func clear() { + messages.removeAll() + } +} + +private struct ConversationStoreDifferentialRNG { + private var state: UInt64 + + init(seed: UInt64) { + state = seed + } + + mutating func next() -> UInt64 { + state &+= 0x9E37_79B9_7F4A_7C15 + var value = state + value = (value ^ (value >> 30)) &* 0xBF58_476D_1CE4_E5B9 + value = (value ^ (value >> 27)) &* 0x94D0_49BB_1331_11EB + return value ^ (value >> 31) + } + + mutating func index(upperBound: Int) -> Int { + precondition(upperBound > 0) + return Int(next() % UInt64(upperBound)) + } +} + +@MainActor +private func expectStore( + _ store: ConversationStore, + matches reference: ReferenceConversationTimeline, + issuedIDs: [String], + checkpoint: String +) { + let conversation = store.conversation(for: .mesh) + let actual = conversation.messages.map(ReferenceConversationTimeline.Message.init) + #expect(actual == reference.messages, "timeline mismatch at \(checkpoint)") + + let lookupSnapshot = reference.messages.compactMap { expected in + conversation.message(withID: expected.id).map(ReferenceConversationTimeline.Message.init) + } + #expect(lookupSnapshot == reference.messages, "ID lookup mismatch at \(checkpoint)") + #expect( + Set(conversation.messageIDs) == Set(reference.messages.map(\.id)), + "per-conversation ID set mismatch at \(checkpoint)" + ) + + if !reference.messages.isEmpty { + for index in Set([0, reference.messages.count / 2, reference.messages.count - 1]) { + let id = reference.messages[index].id + #expect(store.conversationIDs(forMessageID: id) == [.mesh], "store ID map mismatch at \(checkpoint)") + } + } + + let activeIDs = Set(reference.messages.map(\.id)) + var checkedStaleIDs = 0 + for id in issuedIDs.reversed() where !activeIDs.contains(id) { + #expect(conversation.message(withID: id) == nil, "stale conversation index entry at \(checkpoint)") + #expect(store.conversationIDs(forMessageID: id).isEmpty, "stale store ID map entry at \(checkpoint)") + checkedStaleIDs += 1 + if checkedStaleIDs == 16 { break } + } + + #expect(store.auditInvariants().isEmpty, "invariant audit failed at \(checkpoint)") +} + @Suite("ConversationStore") struct ConversationStoreTests { @@ -140,6 +292,274 @@ struct ConversationStoreTests { #expect(conversation.message(withID: probeID)?.deliveryStatus == .sent) } + @Test("steady-state cap trimming keeps lookups exact across mixed mutations") + @MainActor + func steadyStateCapTrimmingKeepsLogicalIndexExact() { + let store = ConversationStore() + let conversation = store.conversation(for: .mesh) + let overflow = 64 + + for i in 0..<(conversation.cap + overflow) { + store.append(makeMessage(id: "m\(i)", timestamp: TimeInterval(i)), to: .mesh) + } + + #expect(conversation.messages.first?.id == "m\(overflow)") + #expect(conversation.message(withID: "m\(overflow)")?.id == "m\(overflow)") + + // Exercise a suffix reindex after the head offset has advanced, then + // trim the old head. The late row becomes the new first element. + let late = makeMessage(id: "late", timestamp: TimeInterval(overflow) + 0.5) + #expect(store.append(late, to: .mesh)) + #expect(conversation.messages.first?.id == "late") + #expect(conversation.message(withID: "m\(overflow + 1)")?.id == "m\(overflow + 1)") + + // Head and middle removals, an in-place upsert, and a status update + // must all resolve through the same logical index representation. + #expect(store.removeMessage(withID: "late", from: .mesh)?.id == "late") + let middleID = "m\(overflow + conversation.cap / 2)" + #expect(store.removeMessage(withID: middleID, from: .mesh)?.id == middleID) + + let probeID = "m\(overflow + 10)" + store.upsertByID( + makeMessage(id: probeID, timestamp: TimeInterval(overflow + 10), content: "edited"), + in: .mesh + ) + #expect(conversation.message(withID: probeID)?.content == "edited") + #expect(store.setDeliveryStatus(.sent, forMessageID: probeID, in: .mesh)) + #expect(conversation.message(withID: probeID)?.deliveryStatus == .sent) + #expect(store.auditInvariants().isEmpty) + + // Clearing resets the logical offset as well as the maps. + store.clear(.mesh) + #expect(store.append(makeMessage(id: "after-clear", timestamp: 10_000), to: .mesh)) + #expect(conversation.message(withID: "after-clear")?.id == "after-clear") + #expect(store.auditInvariants().isEmpty) + } + + @Test("logical index offset matches a reference model under adversarial mutations") + @MainActor + func logicalIndexOffsetDifferentialStress() { + let store = ConversationStore() + let cap = store.conversation(for: .mesh).cap + var reference = ReferenceConversationTimeline(cap: cap) + var rng = ConversationStoreDifferentialRNG(seed: 0xC0FF_EE13_37CA_FE42) + var issuedIDs: [String] = [] + var nextID = 0 + var nextTailTimestamp: TimeInterval = 1_700_000_000 + var trimmedCount = 0 + + var tailAppendCount = 0 + var outOfOrderCount = 0 + var duplicateOrReuseCount = 0 + var headRemovalCount = 0 + var middleRemovalCount = 0 + var upsertCount = 0 + var deliveryUpdateCount = 0 + var filterCount = 0 + var clearCount = 0 + + func issueMessage(timestamp: TimeInterval? = nil, tag: String) -> BitchatMessage { + let number = nextID + nextID += 1 + let id = "diff-\(number)" + issuedIDs.append(id) + let resolvedTimestamp: TimeInterval + if let timestamp { + resolvedTimestamp = timestamp + } else { + resolvedTimestamp = nextTailTimestamp + nextTailTimestamp += 1 + } + let dropMarker = number.isMultiple(of: 11) ? " [drop]" : "" + return makeMessage( + id: id, + timestamp: resolvedTimestamp, + content: "\(tag) \(number)\(dropMarker)" + ) + } + + @discardableResult + func appendAndCompare(_ message: BitchatMessage, checkpoint: String) -> ReferenceConversationTimeline.AppendResult { + let expected = reference.append(message) + let actual = store.append(message, to: .mesh) + #expect(actual == expected.inserted, "append result mismatch at \(checkpoint)") + trimmedCount += expected.trimmedCount + return expected + } + + func refill(extra: Int, checkpoint: String) { + let appendCount = max(0, cap - reference.messages.count) + extra + for index in 0.. 1_200) + #expect(tailAppendCount > 300) + #expect(outOfOrderCount > 150) + #expect(duplicateOrReuseCount > 75) + #expect(headRemovalCount > 50) + #expect(middleRemovalCount > 50) + #expect(upsertCount > 75) + #expect(deliveryUpdateCount > 75) + #expect(filterCount == 2) + #expect(clearCount == 1) + } + // MARK: - Upsert @Test("upsertByID replaces in place and appends when absent") diff --git a/bitchatTests/Performance/PerformanceBaselineTests.swift b/bitchatTests/Performance/PerformanceBaselineTests.swift index 0cfb6f92..949ca60a 100644 --- a/bitchatTests/Performance/PerformanceBaselineTests.swift +++ b/bitchatTests/Performance/PerformanceBaselineTests.swift @@ -501,6 +501,62 @@ final class PerformanceBaselineTests: XCTestCase { reportThroughput("store.append", samples: samples, operations: messageCount, unit: "messages") } + // MARK: - 7b. ConversationStore append at the retention cap + + /// Steady-state public timeline traffic after the 1337-message retention + /// cap has been reached. Every tail append evicts the oldest row, which is + /// the long-lived workload the cold `store.append` benchmark does not + /// exercise. + func testConversationStoreSteadyStateAppend() { + let store = ConversationStore() + let cap = TransportConfig.meshTimelineCap + let messagesPerPass = 500 + let base = Date(timeIntervalSince1970: 1_700_000_000) + + for i in 0..