android, desktop: fix slow bulk member removal (O(members * items))

Bulk removing group members rebuilt the entire open-chat item list once per removed member (per-member upsertGroupMember + removeMemberItems, each a full SnapshotStateList rebuild on the Main thread), giving O(members * items) work that froze the UI for minutes on large groups. Batch the model updates so chat items and the members list are scanned and rebuilt once for all removed members: add batched ChatsContext.removeMemberItems(List<GroupMember>) and upsertGroupMembers, and call them once per context from removeMembers. Also fix the second update block to target secondaryChatsContext instead of the primary one.
2026-07-10 10:01:50 +00:00 · 2026-06-29 19:17:03 +00:00
parent 2c2337b07c
commit 226c9a7c8d
4 changed files with 359 additions and 14 deletions
@@ -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<GroupMember>, 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<GroupMember>) {
+      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<Long, GroupMember>(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) {
@@ -1369,19 +1369,15 @@ fun removeMembers(rhId: Long?, groupInfo: GroupInfo, memberIds: List<Long>, 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()
@@ -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<GroupMember> = (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<GroupMember> = presentMembers.map { it.copy(memberStatus = GroupMemberStatus.MemRemoved) }
+
+  private fun buildItems(): List<ChatItem> {
+    var id = 1L
+    val items = ArrayList<ChatItem>(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<Long>, val members: List<GroupMember>)
+
+  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
+  }
+}
@@ -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<ChatItem>` and copies all
+  N items (`ChatModel.kt`, `fun MutableState<SnapshotStateList<T>>.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<GroupMember>, 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<Long>` 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<GroupMember>)`:
+  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`