From 9946ba8aa4bdf4bd9b5f27041641fdd3c59667d6 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Mar 2021 11:07:31 +0100 Subject: [PATCH 1/6] Split network request `/keys/query` into smaller requests (250 users max) (#2925) --- CHANGES.md | 2 +- .../crypto/tasks/DownloadKeysForUsersTask.kt | 63 ++++++++++++++++--- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index cad62d0851..594ac8e84d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,7 +5,7 @@ Features ✨: - Improvements 🙌: - - + - Split network request `/keys/query` into smaller requests (250 users max) (#2925) Bugfix 🐛: - diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt index 5eb24b116a..c78a5cb74c 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt @@ -16,13 +16,21 @@ package org.matrix.android.sdk.internal.crypto.tasks +import kotlinx.coroutines.async +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.joinAll +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import org.matrix.android.sdk.internal.crypto.api.CryptoApi +import org.matrix.android.sdk.internal.crypto.model.rest.DeviceKeysWithUnsigned import org.matrix.android.sdk.internal.crypto.model.rest.KeysQueryBody import org.matrix.android.sdk.internal.crypto.model.rest.KeysQueryResponse +import org.matrix.android.sdk.internal.crypto.model.rest.RestKeyInfo import org.matrix.android.sdk.internal.network.GlobalErrorReceiver import org.matrix.android.sdk.internal.network.executeRequest import org.matrix.android.sdk.internal.task.Task import javax.inject.Inject +import kotlin.math.ceil internal interface DownloadKeysForUsersTask : Task { data class Params( @@ -39,15 +47,56 @@ internal class DefaultDownloadKeysForUsers @Inject constructor( ) : DownloadKeysForUsersTask { override suspend fun execute(params: DownloadKeysForUsersTask.Params): KeysQueryResponse { - val downloadQuery = params.userIds.associateWith { emptyList() } + val numberOfChunks = ceil(params.userIds.size / LIMIT.toDouble()).toInt().coerceAtLeast(1) + val chunkSize = params.userIds.size / numberOfChunks - val body = KeysQueryBody( - deviceKeys = downloadQuery, - token = params.token?.takeIf { it.isNotEmpty() } - ) + // Store server results in these mutable maps + val deviceKeys = mutableMapOf>() + val failures = mutableMapOf>() + val masterKeys = mutableMapOf() + val selfSigningKeys = mutableMapOf() + val userSigningKeys = mutableMapOf() - return executeRequest(globalErrorReceiver) { - apiCall = cryptoApi.downloadKeysForUsers(body) + val mutex = Mutex() + + // Split network request into smaller request (#2925) + coroutineScope { + params.userIds + .chunked(chunkSize) + .map { + KeysQueryBody( + deviceKeys = it.associateWith { emptyList() }, + token = params.token?.takeIf { token -> token.isNotEmpty() } + ) + } + .map { body -> + async { + val result = executeRequest(globalErrorReceiver) { + apiCall = cryptoApi.downloadKeysForUsers(body) + } + + mutex.withLock { + deviceKeys.putAll(result.deviceKeys.orEmpty()) + failures.putAll(result.failures.orEmpty()) + masterKeys.putAll(result.masterKeys.orEmpty()) + selfSigningKeys.putAll(result.selfSigningKeys.orEmpty()) + userSigningKeys.putAll(result.userSigningKeys.orEmpty()) + } + } + } + .joinAll() } + + return KeysQueryResponse( + deviceKeys = deviceKeys, + failures = failures, + masterKeys = masterKeys, + selfSigningKeys = selfSigningKeys, + userSigningKeys = userSigningKeys + ) + } + + companion object { + const val LIMIT = 250 } } From 103ba463c38ec5532f8ebb28d09897db237d2f69 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Mar 2021 11:34:27 +0100 Subject: [PATCH 2/6] Create getBetsChunkSize to avoid code duplication --- .../crypto/tasks/DownloadKeysForUsersTask.kt | 9 +-- .../internal/session/sync/RoomSyncHandler.kt | 18 ++--- .../android/sdk/internal/util/MathUtils.kt | 43 +++++++++++ .../android/sdk/internal/util/MathUtilTest.kt | 73 +++++++++++++++++++ 4 files changed, 129 insertions(+), 14 deletions(-) create mode 100644 matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt create mode 100644 matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt index c78a5cb74c..3f1f5299d0 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt @@ -29,12 +29,12 @@ import org.matrix.android.sdk.internal.crypto.model.rest.RestKeyInfo import org.matrix.android.sdk.internal.network.GlobalErrorReceiver import org.matrix.android.sdk.internal.network.executeRequest import org.matrix.android.sdk.internal.task.Task +import org.matrix.android.sdk.internal.util.getBetsChunkSize import javax.inject.Inject -import kotlin.math.ceil internal interface DownloadKeysForUsersTask : Task { data class Params( - // the list of users to get keys for. + // the list of users to get keys for. The list MUST NOT be empty val userIds: List, // the up-to token val token: String? @@ -47,8 +47,7 @@ internal class DefaultDownloadKeysForUsers @Inject constructor( ) : DownloadKeysForUsersTask { override suspend fun execute(params: DownloadKeysForUsersTask.Params): KeysQueryResponse { - val numberOfChunks = ceil(params.userIds.size / LIMIT.toDouble()).toInt().coerceAtLeast(1) - val chunkSize = params.userIds.size / numberOfChunks + val bestChunkSize = getBetsChunkSize(params.userIds.size, LIMIT) // Store server results in these mutable maps val deviceKeys = mutableMapOf>() @@ -62,7 +61,7 @@ internal class DefaultDownloadKeysForUsers @Inject constructor( // Split network request into smaller request (#2925) coroutineScope { params.userIds - .chunked(chunkSize) + .chunked(bestChunkSize.chunkSize) .map { KeysQueryBody( deviceKeys = it.associateWith { emptyList() }, diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/RoomSyncHandler.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/RoomSyncHandler.kt index 0f97d0cb39..4553ff90ed 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/RoomSyncHandler.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/RoomSyncHandler.kt @@ -64,9 +64,9 @@ import org.matrix.android.sdk.internal.session.sync.model.LazyRoomSyncEphemeral import org.matrix.android.sdk.internal.session.sync.model.RoomSync import org.matrix.android.sdk.internal.session.sync.model.RoomSyncAccountData import org.matrix.android.sdk.internal.session.sync.model.RoomsSyncResponse +import org.matrix.android.sdk.internal.util.getBetsChunkSize import timber.log.Timber import javax.inject.Inject -import kotlin.math.ceil internal class RoomSyncHandler @Inject constructor(private val readReceiptHandler: ReadReceiptHandler, private val roomSummaryUpdater: RoomSummaryUpdater, @@ -140,17 +140,17 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle syncLocalTimeStampMillis: Long, aggregator: SyncResponsePostTreatmentAggregator, reporter: ProgressReporter?) { - val maxSize = (initialSyncStrategy as? InitialSyncStrategy.Optimized)?.maxRoomsToInsert ?: Int.MAX_VALUE - val listSize = handlingStrategy.data.keys.size - val numberOfChunks = ceil(listSize / maxSize.toDouble()).toInt() + val bestChunkSize = getBetsChunkSize( + listSize = handlingStrategy.data.keys.size, + limit = (initialSyncStrategy as? InitialSyncStrategy.Optimized)?.maxRoomsToInsert ?: Int.MAX_VALUE + ) - if (numberOfChunks > 1) { - reportSubtask(reporter, InitSyncStep.ImportingAccountJoinedRooms, numberOfChunks, 0.6f) { - val chunkSize = listSize / numberOfChunks - Timber.d("INIT_SYNC $listSize rooms to insert, split into $numberOfChunks sublists of $chunkSize items") + if (bestChunkSize.shouldSplit()) { + reportSubtask(reporter, InitSyncStep.ImportingAccountJoinedRooms, bestChunkSize.numberOfChunks, 0.6f) { + Timber.d("INIT_SYNC ${handlingStrategy.data.keys.size} rooms to insert, split with $bestChunkSize") // I cannot find a better way to chunk a map, so chunk the keys and then create new maps handlingStrategy.data.keys - .chunked(chunkSize) + .chunked(bestChunkSize.chunkSize) .forEachIndexed { index, roomIds -> val roomEntities = roomIds .also { Timber.d("INIT_SYNC insert ${roomIds.size} rooms") } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt new file mode 100644 index 0000000000..6abf917ab0 --- /dev/null +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2021 The Matrix.org Foundation C.I.C. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.matrix.android.sdk.internal.util + +import kotlin.math.ceil + +internal data class BestChunkSize( + val numberOfChunks: Int, + val chunkSize: Int +) { + fun shouldSplit() = numberOfChunks > 1 +} + +internal fun getBetsChunkSize(listSize: Int, limit: Int): BestChunkSize { + return if (listSize <= limit) { + BestChunkSize( + numberOfChunks = 1, + chunkSize = listSize + ) + } else { + val numberOfChunks = ceil(listSize / limit.toDouble()).toInt() + val chunkSize = listSize / numberOfChunks + + BestChunkSize( + numberOfChunks = numberOfChunks, + chunkSize = chunkSize + ) + } +} diff --git a/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt b/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt new file mode 100644 index 0000000000..7cb355a621 --- /dev/null +++ b/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt @@ -0,0 +1,73 @@ +/* + * Copyright 2020 The Matrix.org Foundation C.I.C. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.matrix.android.sdk.internal.util + +import org.amshove.kluent.shouldBeEqualTo +import org.amshove.kluent.shouldHaveSize +import org.junit.FixMethodOrder +import org.junit.Test +import org.junit.runners.MethodSorters +import org.matrix.android.sdk.MatrixTest + +@FixMethodOrder(MethodSorters.JVM) +class MathUtilTest : MatrixTest { + + @Test + fun testGetBestChunkSize0() = doTest(0, 100, 1, 0) + + @Test + fun testGetBestChunkSize1() = doTest(1, 100, 1, 1) + + @Test + fun testGetBestChunkSize5() = doTest(5, 100, 1, 5) + + @Test + fun testGetBestChunkSize99() = doTest(99, 100, 1, 99) + + @Test + fun testGetBestChunkSize100() = doTest(100, 100, 1, 100) + + @Test + fun testGetBestChunkSize101() = doTest(101, 100, 2, 50) + + @Test + fun testGetBestChunkSize199() = doTest(199, 100, 2, 99) + + @Test + fun testGetBestChunkSize200() = doTest(200, 100, 2, 100) + + @Test + fun testGetBestChunkSize201() = doTest(201, 100, 3, 67) + + @Test + fun testGetBestChunkSize240() = doTest(240, 100, 3, 80) + + private fun doTest(listSize: Int, limit: Int, expectedNumberOfChunks: Int, expectedChunkSize: Int) { + val result = getBetsChunkSize(listSize, limit) + + result.numberOfChunks shouldBeEqualTo expectedNumberOfChunks + result.chunkSize shouldBeEqualTo expectedChunkSize + + // Test that the result make sense, when we use chunked() + if (result.chunkSize > 0) { + generateSequence { "a" } + .take(listSize) + .chunked(result.chunkSize) + .shouldHaveSize(result.numberOfChunks) + } + } +} From da9f0c66671d4c964dcc43833d68ade797aa690f Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Mar 2021 12:03:01 +0100 Subject: [PATCH 3/6] Fix an issue discovered by unit test --- .../java/org/matrix/android/sdk/internal/util/MathUtils.kt | 3 ++- .../java/org/matrix/android/sdk/internal/util/MathUtilTest.kt | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt index 6abf917ab0..0e18b62acd 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt @@ -33,7 +33,8 @@ internal fun getBetsChunkSize(listSize: Int, limit: Int): BestChunkSize { ) } else { val numberOfChunks = ceil(listSize / limit.toDouble()).toInt() - val chunkSize = listSize / numberOfChunks + // Round on next Int + val chunkSize = ceil(listSize / numberOfChunks.toDouble()).toInt() BestChunkSize( numberOfChunks = numberOfChunks, diff --git a/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt b/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt index 7cb355a621..f684d93310 100644 --- a/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt +++ b/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt @@ -42,10 +42,10 @@ class MathUtilTest : MatrixTest { fun testGetBestChunkSize100() = doTest(100, 100, 1, 100) @Test - fun testGetBestChunkSize101() = doTest(101, 100, 2, 50) + fun testGetBestChunkSize101() = doTest(101, 100, 2, 51) @Test - fun testGetBestChunkSize199() = doTest(199, 100, 2, 99) + fun testGetBestChunkSize199() = doTest(199, 100, 2, 100) @Test fun testGetBestChunkSize200() = doTest(200, 100, 2, 100) From f6032da7885efca70074617d235ac5aade5d91e0 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 16 Mar 2021 12:05:36 +0100 Subject: [PATCH 4/6] Add more test --- .../matrix/android/sdk/internal/util/MathUtilTest.kt | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt b/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt index f684d93310..3abb3db6ca 100644 --- a/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt +++ b/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt @@ -30,13 +30,11 @@ class MathUtilTest : MatrixTest { fun testGetBestChunkSize0() = doTest(0, 100, 1, 0) @Test - fun testGetBestChunkSize1() = doTest(1, 100, 1, 1) - - @Test - fun testGetBestChunkSize5() = doTest(5, 100, 1, 5) - - @Test - fun testGetBestChunkSize99() = doTest(99, 100, 1, 99) + fun testGetBestChunkSize1to99() { + for (i in 1..99) { + doTest(i, 100, 1, i) + } + } @Test fun testGetBestChunkSize100() = doTest(100, 100, 1, 100) From 96b37a8206f8518924e059f428ee9202d4292dc6 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 18 Mar 2021 10:57:28 +0100 Subject: [PATCH 5/6] Fix typo --- .../crypto/tasks/DownloadKeysForUsersTask.kt | 4 ++-- .../internal/session/sync/RoomSyncHandler.kt | 6 +++--- .../android/sdk/internal/util/MathUtils.kt | 4 ++-- .../android/sdk/internal/util/MathUtilTest.kt | 18 +++++++++--------- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt index 3f1f5299d0..ccf374ad7c 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt @@ -29,7 +29,7 @@ import org.matrix.android.sdk.internal.crypto.model.rest.RestKeyInfo import org.matrix.android.sdk.internal.network.GlobalErrorReceiver import org.matrix.android.sdk.internal.network.executeRequest import org.matrix.android.sdk.internal.task.Task -import org.matrix.android.sdk.internal.util.getBetsChunkSize +import org.matrix.android.sdk.internal.util.computeBestChunkSize import javax.inject.Inject internal interface DownloadKeysForUsersTask : Task { @@ -47,7 +47,7 @@ internal class DefaultDownloadKeysForUsers @Inject constructor( ) : DownloadKeysForUsersTask { override suspend fun execute(params: DownloadKeysForUsersTask.Params): KeysQueryResponse { - val bestChunkSize = getBetsChunkSize(params.userIds.size, LIMIT) + val bestChunkSize = computeBestChunkSize(params.userIds.size, LIMIT) // Store server results in these mutable maps val deviceKeys = mutableMapOf>() diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/RoomSyncHandler.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/RoomSyncHandler.kt index 4553ff90ed..e938d54903 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/RoomSyncHandler.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/RoomSyncHandler.kt @@ -64,7 +64,7 @@ import org.matrix.android.sdk.internal.session.sync.model.LazyRoomSyncEphemeral import org.matrix.android.sdk.internal.session.sync.model.RoomSync import org.matrix.android.sdk.internal.session.sync.model.RoomSyncAccountData import org.matrix.android.sdk.internal.session.sync.model.RoomsSyncResponse -import org.matrix.android.sdk.internal.util.getBetsChunkSize +import org.matrix.android.sdk.internal.util.computeBestChunkSize import timber.log.Timber import javax.inject.Inject @@ -140,12 +140,12 @@ internal class RoomSyncHandler @Inject constructor(private val readReceiptHandle syncLocalTimeStampMillis: Long, aggregator: SyncResponsePostTreatmentAggregator, reporter: ProgressReporter?) { - val bestChunkSize = getBetsChunkSize( + val bestChunkSize = computeBestChunkSize( listSize = handlingStrategy.data.keys.size, limit = (initialSyncStrategy as? InitialSyncStrategy.Optimized)?.maxRoomsToInsert ?: Int.MAX_VALUE ) - if (bestChunkSize.shouldSplit()) { + if (bestChunkSize.shouldChunk()) { reportSubtask(reporter, InitSyncStep.ImportingAccountJoinedRooms, bestChunkSize.numberOfChunks, 0.6f) { Timber.d("INIT_SYNC ${handlingStrategy.data.keys.size} rooms to insert, split with $bestChunkSize") // I cannot find a better way to chunk a map, so chunk the keys and then create new maps diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt index 0e18b62acd..c9c597e93d 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/MathUtils.kt @@ -22,10 +22,10 @@ internal data class BestChunkSize( val numberOfChunks: Int, val chunkSize: Int ) { - fun shouldSplit() = numberOfChunks > 1 + fun shouldChunk() = numberOfChunks > 1 } -internal fun getBetsChunkSize(listSize: Int, limit: Int): BestChunkSize { +internal fun computeBestChunkSize(listSize: Int, limit: Int): BestChunkSize { return if (listSize <= limit) { BestChunkSize( numberOfChunks = 1, diff --git a/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt b/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt index 3abb3db6ca..ade811f9b7 100644 --- a/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt +++ b/matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/util/MathUtilTest.kt @@ -27,35 +27,35 @@ import org.matrix.android.sdk.MatrixTest class MathUtilTest : MatrixTest { @Test - fun testGetBestChunkSize0() = doTest(0, 100, 1, 0) + fun testComputeBestChunkSize0() = doTest(0, 100, 1, 0) @Test - fun testGetBestChunkSize1to99() { + fun testComputeBestChunkSize1to99() { for (i in 1..99) { doTest(i, 100, 1, i) } } @Test - fun testGetBestChunkSize100() = doTest(100, 100, 1, 100) + fun testComputeBestChunkSize100() = doTest(100, 100, 1, 100) @Test - fun testGetBestChunkSize101() = doTest(101, 100, 2, 51) + fun testComputeBestChunkSize101() = doTest(101, 100, 2, 51) @Test - fun testGetBestChunkSize199() = doTest(199, 100, 2, 100) + fun testComputeBestChunkSize199() = doTest(199, 100, 2, 100) @Test - fun testGetBestChunkSize200() = doTest(200, 100, 2, 100) + fun testComputeBestChunkSize200() = doTest(200, 100, 2, 100) @Test - fun testGetBestChunkSize201() = doTest(201, 100, 3, 67) + fun testComputeBestChunkSize201() = doTest(201, 100, 3, 67) @Test - fun testGetBestChunkSize240() = doTest(240, 100, 3, 80) + fun testComputeBestChunkSize240() = doTest(240, 100, 3, 80) private fun doTest(listSize: Int, limit: Int, expectedNumberOfChunks: Int, expectedChunkSize: Int) { - val result = getBetsChunkSize(listSize, limit) + val result = computeBestChunkSize(listSize, limit) result.numberOfChunks shouldBeEqualTo expectedNumberOfChunks result.chunkSize shouldBeEqualTo expectedChunkSize From dbff5015df3d4ce43627975f918c199dddb04215 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 18 Mar 2021 11:56:19 +0100 Subject: [PATCH 6/6] Keep is simple if there is no need to chunk --- .../crypto/tasks/DownloadKeysForUsersTask.kt | 89 +++++++++++-------- 1 file changed, 51 insertions(+), 38 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt index ccf374ad7c..0c17cbb43a 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DownloadKeysForUsersTask.kt @@ -48,51 +48,64 @@ internal class DefaultDownloadKeysForUsers @Inject constructor( override suspend fun execute(params: DownloadKeysForUsersTask.Params): KeysQueryResponse { val bestChunkSize = computeBestChunkSize(params.userIds.size, LIMIT) + val token = params.token?.takeIf { token -> token.isNotEmpty() } - // Store server results in these mutable maps - val deviceKeys = mutableMapOf>() - val failures = mutableMapOf>() - val masterKeys = mutableMapOf() - val selfSigningKeys = mutableMapOf() - val userSigningKeys = mutableMapOf() + return if (bestChunkSize.shouldChunk()) { + // Store server results in these mutable maps + val deviceKeys = mutableMapOf>() + val failures = mutableMapOf>() + val masterKeys = mutableMapOf() + val selfSigningKeys = mutableMapOf() + val userSigningKeys = mutableMapOf() - val mutex = Mutex() + val mutex = Mutex() - // Split network request into smaller request (#2925) - coroutineScope { - params.userIds - .chunked(bestChunkSize.chunkSize) - .map { - KeysQueryBody( - deviceKeys = it.associateWith { emptyList() }, - token = params.token?.takeIf { token -> token.isNotEmpty() } - ) - } - .map { body -> - async { - val result = executeRequest(globalErrorReceiver) { - apiCall = cryptoApi.downloadKeysForUsers(body) - } + // Split network request into smaller request (#2925) + coroutineScope { + params.userIds + .chunked(bestChunkSize.chunkSize) + .map { + KeysQueryBody( + deviceKeys = it.associateWith { emptyList() }, + token = token + ) + } + .map { body -> + async { + val result = executeRequest(globalErrorReceiver) { + apiCall = cryptoApi.downloadKeysForUsers(body) + } - mutex.withLock { - deviceKeys.putAll(result.deviceKeys.orEmpty()) - failures.putAll(result.failures.orEmpty()) - masterKeys.putAll(result.masterKeys.orEmpty()) - selfSigningKeys.putAll(result.selfSigningKeys.orEmpty()) - userSigningKeys.putAll(result.userSigningKeys.orEmpty()) + mutex.withLock { + deviceKeys.putAll(result.deviceKeys.orEmpty()) + failures.putAll(result.failures.orEmpty()) + masterKeys.putAll(result.masterKeys.orEmpty()) + selfSigningKeys.putAll(result.selfSigningKeys.orEmpty()) + userSigningKeys.putAll(result.userSigningKeys.orEmpty()) + } } } - } - .joinAll() - } + .joinAll() + } - return KeysQueryResponse( - deviceKeys = deviceKeys, - failures = failures, - masterKeys = masterKeys, - selfSigningKeys = selfSigningKeys, - userSigningKeys = userSigningKeys - ) + KeysQueryResponse( + deviceKeys = deviceKeys, + failures = failures, + masterKeys = masterKeys, + selfSigningKeys = selfSigningKeys, + userSigningKeys = userSigningKeys + ) + } else { + // No need to chunk, direct request + executeRequest(globalErrorReceiver) { + apiCall = cryptoApi.downloadKeysForUsers( + KeysQueryBody( + deviceKeys = params.userIds.associateWith { emptyList() }, + token = token + ) + ) + } + } } companion object {