From 3bc187fa5e7fb839b1a2caa3a965a5933101bdcc Mon Sep 17 00:00:00 2001 From: William Grant Date: Thu, 11 May 2023 17:29:01 +1000 Subject: [PATCH] feat: performance improvements to quote lookup getMessagesByConversation optionally returns quotes from messages in view, quoted messages that are deleted are removed from the lookup map. getMessageBySenderAndSentAt supports an array of messages and renamed to getMessagesBySenderAndSentAt --- ts/data/data.ts | 45 ++++++++++-------- ts/data/dataInit.ts | 2 +- ts/models/conversation.ts | 2 +- ts/node/sql.ts | 83 ++++++++++++++++++++++----------- ts/receiver/contentMessage.ts | 14 ++++-- ts/receiver/dataMessage.ts | 12 +++-- ts/state/ducks/conversations.ts | 75 +++++++++++++++++------------ 7 files changed, 146 insertions(+), 87 deletions(-) diff --git a/ts/data/data.ts b/ts/data/data.ts index 900920b6a..f6051429c 100644 --- a/ts/data/data.ts +++ b/ts/data/data.ts @@ -18,6 +18,7 @@ import { channels } from './channels'; import * as dataInit from './dataInit'; import { StorageItem } from '../node/storage_item'; import { fromArrayBufferToBase64, fromBase64ToArrayBuffer } from '../session/utils/String'; +import { Quote } from '../receiver/types'; const ERASE_SQL_KEY = 'erase-sql-key'; const ERASE_ATTACHMENTS_KEY = 'erase-attachments'; @@ -144,7 +145,7 @@ export const Data = { getMessageById, getMessageByServerId, filterAlreadyFetchedOpengroupMessage, - getMessageBySenderAndSentAt, + getMessagesBySenderAndSentAt, getUnreadByConversation, getUnreadCountByConversation, markAllAsReadByConversationNoExpiration, @@ -451,26 +452,22 @@ async function filterAlreadyFetchedOpengroupMessage( /** * - * @param source senders id - * @param timestamp the timestamp of the message - not to be confused with the serverTimestamp. This is equivalent to sent_at + * @param propsList An array of objects containing a source (the sender id) and timestamp of the message - not to be confused with the serverTimestamp. This is equivalent to sent_at + * @returns */ -async function getMessageBySenderAndSentAt({ - source, - timestamp, -}: { - source: string; - timestamp: number; -}): Promise { - const messages = await channels.getMessageBySenderAndSentAt({ - source, - timestamp, - }); +async function getMessagesBySenderAndSentAt( + propsList: Array<{ + source: string; + timestamp: number; + }> +): Promise { + const messages = await channels.getMessagesBySenderAndSentAt(propsList); if (!messages || !messages.length) { return null; } - return new MessageModel(messages[0]); + return new MessageCollection(messages); } async function getUnreadByConversation(conversationId: string): Promise { @@ -512,17 +509,27 @@ async function getMessageCountByType( async function getMessagesByConversation( conversationId: string, - { skipTimerInit = false, messageId = null }: { skipTimerInit?: false; messageId: string | null } -): Promise { - const messages = await channels.getMessagesByConversation(conversationId, { + { + skipTimerInit = false, + returnQuotes = false, + messageId = null, + }: { skipTimerInit?: false; returnQuotes?: boolean; messageId: string | null } +): Promise<{ messages: MessageCollection; quotes: Array }> { + const { messages, quotes } = await channels.getMessagesByConversation(conversationId, { messageId, + returnQuotes, }); + if (skipTimerInit) { for (const message of messages) { message.skipTimerInit = skipTimerInit; } } - return new MessageCollection(messages); + + return { + messages: new MessageCollection(messages), + quotes, + }; } /** diff --git a/ts/data/dataInit.ts b/ts/data/dataInit.ts index 09c402f30..7b546c13d 100644 --- a/ts/data/dataInit.ts +++ b/ts/data/dataInit.ts @@ -46,7 +46,7 @@ const channelsToMake = new Set([ 'removeAllMessagesInConversation', 'getMessageCount', 'filterAlreadyFetchedOpengroupMessage', - 'getMessageBySenderAndSentAt', + 'getMessagesBySenderAndSentAt', 'getMessageIdsFromServerIds', 'getMessageById', 'getMessagesBySentAt', diff --git a/ts/models/conversation.ts b/ts/models/conversation.ts index 845c5daa6..dc40fe278 100644 --- a/ts/models/conversation.ts +++ b/ts/models/conversation.ts @@ -1773,7 +1773,7 @@ export class ConversationModel extends Backbone.Model { ); }).length === 1; const isFirstMessageOfConvo = - (await Data.getMessagesByConversation(this.id, { messageId: null })).length === 1; + (await Data.getMessagesByConversation(this.id, { messageId: null })).messages.length === 1; if (hadNoRequestsPrior && isFirstMessageOfConvo) { friendRequestText = window.i18n('youHaveANewFriendRequest'); } else { diff --git a/ts/node/sql.ts b/ts/node/sql.ts index 41a2a8c2d..f0b6d1714 100644 --- a/ts/node/sql.ts +++ b/ts/node/sql.ts @@ -17,6 +17,7 @@ import { isString, last, map, + uniq, } from 'lodash'; import { redactAll } from '../util/privacy'; // checked - only node import { LocaleMessagesType } from './locale'; // checked - only node @@ -55,6 +56,7 @@ import { updateSchema, } from './migration/signalMigrations'; import { SettingsKey } from '../data/settings-key'; +import { Quote } from '../receiver/types'; // tslint:disable: no-console function-name non-literal-fs-path @@ -1062,19 +1064,32 @@ function getMessagesCountBySender({ source }: { source: string }) { return count['count(*)'] || 0; } -function getMessageBySenderAndSentAt({ source, timestamp }: { source: string; timestamp: number }) { - const rows = assertGlobalInstance() - .prepare( - `SELECT json FROM ${MESSAGES_TABLE} WHERE +function getMessagesBySenderAndSentAt( + propsList: Array<{ + source: string; + timestamp: number; + }> +) { + const db = assertGlobalInstance(); + const rows = []; + + for (let i = 0; i < propsList.length; i++) { + const { source, timestamp } = propsList[i]; + + const _rows = db + .prepare( + `SELECT json FROM ${MESSAGES_TABLE} WHERE source = $source AND sent_at = $timestamp;` - ) - .all({ - source, - timestamp, - }); + ) + .all({ + source, + timestamp, + }); + rows.push(..._rows); + } - return map(rows, row => jsonToObject(row.json)); + return uniq(map(rows, row => jsonToObject(row.json))); } function filterAlreadyFetchedOpengroupMessage( @@ -1203,7 +1218,10 @@ function getMessageCountByType(conversationId: string, type = '%') { const orderByClause = 'ORDER BY COALESCE(serverTimestamp, sent_at, received_at) DESC'; const orderByClauseASC = 'ORDER BY COALESCE(serverTimestamp, sent_at, received_at) ASC'; -function getMessagesByConversation(conversationId: string, { messageId = null } = {}) { +function getMessagesByConversation( + conversationId: string, + { messageId = null, returnQuotes = false } = {} +): { messages: Array>; quotes: Array } { const absLimit = 30; // If messageId is given it means we are opening the conversation to that specific messageId, // or that we just scrolled to it by a quote click and needs to load around it. @@ -1213,6 +1231,9 @@ function getMessagesByConversation(conversationId: string, { messageId = null } const numberOfMessagesInConvo = getMessagesCountByConversation(conversationId, globalInstance); const floorLoadAllMessagesInConvo = 70; + let messages: Array> = []; + let quotes = []; + if (messageId || firstUnread) { const messageFound = getMessageById(messageId || firstUnread); @@ -1244,33 +1265,39 @@ function getMessagesByConversation(conversationId: string, { messageId = null } : absLimit, }); - return map(rows, row => jsonToObject(row.json)); + messages = map(rows, row => jsonToObject(row.json)); } console.info( `getMessagesByConversation: Could not find messageId ${messageId} in db with conversationId: ${conversationId}. Just fetching the convo as usual.` ); - } + } else { + const limit = + numberOfMessagesInConvo < floorLoadAllMessagesInConvo + ? floorLoadAllMessagesInConvo + : absLimit * 2; - const limit = - numberOfMessagesInConvo < floorLoadAllMessagesInConvo - ? floorLoadAllMessagesInConvo - : absLimit * 2; - - const rows = assertGlobalInstance() - .prepare( - ` + const rows = assertGlobalInstance() + .prepare( + ` SELECT json FROM ${MESSAGES_TABLE} WHERE conversationId = $conversationId ${orderByClause} LIMIT $limit; ` - ) - .all({ - conversationId, - limit, - }); + ) + .all({ + conversationId, + limit, + }); - return map(rows, row => jsonToObject(row.json)); + messages = map(rows, row => jsonToObject(row.json)); + } + + if (returnQuotes) { + quotes = messages.filter(message => message.quote).map(message => message.quote); + } + + return { messages, quotes }; } function getLastMessagesByConversation(conversationId: string, limit: number) { @@ -2454,7 +2481,7 @@ export const sqlNode = { getMessageCountByType, filterAlreadyFetchedOpengroupMessage, - getMessageBySenderAndSentAt, + getMessagesBySenderAndSentAt, getMessageIdsFromServerIds, getMessageById, getMessagesBySentAt, diff --git a/ts/receiver/contentMessage.ts b/ts/receiver/contentMessage.ts index 2235933e8..3cf52cda0 100644 --- a/ts/receiver/contentMessage.ts +++ b/ts/receiver/contentMessage.ts @@ -565,10 +565,14 @@ async function handleUnsendMessage(envelope: EnvelopePlus, unsendMessage: Signal return; } - const messageToDelete = await Data.getMessageBySenderAndSentAt({ - source: messageAuthor, - timestamp: toNumber(timestamp), - }); + const messageToDelete = ( + await Data.getMessagesBySenderAndSentAt([ + { + source: messageAuthor, + timestamp: toNumber(timestamp), + }, + ]) + )?.models?.[0]; const messageHash = messageToDelete?.get('messageHash'); //#endregion @@ -665,7 +669,7 @@ async function handleMessageRequestResponse( ) ); - const allMessageModels = flatten(allMessagesCollections.map(m => m.models)); + const allMessageModels = flatten(allMessagesCollections.map(m => m.messages.models)); allMessageModels.forEach(messageModel => { messageModel.set({ conversationId: unblindedConvoId }); diff --git a/ts/receiver/dataMessage.ts b/ts/receiver/dataMessage.ts index 853889a26..ba6e6f53d 100644 --- a/ts/receiver/dataMessage.ts +++ b/ts/receiver/dataMessage.ts @@ -258,10 +258,14 @@ export async function isSwarmMessageDuplicate({ sentAt: number; }) { try { - const result = await Data.getMessageBySenderAndSentAt({ - source, - timestamp: sentAt, - }); + const result = ( + await Data.getMessagesBySenderAndSentAt([ + { + source, + timestamp: sentAt, + }, + ]) + )?.models?.length; return Boolean(result); } catch (error) { diff --git a/ts/state/ducks/conversations.ts b/ts/state/ducks/conversations.ts index 431c62f34..594b35b63 100644 --- a/ts/state/ducks/conversations.ts +++ b/ts/state/ducks/conversations.ts @@ -289,6 +289,7 @@ export type ConversationLookupType = { }; export type QuoteLookupType = { + // key is message [timestamp]-[author-pubkey] [key: string]: MessageModelPropsWithoutConvoProps; }; @@ -298,7 +299,6 @@ export type ConversationsStateType = { // NOTE the messages that are in view messages: Array; // NOTE the quotes that are in view - // key is message [timestamp]-[author-pubkey] quotes: QuoteLookupType; firstUnreadMessageId: string | undefined; messageDetailProps?: MessagePropsDetails; @@ -364,48 +364,45 @@ async function getMessages({ return { messagesProps: [], quotesProps: {} }; } - const messageSet = await Data.getMessagesByConversation(conversationKey, { + const { + messages: messagesCollection, + quotes: quotesCollection, + } = await Data.getMessagesByConversation(conversationKey, { messageId, + returnQuotes: true, }); - const messagesProps: Array = messageSet.models.map(m => - m.getMessageModelProps() + const messagesProps: Array = messagesCollection.models.map( + m => m.getMessageModelProps() ); const time = Date.now() - beforeTimestamp; window?.log?.info(`Loading ${messagesProps.length} messages took ${time}ms to load.`); const quotesProps: QuoteLookupType = {}; - const quotes = messagesProps.filter( - message => message.propsForMessage?.quote?.messageId && message.propsForMessage.quote?.sender - ); - for (let i = 0; i < quotes.length; i++) { - const id = quotes[i].propsForMessage?.quote?.messageId; - const sender = quotes[i].propsForMessage.quote?.sender; - - if (id && sender) { - const timestamp = Number(id); - // See if a quoted message is already in memory if not lookup in db - let results = messagesProps.filter( - message => - message.propsForMessage.timestamp === timestamp && - message.propsForMessage.sender === sender - ); - - if (!results.length) { - const dbResult = ( - await Data.getMessageBySenderAndSentAt({ source: sender, timestamp }) - )?.getMessageModelProps(); - if (dbResult) { - results = [dbResult]; + if (quotesCollection?.length) { + const quotePropsList = quotesCollection.map(quote => ({ + timestamp: Number(quote?.id), + source: String(quote?.author), + })); + + const quotedMessagesCollection = await Data.getMessagesBySenderAndSentAt(quotePropsList); + + if (quotedMessagesCollection?.length) { + for (let i = 0; i < quotedMessagesCollection.length; i++) { + const quotedMessage = quotedMessagesCollection.models.at(i)?.getMessageModelProps(); + if (quotedMessage) { + const timestamp = Number(quotedMessage.propsForMessage.timestamp); + const sender = quotedMessage.propsForMessage.sender; + if (timestamp && sender) { + quotesProps[`${timestamp}-${sender}`] = quotedMessage; + } } } - quotesProps[`${timestamp}-${sender}`] = results[0]; } } - // window.log.debug(`WIP: duck quoteProps`, quotesProps); - + // window.log.debug(`WIP: duck quotesProps`, quotesProps); return { messagesProps, quotesProps }; } @@ -563,7 +560,26 @@ function handleMessageExpiredOrDeleted( // search if we find this message id. // we might have not loaded yet, so this case might not happen const messageInStoreIndex = state?.messages.findIndex(m => m.propsForMessage.id === messageId); + const editedQuotes = { ...state.quotes }; if (messageInStoreIndex >= 0) { + // Check if the message is quoted somewhere, and if so, remove it from the quotes + const msgProps = state.messages[messageInStoreIndex].propsForMessage; + // TODO check if message is a group or public group because we will need to use the server timestamp + const { timestamp, sender } = msgProps; + if (timestamp && sender) { + const message2Delete = editedQuotes[`${timestamp}-${sender}`]; + window.log.debug( + `WIP: deleting quote {${timestamp}-${sender}} ${JSON.stringify(message2Delete)}` + ); + window.log.debug( + `WIP: editedQuotes count before delete ${Object.keys(editedQuotes).length}` + ); + delete editedQuotes[`${timestamp}-${sender}`]; + window.log.debug( + `WIP: editedQuotes count after delete ${Object.keys(editedQuotes).length}` + ); + } + // we cannot edit the array directly, so slice the first part, and slice the second part, // keeping the index removed out const editedMessages = [ @@ -578,6 +594,7 @@ function handleMessageExpiredOrDeleted( return { ...state, messages: editedMessages, + quotes: editedQuotes, firstUnreadMessageId: state.firstUnreadMessageId === messageId ? undefined : state.firstUnreadMessageId, };