ElementR | backup: call expensive `roomKeyCounts` less often #4015

BillCarsonFr · 2024-01-18T13:06:09Z

Fixes element-hq/element-web#26893

Avoid calling the expensive room count at each backup iteration

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

ElementR | backup: call expensive roomKeyCounts less often (#4015). Fixes Element-R | count() room keys can be a slow operation on big stores, causing send performance degradation when backing up. element-hq/element-web#26893.

ara4n · 2024-01-19T10:25:35Z

src/rust-crypto/backup.ts

-                    numFailures = 0;
-                    if (this.stopped) break;
+                // Keys count performance (`olmMachine.roomKeyCounts()`) can be pretty bad on some configurations (big database in FF).
+                // We detected on some M1 mac that when the object store reach a threshold, the count performances stops growing in O(n) and


would be useful to note what that threshold roughly is, for future readers

added here ac383aa

richvdh

A few suggestions

src/rust-crypto/backup.ts

richvdh · 2024-01-19T10:42:14Z

src/rust-crypto/backup.ts

                    return;
                }

-                try {
-                    await this.outgoingRequestProcessor.makeOutgoingRequest(request);


I find it a bit confusing that the call to roomKeyCounts now happens before the call to makeOutgoingRequest. I feel like it would be simpler the other way around:

await this.outgoingRequestProcessor.makeOutgoingRequest(request); numFailures = 0; if (this.stopped) break; // ... comments about performance ... if (!isFirstIteration) { if(remainingToUploadCount === null) { const keyCount = await this.olmMachine.roomKeyCounts(); remainingToUploadCount = keyCount.total - keyCount.backedUp; } else { const keysCountInBatch = this.keysCountInBatch(request); remainingToUploadCount = Math.max(remainingToUploadCount - keysCountInBatch, 0); } this.emit(CryptoEvent.KeyBackupSessionsRemaining, remainingToUploadCount); }

WDYT?

Right, done here 3e7fa56

richvdh · 2024-01-19T10:44:29Z

src/rust-crypto/backup.ts

+        for (const entry of Object.entries(parsedBody.rooms)) {
+            count += Object.keys(entry[1].sessions).length;
+        }


Suggested change

for (const entry of Object.entries(parsedBody.rooms)) {

count += Object.keys(entry[1].sessions).length;

}

for (const entry of Object.values(parsedBody.rooms)) {

count += Object.keys(entry.sessions).length;

}

Done here 418074a

src/rust-crypto/backup.ts

ara4n · 2024-01-19T10:59:42Z

src/rust-crypto/backup.ts

+                    if (this.stopped) break;
+                    if (remainingToUploadCount !== null) {
+                        const keysCountInBatch = this.keysCountInBatch(request);
+                        remainingToUploadCount = Math.max(remainingToUploadCount - keysCountInBatch, 0);


while the backup loop runs, new megolm sessions could arrive (especially if you're backing up 400K sessions or so), so presumably memoizing the count like this could mean remainingToUploadCount gets stuck at 0 for quite a while at the end (while it backs up the additional keys that arrived). Worst case, the user might think that the progress has got stuck at zero and cancel it somehow. I wonder if it's worth spelling this edge case out at least as a comment, that the KeyBackupSessionsRemaining event may now lie and keep emitting 0 for a bit at the end of the backup loop?

Yes, worth explaining. Added comment here 8dc1ff7

Co-authored-by: Richard van der Hoff <[email protected]>

…calls

src/rust-crypto/backup.ts

richvdh · 2024-01-22T12:18:00Z