diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt index 44361baa73..d07bd047b4 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt @@ -696,10 +696,17 @@ object ChatModel { } } - suspend fun removeMemberItems(rhId: Long?, removedMember: GroupMember, byMember: GroupMember, groupInfo: GroupInfo) { - fun isRemovedMemberItem(item: ChatItem): Boolean = when { - item.chatDir is CIDirection.GroupSnd -> removedMember.groupMemberId == groupInfo.membership.groupMemberId - item.chatDir is CIDirection.GroupRcv -> item.chatDir.groupMember.groupMemberId == removedMember.groupMemberId + suspend fun removeMemberItems(rhId: Long?, removedMember: GroupMember, byMember: GroupMember, groupInfo: GroupInfo) = + removeMemberItems(rhId, listOf(removedMember), byMember, groupInfo) + + // Batched variant: scans chat items once for all removed members, avoiding O(members * items) + // rebuilds of the chat items list when removing many members at once. + suspend fun removeMemberItems(rhId: Long?, removedMembers: List, byMember: GroupMember, groupInfo: GroupInfo) { + val removedMemberIds = removedMembers.mapTo(mutableSetOf()) { it.groupMemberId } + val membershipRemoved = groupInfo.membership.groupMemberId in removedMemberIds + fun isRemovedMemberItem(item: ChatItem): Boolean = when (val dir = item.chatDir) { + is CIDirection.GroupSnd -> membershipRemoved + is CIDirection.GroupRcv -> dir.groupMember.groupMemberId in removedMemberIds else -> false } fun markedUpdatedItem(item: ChatItem): ChatItem? { @@ -971,6 +978,68 @@ object ChatModel { } } + // Batched variant of [upsertGroupMember]: scans chat items and rebuilds the members list once + // for all members, avoiding O(members * items) work when upserting many members at once. + suspend fun upsertGroupMembers(rhId: Long?, groupInfo: GroupInfo, members: List) { + if (members.isEmpty()) return + // the user's own membership was updated + if (members.any { it.groupMemberId == groupInfo.membership.groupMemberId }) { + updateGroup(rhId, groupInfo) + } + // update current chat or channel being created + if (chatId.value != groupInfo.id && creatingChannelId.value != groupInfo.id) return + if (groupMembers.value.isNotEmpty() && groupMembers.value.firstOrNull()?.groupId != groupInfo.groupId) { + // stale data, should be cleared at that point, otherwise, duplicated items will be here which will produce crashes in LazyColumn + groupMembers.value = emptyList() + groupMembersIndexes.value = emptyMap() + } + // index members to upsert by id, excluding the user's own membership (handled above) + val membersById = HashMap(members.size * 2) + for (m in members) { + if (m.groupMemberId != groupInfo.membership.groupMemberId) membersById[m.groupMemberId] = m + } + if (membersById.isEmpty()) return + // single pass over chat items + var itemsChanged = false + val updated = chatItems.value.map { item -> + val dir = item.chatDir + if (dir is CIDirection.GroupRcv) { + val m = membersById[dir.groupMember.groupMemberId] + // Take into account only specific changes, not all. Other member updates are not important and can be skipped + if (m != null && (dir.groupMember.image != m.image || + dir.groupMember.chatViewName != m.chatViewName || + dir.groupMember.blocked != m.blocked || + dir.groupMember.memberRole != m.memberRole) + ) { + itemsChanged = true + item.copy(chatDir = CIDirection.GroupRcv(m)) + } else item + } else item + } + if (itemsChanged) { + chatItems.replaceAll(updated) + } + // update the members list once + val gMembers = groupMembers.value.toMutableList() + val gmIndexes = groupMembersIndexes.value.toMutableMap() + var addedNew = false + for (m in members) { + if (m.groupMemberId == groupInfo.membership.groupMemberId) continue + val idx = gmIndexes[m.groupMemberId] + if (idx != null) { + gMembers[idx] = m + } else { + gMembers.add(m) + gmIndexes[m.groupMemberId] = gMembers.size - 1 + addedNew = true + } + } + groupMembers.value = gMembers + if (addedNew) { + groupMembersIndexes.value = gmIndexes + } + } + suspend fun updateGroupMemberConnectionStats(rhId: Long?, groupInfo: GroupInfo, member: GroupMember, connectionStats: ConnectionStats) { val memberConn = member.activeConn if (memberConn != null) { diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/group/GroupChatInfoView.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/group/GroupChatInfoView.kt index 2b1c7bcd09..87cbd22865 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/group/GroupChatInfoView.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/group/GroupChatInfoView.kt @@ -1369,19 +1369,15 @@ fun removeMembers(rhId: Long?, groupInfo: GroupInfo, memberIds: List, with val (updatedGroupInfo, updatedMembers) = r withContext(Dispatchers.Main) { chatModel.chatsContext.updateGroup(rhId, updatedGroupInfo) - updatedMembers.forEach { updatedMember -> - chatModel.chatsContext.upsertGroupMember(rhId, updatedGroupInfo, updatedMember) - if (withMessages) { - chatModel.chatsContext.removeMemberItems(rhId, updatedMember, byMember = groupInfo.membership, groupInfo) - } + chatModel.chatsContext.upsertGroupMembers(rhId, updatedGroupInfo, updatedMembers) + if (withMessages) { + chatModel.chatsContext.removeMemberItems(rhId, updatedMembers, byMember = groupInfo.membership, groupInfo) } } withContext(Dispatchers.Main) { - updatedMembers.forEach { updatedMember -> - chatModel.secondaryChatsContext.value?.upsertGroupMember(rhId, updatedGroupInfo, updatedMember) - if (withMessages) { - chatModel.chatsContext.removeMemberItems(rhId, updatedMember, byMember = groupInfo.membership, groupInfo) - } + chatModel.secondaryChatsContext.value?.upsertGroupMembers(rhId, updatedGroupInfo, updatedMembers) + if (withMessages) { + chatModel.secondaryChatsContext.value?.removeMemberItems(rhId, updatedMembers, byMember = groupInfo.membership, groupInfo) } } onSuccess() diff --git a/apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/RemoveMembersPerfTest.kt b/apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/RemoveMembersPerfTest.kt new file mode 100644 index 0000000000..4cf143dc74 --- /dev/null +++ b/apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/RemoveMembersPerfTest.kt @@ -0,0 +1,150 @@ +package chat.simplex.app + +import chat.simplex.common.model.* +import chat.simplex.common.model.ChatModel.ChatsContext +import kotlinx.coroutines.runBlocking +import kotlin.test.* + +/** + * Correctness + performance test for the batched group-member removal path + * ([ChatsContext.upsertGroupMembers] and the batched [ChatsContext.removeMemberItems]). + * + * Reproduces the reported slowdown scenario: removing 100 members from a group whose + * chat is open and holds many items. The old code rebuilt the whole item list once per + * removed member (O(members * items)); the batched code does it once (O(members + items)). + * + * The test runs the OLD per-member loop and the NEW batched call on identical fresh state, + * asserts they produce the same result (and the correct result), and prints the speedup. + */ +class RemoveMembersPerfTest { + private val rhId: Long? = null + private val N_MEMBERS = 100 + private val ITEMS_PER_MEMBER = 20 + private val KEEPER_ITEMS = 50 + private val KEEPER_ID = 500L + private val MEMBERSHIP_ID = 1000L + + // the user's own membership (owner) — never part of the removed set + private val membership = GroupMember.sampleData.copy( + groupMemberId = MEMBERSHIP_ID, groupId = 1L, memberId = "self", memberRole = GroupMemberRole.Owner + ) + + // group with fullDelete ON so removeMemberItems takes the (expensive) removeAllAndNotify branch + private val groupInfo = GroupInfo.sampleData.copy( + groupId = 1L, + membership = membership, + fullGroupPreferences = GroupInfo.sampleData.fullGroupPreferences.copy( + fullDelete = GroupPreference(GroupFeatureEnabled.ON, role = null) + ) + ) + + // a member that is NOT removed — its items and entry must survive + private val keeper = GroupMember.sampleData.copy( + groupMemberId = KEEPER_ID, groupId = 1L, memberId = "keeper", localDisplayName = "keeper" + ) + + // the 100 members present in the group before removal + private val presentMembers: List = (1L..N_MEMBERS).map { id -> + GroupMember.sampleData.copy( + groupMemberId = id, groupId = 1L, memberId = "mid_$id", localDisplayName = "member_$id", + memberStatus = GroupMemberStatus.MemComplete + ) + } + + // what apiRemoveMembers returns: the same members with an updated status + private val removedMembers: List = presentMembers.map { it.copy(memberStatus = GroupMemberStatus.MemRemoved) } + + private fun buildItems(): List { + var id = 1L + val items = ArrayList(N_MEMBERS * ITEMS_PER_MEMBER + KEEPER_ITEMS) + for (m in presentMembers) { + repeat(ITEMS_PER_MEMBER) { items.add(ChatItem.getSampleData(id = id++, dir = CIDirection.GroupRcv(m))) } + } + repeat(KEEPER_ITEMS) { items.add(ChatItem.getSampleData(id = id++, dir = CIDirection.GroupRcv(keeper))) } + return items + } + + // chatId / groupMembers / groupMembersIndexes are shared state on the ChatModel object; + // only chatItems (and chatState) are per-context. + private fun ChatsContext.seed() { + ChatModel.chatId.value = groupInfo.id + chatItems.replaceAll(buildItems()) + val all = presentMembers + keeper + ChatModel.groupMembers.value = all + ChatModel.groupMembersIndexes.value = all.mapIndexed { i, m -> m.groupMemberId to i }.toMap() + } + + private data class Snapshot(val itemIds: List, val members: List) + + private fun ChatsContext.snapshot() = Snapshot( + itemIds = chatItems.value.map { it.id }, + members = ChatModel.groupMembers.value.toList() + ) + + private fun assertCorrect(s: Snapshot) { + // only the keeper's items remain; all removed members' items are gone + assertEquals(KEEPER_ITEMS, s.itemIds.size, "only keeper items should remain") + // every removed member is now marked removed, keeper is untouched + val byId = s.members.associateBy { it.groupMemberId } + assertTrue((1L..N_MEMBERS).all { byId[it]?.memberStatus == GroupMemberStatus.MemRemoved }, "removed members must be updated") + assertEquals(GroupMemberStatus.MemComplete, byId[KEEPER_ID]?.memberStatus, "keeper must be untouched") + } + + @Test + fun batchedRemovalIsCorrectAndFaster() { + val ctx = ChatModel.chatsContext + + // OLD: per-member loop (mirrors the pre-fix removeMembers body) + ctx.seed() + val oldNanos = measure { + runBlocking { + for (m in removedMembers) { + ctx.upsertGroupMember(rhId, groupInfo, m) + ctx.removeMemberItems(rhId, m, byMember = membership, groupInfo) + } + } + } + val oldResult = ctx.snapshot() + + // NEW: batched calls (the fixed removeMembers body) + ctx.seed() + val newNanos = measure { + runBlocking { + ctx.upsertGroupMembers(rhId, groupInfo, removedMembers) + ctx.removeMemberItems(rhId, removedMembers, byMember = membership, groupInfo) + } + } + val newResult = ctx.snapshot() + + // correctness + assertCorrect(oldResult) + assertCorrect(newResult) + assertEquals(oldResult.itemIds.sorted(), newResult.itemIds.sorted(), "batched must remove the same items as the loop") + assertEquals(oldResult.members, newResult.members, "batched must produce the same members list as the loop") + + // performance + val oldMs = oldNanos / 1_000_000.0 + val newMs = newNanos / 1_000_000.0 + val speedup = oldNanos.toDouble() / newNanos.coerceAtLeast(1) + println( + """ + | + |========= remove-members performance ========= + | members removed : $N_MEMBERS + | chat items : ${N_MEMBERS * ITEMS_PER_MEMBER + KEEPER_ITEMS} + | old (per-member): ${"%.1f".format(oldMs)} ms O(members * items) + | new (batched) : ${"%.1f".format(newMs)} ms O(members + items) + | speedup : ${"%.1f".format(speedup)}x + |============================================== + """.trimMargin() + ) + + assertTrue(newNanos * 2 < oldNanos, "batched should be clearly faster: old=${"%.1f".format(oldMs)}ms new=${"%.1f".format(newMs)}ms (${"%.1f".format(speedup)}x)") + } + + private inline fun measure(block: () -> Unit): Long { + val start = System.nanoTime() + block() + return System.nanoTime() - start + } +} diff --git a/plans/2026-06-29-fix-bulk-remove-members-perf.md b/plans/2026-06-29-fix-bulk-remove-members-perf.md new file mode 100644 index 0000000000..17392d9feb --- /dev/null +++ b/plans/2026-06-29-fix-bulk-remove-members-perf.md @@ -0,0 +1,130 @@ +# Fix: bulk "remove members" freezes UI for minutes (O(members × items)) + +Date: 2026-06-29 +Branch: nd/fix-bulk-remove-members-perf + +## Symptom + +Slow-execution diagnostic fired on desktop: + +``` +Execution of function takes too long time: 353 seconds: java.lang.Exception + at chat.simplex.common.views.helpers.UtilsKt.withBGApi(Utils.kt:39) + at chat.simplex.common.views.chat.group.GroupChatInfoViewKt.removeMembers(GroupChatInfoView.kt:1366) +``` + +`withBGApi` (`Utils.kt:38`) wraps work in `wrapWithLogging`, which logs an exception when the block +exceeds the `slow` threshold (20 s). The whole `removeMembers { … }` block ran for 353 s, freezing the +UI (all of the work runs on `Dispatchers.Main`). + +## Root cause (multiplatform — Android + desktop) + +`removeMembers` (`GroupChatInfoView.kt`) is reachable from **bulk multi-select removal** +(`GroupChatInfoView.kt:838`, `removeMembersAlert(rhId, groupInfo, selectedItems.value!!.sorted())`), +so the member list `M` can be large ("select all → remove"). + +The body looped over every removed member and, **per member**, rebuilt the entire open-chat item list: + +- `ChatsContext.upsertGroupMember` (`ChatModel.kt`): `chatItems.value.map { … }` (O(N)) + + `replaceAll` (O(N)) — `replaceAll` allocates a brand-new `SnapshotStateList` and copies all + N items (`ChatModel.kt`, `fun MutableState>.replaceAll`). +- `ChatsContext.removeMemberItems` (`ChatModel.kt`), when `withMessages` and `fullDelete`: + `removeAllAndNotify` — another full O(N) `SnapshotStateList` rebuild. + +With `M` removed members and `N` items in the open chat this is **O(M × N)** snapshot rebuilds, plus `M` +LazyColumn recompositions, all on the Main thread → 353 s. + +Secondary bug: the second `withContext(Dispatchers.Main)` block (intended for `secondaryChatsContext`) +called `chatModel.chatsContext.removeMemberItems(...)` (primary context) instead of +`secondaryChatsContext` — so removeMemberItems ran on the primary context **twice** and was skipped on +the secondary context. + +## Fix (batch the model ops → O(N)) + +Process the open-chat item list **once** for the whole set of removed members instead of once per member. + +### `common/.../model/ChatModel.kt` + +- Added batched `removeMemberItems(rhId, removedMembers: List, byMember, groupInfo)`: + a single pass over chat items, matching any item whose member is in the removed set + (`isRemovedMemberItem` now tests membership against a `Set` of removed ids). + The original single-member overload now delegates: + `removeMemberItems(rhId, removedMember, …) = removeMemberItems(rhId, listOf(removedMember), …)` + — provably equivalent (for one id, `id in {x}` ≡ `id == x`). +- Added batched `upsertGroupMembers(rhId, groupInfo, members: List)`: + one pass over `chatItems` (single `replaceAll` only if something changed) and one rebuild of the + members list + indexes for all members. The single-member `upsertGroupMember` is left untouched + (used by many event handlers); its `Boolean` return is unused by all callers. + +### `common/.../views/chat/group/GroupChatInfoView.kt` + +`removeMembers` now calls the batched functions once per context, and the second block correctly targets +`secondaryChatsContext` (bug fixed): + +```kotlin +withContext(Dispatchers.Main) { + chatModel.chatsContext.updateGroup(rhId, updatedGroupInfo) + chatModel.chatsContext.upsertGroupMembers(rhId, updatedGroupInfo, updatedMembers) + if (withMessages) { + chatModel.chatsContext.removeMemberItems(rhId, updatedMembers, byMember = groupInfo.membership, groupInfo) + } +} +withContext(Dispatchers.Main) { + chatModel.secondaryChatsContext.value?.upsertGroupMembers(rhId, updatedGroupInfo, updatedMembers) + if (withMessages) { + chatModel.secondaryChatsContext.value?.removeMemberItems(rhId, updatedMembers, byMember = groupInfo.membership, groupInfo) + } +} +``` + +Complexity: **O(M × N) → O(N + M)**. + +The `withBGApi` slow threshold was intentionally left unchanged — with the quadratic gone the diagnostic +will not fire for normal removals, and keeping the 20 s threshold preserves its value as a real-problem +signal. + +## iOS: not affected (no fix needed) + +The same per-member loop exists in iOS (`apps/ios/.../GroupChatInfoView.swift` `removeMember`, +`apps/ios/.../ChatModel.swift` `removeMemberItems`/`upsertGroupMember`), **but the quadratic cannot +occur on iOS**: + +- iOS has **no bulk multi-select member removal**. `apiRemoveMembers` is only ever called with a single + member: `apiRemoveMembers(groupInfo.groupId, [mem.groupMemberId], withMessages)` + (`GroupChatInfoView.swift:1006` — the only call site in the app). So `M` is always 1 and the loop runs + once → O(N). +- iOS `upsertGroupMember` (`ChatModel.swift:1280`) only updates the `groupMembers` array; it does **not** + scan `reversedChatItems`, so it lacks even the per-call O(N) item pass that the desktop version has. + +If iOS ever gains bulk member removal, mirror the multiplatform batching (batched `removeMemberItems` + +batched `upsertGroupMembers`) at that time. + +## Verification + +- `./gradlew :common:compileKotlinDesktop` — BUILD SUCCESSFUL (only pre-existing deprecation warnings). +- `./gradlew :common:desktopTest --tests "chat.simplex.app.RemoveMembersPerfTest"` — passes. + +### Performance test + +`RemoveMembersPerfTest` builds a group with 100 members whose open chat holds 2050 items, +then runs the OLD per-member loop and the NEW batched calls on identical fresh state. It +asserts both produce the same (and correct) result — only the non-removed member's items +remain, and removed members are marked removed — and that the batched path is faster. + +Measured (JVM, one run): + +``` +========= remove-members performance ========= + members removed : 100 + chat items : 2050 + old (per-member): 1291.3 ms O(members * items) + new (batched) : 12.1 ms O(members + items) + speedup : 107.0x +============================================== +``` + +## Files changed + +- `apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt` +- `apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/group/GroupChatInfoView.kt` +- `apps/multiplatform/common/src/desktopTest/kotlin/chat/simplex/app/RemoveMembersPerfTest.kt`