-
Notifications
You must be signed in to change notification settings - Fork 383
perf(persistence): Reduce the number of read message per channel from DB when paginating (part 1) #2679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(persistence): Reduce the number of read message per channel from DB when paginating (part 1) #2679
Changes from all commits
88987e9
fe4d4e2
5652130
466d9d3
e1ec859
d38c049
4689606
edb86f7
b580ccd
52f0c26
d7a18d6
2e719f4
35569a0
fa6898e
1a727ec
0e709eb
e49ffb7
8d408e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,3 @@ | ||
| import 'dart:math'; | ||
|
|
||
| import 'package:drift/drift.dart'; | ||
| import 'package:stream_chat/stream_chat.dart'; | ||
| import 'package:stream_chat_persistence/src/db/drift_chat_database.dart'; | ||
|
|
@@ -171,6 +169,38 @@ class MessageDao extends DatabaseAccessor<DriftChatDatabase> | |
| bool fetchDraft = true, | ||
| PaginationParams? messagePagination, | ||
| }) async { | ||
| final ( | ||
| lessThanCursor, | ||
| lessThanOrEqualCursor, | ||
| greaterThanCursor, | ||
| greaterThanOrEqualCursor, | ||
| ) = await ( | ||
| _lookupCursor(messagePagination?.lessThan), | ||
| _lookupCursor(messagePagination?.lessThanOrEqual), | ||
| _lookupCursor(messagePagination?.greaterThan), | ||
| _lookupCursor(messagePagination?.greaterThanOrEqual), | ||
| ).wait; | ||
|
|
||
| // When the caller is paginating forward (greaterThan / greaterThanOrEqual | ||
| // only), order ASC so the SQL `LIMIT` retains the N messages immediately | ||
| // AFTER the cursor. Otherwise order DESC so `LIMIT` retains the N most | ||
| // recent (closest to a `lessThan` cursor, or the channel tail when no | ||
| // cursor is set). The final result is always reshaped to ASC for display. | ||
| final isForwardPagination = | ||
| (greaterThanCursor != null || greaterThanOrEqualCursor != null) && | ||
| lessThanCursor == null && | ||
| lessThanOrEqualCursor == null; | ||
|
|
||
| final orderBy = isForwardPagination | ||
| ? [ | ||
| OrderingTerm.asc(messages.createdAt), | ||
| OrderingTerm.asc(messages.id), | ||
| ] | ||
| : [ | ||
| OrderingTerm.desc(messages.createdAt), | ||
| OrderingTerm.desc(messages.id), | ||
| ]; | ||
|
|
||
| final query = select(messages).join([ | ||
| leftOuterJoin(_users, messages.userId.equalsExp(_users.id)), | ||
| leftOuterJoin( | ||
|
|
@@ -180,44 +210,52 @@ class MessageDao extends DatabaseAccessor<DriftChatDatabase> | |
| ]) | ||
| ..where(messages.channelCid.equals(cid)) | ||
| ..where(messages.parentId.isNull() | messages.showInChannel.equals(true)) | ||
| ..orderBy([OrderingTerm.asc(messages.createdAt)]); | ||
| ..orderBy(orderBy); | ||
|
|
||
| final result = await query.get(); | ||
| if (result.isEmpty) return []; | ||
|
|
||
| final msgList = await Future.wait( | ||
| result.map( | ||
| (row) => _messageFromJoinRow( | ||
| row, | ||
| fetchDraft: fetchDraft, | ||
| ), | ||
| ), | ||
| ); | ||
| // Cursor predicates compare the full `(createdAt, id)` tuple — the same | ||
| // key used in ORDER BY — so messages sharing a `createdAt` with the cursor | ||
| // fall on the correct side of the boundary. Filtering on `createdAt` alone | ||
| // would skip or repeat those siblings across pages. | ||
| if (lessThanCursor case final c?) { | ||
| query.where( | ||
| messages.createdAt.isSmallerThanValue(c.createdAt) | | ||
| (messages.createdAt.equals(c.createdAt) & | ||
| messages.id.isSmallerThanValue(c.id)), | ||
| ); | ||
| } | ||
| if (lessThanOrEqualCursor case final c?) { | ||
| query.where( | ||
| messages.createdAt.isSmallerThanValue(c.createdAt) | | ||
| (messages.createdAt.equals(c.createdAt) & | ||
| messages.id.isSmallerOrEqualValue(c.id)), | ||
| ); | ||
| } | ||
| if (greaterThanCursor case final c?) { | ||
| query.where( | ||
| messages.createdAt.isBiggerThanValue(c.createdAt) | | ||
| (messages.createdAt.equals(c.createdAt) & | ||
| messages.id.isBiggerThanValue(c.id)), | ||
| ); | ||
| } | ||
| if (greaterThanOrEqualCursor case final c?) { | ||
| query.where( | ||
| messages.createdAt.isBiggerThanValue(c.createdAt) | | ||
| (messages.createdAt.equals(c.createdAt) & | ||
| messages.id.isBiggerOrEqualValue(c.id)), | ||
| ); | ||
| } | ||
|
|
||
| if (msgList.isNotEmpty) { | ||
| if (messagePagination?.lessThan != null) { | ||
| final lessThanIndex = msgList.indexWhere( | ||
| (m) => m.id == messagePagination!.lessThan, | ||
| ); | ||
| if (lessThanIndex != -1) { | ||
| msgList.removeRange(lessThanIndex, msgList.length); | ||
| } | ||
| } | ||
| if (messagePagination?.greaterThan != null) { | ||
| final greaterThanIndex = msgList.indexWhere( | ||
| (m) => m.id == messagePagination!.greaterThan, | ||
| ); | ||
| if (greaterThanIndex != -1) { | ||
| msgList.removeRange(0, greaterThanIndex); | ||
| } | ||
| } | ||
| if (messagePagination?.limit != null) { | ||
| return msgList | ||
| .skip(max(0, msgList.length - messagePagination!.limit)) | ||
| .toList(); | ||
| } | ||
| if (messagePagination != null) { | ||
| query.limit(messagePagination.limit); | ||
| } | ||
|
Comment on lines
+248
to
250
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we only limit this when either
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This actually matches the previous behaviour of the method. The But now that you mentioned it, we actually have several smaller issues that neither the 'old' nor the 'new' implementation cover:
Should we fix these things in this PR? Or do you think it is better to handle them in a follow up?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think they can be fixed in this PR. You already change the method quite a bit, so would be good to make it correct.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I will address this!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adressed here: b580ccd |
||
| return msgList; | ||
|
|
||
| final rows = await query.get(); | ||
| final orderedRows = isForwardPagination ? rows : rows.reversed.toList(); | ||
|
|
||
| return Future.wait( | ||
| orderedRows | ||
| .map((row) => _messageFromJoinRow(row, fetchDraft: fetchDraft)), | ||
| ); | ||
| } | ||
|
|
||
| /// Updates the message data of a particular channel with | ||
|
|
@@ -241,4 +279,22 @@ class MessageDao extends DatabaseAccessor<DriftChatDatabase> | |
| (batch) => batch.insertAllOnConflictUpdate(messages, entities), | ||
| ); | ||
| } | ||
|
|
||
| /// Returns the `(createdAt, id)` cursor for the message with [id] in the | ||
| /// local cache, or `null` if [id] is null, the message isn't cached, or | ||
| /// isn't visible in the channel (i.e. a thread reply with | ||
| /// `showInChannel = false`). | ||
| Future<({DateTime createdAt, String id})?> _lookupCursor(String? id) async { | ||
| if (id == null) return null; | ||
| final createdAt = await (selectOnly(messages) | ||
| ..addColumns([messages.createdAt]) | ||
| ..where(messages.id.equals(id)) | ||
| ..where( | ||
| messages.parentId.isNull() | messages.showInChannel.equals(true), | ||
| )) | ||
| .map((row) => row.read(messages.createdAt)) | ||
| .getSingleOrNull(); | ||
| if (createdAt == null) return null; | ||
| return (createdAt: createdAt, id: id); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.