From 836b0e341f31fafa92cbd9c15eeed69467c549fd Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Sat, 16 May 2020 13:35:57 -0700 Subject: [PATCH] perf: lazy-load the thread context (#1774) * perf: lazy-load the thread context fixes #898 * more tests * test: more tests * simplify implementation --- src/routes/_actions/timeline.js | 72 +++- src/routes/_database/databaseApis.js | 2 +- src/routes/_database/timelines/insertion.js | 8 + src/routes/_utils/maps.js | 22 ++ .../_utils/sortItemSummariesForThread.js | 73 ++++ src/routes/_utils/timelineItemToSummary.js | 1 + tests/spec/122-replies-in-thread.js | 6 +- tests/unit/test-thread-ordering.js | 338 ++++++++++++++++++ 8 files changed, 511 insertions(+), 11 deletions(-) create mode 100644 src/routes/_utils/maps.js create mode 100644 src/routes/_utils/sortItemSummariesForThread.js create mode 100644 tests/unit/test-thread-ordering.js diff --git a/src/routes/_actions/timeline.js b/src/routes/_actions/timeline.js index 4722e234..6558f22b 100644 --- a/src/routes/_actions/timeline.js +++ b/src/routes/_actions/timeline.js @@ -11,6 +11,9 @@ import { emit } from '../_utils/eventBus' import { TIMELINE_BATCH_SIZE } from '../_static/timelines' import { timelineItemToSummary } from '../_utils/timelineItemToSummary' import uniqBy from 'lodash-es/uniqBy' +import { addStatusesOrNotifications } from './addStatusOrNotification' +import { scheduleIdleTask } from '../_utils/scheduleIdleTask' +import { sortItemSummariesForThread } from '../_utils/sortItemSummariesForThread' const byId = _ => _.id @@ -26,13 +29,66 @@ async function storeFreshTimelineItemsInDatabase (instanceName, timelineName, it } } +async function updateStatus (instanceName, accessToken, statusId) { + const status = await getStatus(instanceName, accessToken, statusId) + await database.insertStatus(instanceName, status) + emit('statusUpdated', status) + return status +} + +async function updateStatusAndThread (instanceName, accessToken, timelineName, statusId) { + const [status, context] = await Promise.all([ + updateStatus(instanceName, accessToken, statusId), + getStatusContext(instanceName, accessToken, statusId) + ]) + await database.insertTimelineItems( + instanceName, + timelineName, + concat(context.ancestors, status, context.descendants) + ) + addStatusesOrNotifications(instanceName, timelineName, concat(context.ancestors, context.descendants)) +} + +async function fetchFreshThreadFromNetwork (instanceName, accessToken, statusId) { + const [status, context] = await Promise.all([ + getStatus(instanceName, accessToken, statusId), + getStatusContext(instanceName, accessToken, statusId) + ]) + return concat(context.ancestors, status, context.descendants) +} + +async function fetchThreadFromNetwork (instanceName, accessToken, timelineName) { + const statusId = timelineName.split('/').slice(-1)[0] + + // For threads, we do several optimizations to make it a bit faster to load. + // The vast majority of statuses have no replies and aren't in reply to anything, + // so we want that to be as fast as possible. + const status = await database.getStatus(instanceName, statusId) + if (!status) { + // If for whatever reason the status is not cached, fetch everything from the network + // and wait for the result. This happens in very unlikely cases (e.g. loading /statuses/ + // where is not cached locally) but is worth covering. + return fetchFreshThreadFromNetwork(instanceName, accessToken, statusId) + } + + if (!status.in_reply_to_id) { + // status is not a reply to another status (fast path) + // Update the status and thread asynchronously, but return just the status for now + // Any replies to the status will load asynchronously + /* no await */ updateStatusAndThread(instanceName, accessToken, timelineName, statusId) + return [status] + } + // status is a reply to some other status, meaning we don't want some + // jerky behavior where it suddenly scrolls into place. Update the status asynchronously + // but grab the thread now + scheduleIdleTask(() => updateStatus(instanceName, accessToken, statusId)) + const context = await getStatusContext(instanceName, accessToken, statusId) + return concat(context.ancestors, status, context.descendants) +} + async function fetchTimelineItemsFromNetwork (instanceName, accessToken, timelineName, lastTimelineItemId) { if (timelineName.startsWith('status/')) { // special case - this is a list of descendents and ancestors - const statusId = timelineName.split('/').slice(-1)[0] - const statusRequest = getStatus(instanceName, accessToken, statusId) - const contextRequest = getStatusContext(instanceName, accessToken, statusId) - const [status, context] = await Promise.all([statusRequest, contextRequest]) - return concat(context.ancestors, status, context.descendants) + return fetchThreadFromNetwork(instanceName, accessToken, timelineName) } else { // normal timeline return getTimeline(instanceName, accessToken, timelineName, lastTimelineItemId, null, TIMELINE_BATCH_SIZE) } @@ -49,7 +105,7 @@ async function fetchTimelineItems (instanceName, accessToken, timelineName, last try { console.log('fetchTimelineItemsFromNetwork') items = await fetchTimelineItemsFromNetwork(instanceName, accessToken, timelineName, lastTimelineItemId) - /* no await */ storeFreshTimelineItemsInDatabase(instanceName, timelineName, items) + await storeFreshTimelineItemsInDatabase(instanceName, timelineName, items) } catch (e) { console.error(e) toast.say('Internet request failed. Showing offline content.') @@ -160,9 +216,11 @@ export async function showMoreItemsForThread (instanceName, timelineName) { timelineItemSummaries.push(itemSummaryToAdd) } } + const statusId = timelineName.split('/').slice(-1)[0] + const sortedTimelineItemSummaries = await sortItemSummariesForThread(timelineItemSummaries, statusId) store.setForTimeline(instanceName, timelineName, { timelineItemSummariesToAdd: [], - timelineItemSummaries: timelineItemSummaries + timelineItemSummaries: sortedTimelineItemSummaries }) stop('showMoreItemsForThread') } diff --git a/src/routes/_database/databaseApis.js b/src/routes/_database/databaseApis.js index e3ab4b82..6f9bf298 100644 --- a/src/routes/_database/databaseApis.js +++ b/src/routes/_database/databaseApis.js @@ -6,6 +6,6 @@ export * from './timelines/pagination' export * from './timelines/getStatusOrNotification' export * from './timelines/updateStatus' export * from './timelines/deletion' -export * from './timelines/insertion' +export { insertTimelineItems, insertStatus } from './timelines/insertion' export * from './meta' export * from './relationships' diff --git a/src/routes/_database/timelines/insertion.js b/src/routes/_database/timelines/insertion.js index 0dae319b..94f4e41b 100644 --- a/src/routes/_database/timelines/insertion.js +++ b/src/routes/_database/timelines/insertion.js @@ -121,3 +121,11 @@ export async function insertTimelineItems (instanceName, timeline, timelineItems return insertTimelineStatuses(instanceName, timeline, timelineItems) } } + +export async function insertStatus (instanceName, status) { + cacheStatus(status, instanceName) + const db = await getDatabase(instanceName) + await dbPromise(db, [STATUSES_STORE, ACCOUNTS_STORE], 'readwrite', ([statusesStore, accountsStore]) => { + storeStatus(statusesStore, accountsStore, status) + }) +} diff --git a/src/routes/_utils/maps.js b/src/routes/_utils/maps.js new file mode 100644 index 00000000..d7b0f3d1 --- /dev/null +++ b/src/routes/_utils/maps.js @@ -0,0 +1,22 @@ +// utilities for working with Maps + +export function mapBy (items, func) { + const map = new Map() + for (const item of items) { + map.set(func(item), item) + } + return map +} + +export function multimapBy (items, func) { + const map = new Map() + for (const item of items) { + const key = func(item) + if (map.has(key)) { + map.get(key).push(item) + } else { + map.set(key, [item]) + } + } + return map +} diff --git a/src/routes/_utils/sortItemSummariesForThread.js b/src/routes/_utils/sortItemSummariesForThread.js new file mode 100644 index 00000000..90af5ba2 --- /dev/null +++ b/src/routes/_utils/sortItemSummariesForThread.js @@ -0,0 +1,73 @@ +// This is designed to exactly mimic Mastodon's ordering for threads. As described by Gargron: +// "statuses are ordered in the postgresql query and then any of OP's self-replies bubble to the top" +// Source: https://github.com/tootsuite/mastodon/blob/ef15246/app/models/concerns/status_threading_concern.rb +import { concat } from './arrays' +import { compareTimelineItemSummaries } from './statusIdSorting' +import { mapBy, multimapBy } from './maps' + +export function sortItemSummariesForThread (summaries, statusId) { + const ancestors = [] + const descendants = [] + const summariesById = mapBy(summaries, _ => _.id) + const summariesByReplyId = multimapBy(summaries, _ => _.replyId) + + const status = summariesById.get(statusId) + if (!status) { + // bail out, for some reason we can't find the status (should never happen) + return summaries + } + + // find ancestors + let currentStatus = status + do { + currentStatus = summariesById.get(currentStatus.replyId) + if (currentStatus) { + ancestors.unshift(currentStatus) + } + } while (currentStatus) + + // find descendants + // This mirrors the depth-first ordering used in the Postgres query in the Mastodon implementation + const stack = [status] + while (stack.length) { + const current = stack.shift() + const newChildren = (summariesByReplyId.get(current.id) || []).sort(compareTimelineItemSummaries) + Array.prototype.unshift.apply(stack, newChildren) + if (current.id !== status.id) { // the status is not a descendant of itself + descendants.push(current) + } + } + + // Normally descendants are sorted in depth-first order, via normal ID sorting + // but replies that come from the account they're actually replying to get promoted + // This only counts if it's an unbroken self-reply, e.g. in the case of + // A -> A -> A -> B -> A -> A + // B has broken the chain, so only the first three As are considered unbroken self-replies + const isUnbrokenSelfReply = (descendant) => { + let current = descendant + while (true) { + if (current.accountId !== status.accountId) { + return false + } + const parent = summariesById.get(current.replyId) + if (!parent) { + break + } + current = parent + } + return current.id === statusId + } + + const promotedDescendants = [] + const otherDescendants = [] + for (const descendant of descendants) { + (isUnbrokenSelfReply(descendant) ? promotedDescendants : otherDescendants).push(descendant) + } + + return concat( + ancestors, + [status], + promotedDescendants, + otherDescendants + ) +} diff --git a/src/routes/_utils/timelineItemToSummary.js b/src/routes/_utils/timelineItemToSummary.js index 86920444..16fea5c9 100644 --- a/src/routes/_utils/timelineItemToSummary.js +++ b/src/routes/_utils/timelineItemToSummary.js @@ -1,6 +1,7 @@ export function timelineItemToSummary (item) { return { id: item.id, + accountId: item.account.id, replyId: (item.in_reply_to_id) || undefined, reblogId: (item.reblog && item.reblog.id) || undefined, type: item.type || undefined diff --git a/tests/spec/122-replies-in-thread.js b/tests/spec/122-replies-in-thread.js index 2fd1dd02..74b42b2f 100644 --- a/tests/spec/122-replies-in-thread.js +++ b/tests/spec/122-replies-in-thread.js @@ -158,13 +158,13 @@ test('no duplicates in threads', async t => { const id5 = await getNthStatusId(6)() await postReplyAs('admin', 'hey i am replying to 1 again', id1) await t + .expect(getNthStatusContent(6).innerText).contains('this is my thread 5') .click(getNthStatus(6)) .expect(getUrl()).contains(id5) .click(getNthStatus(1)) .expect(getUrl()).contains(id1) - - await t - .click(getNthStatus(6)) + .expect(getNthStatusContent(5).innerText).contains('this is my thread 5') + .click(getNthStatus(5)) .expect(getUrl()).contains(id5) await replyToNthStatus(t, 5, 'this is my thread 6', 6) await t diff --git a/tests/unit/test-thread-ordering.js b/tests/unit/test-thread-ordering.js new file mode 100644 index 00000000..b214a56a --- /dev/null +++ b/tests/unit/test-thread-ordering.js @@ -0,0 +1,338 @@ +/* global describe, it */ +import assert from 'assert' +import { sortItemSummariesForThread } from '../../src/routes/_utils/sortItemSummariesForThread' + +describe('test-thread-ordering.js', () => { + it('orders a complex thread correctly', () => { + const summaries = [ + { + id: '104170273205084988', + accountId: '2', + content: 'a' + }, + { + id: '104170273237400789', + accountId: '5', + replyId: '104170273205084988', + content: 'b' + }, + { + id: '104170273276841718', + accountId: '5', + replyId: '104170273205084988', + content: 'a1' + }, + { + id: '104170273256426023', + accountId: '5', + replyId: '104170273237400789', + content: 'c' + }, + { + id: '104170273298534371', + accountId: '5', + replyId: '104170273256426023', + content: 'd' + }, + { + id: '104170273356931049', + accountId: '5', + replyId: '104170273298534371', + content: 'e' + }, + { + id: '104170273341388633', + accountId: '5', + replyId: '104170273237400789', + content: 'b1' + }, + { + id: '104170273365802755', + accountId: '5', + replyId: '104170273341388633', + content: 'b2' + }, + { + id: '104170273331400302', + accountId: '5', + replyId: '104170273276841718', + content: 'a2' + }, + { + id: '104170273348336156', + accountId: '5', + replyId: '104170273331400302', + content: 'a3' + }, + { + id: '104170273376273045', + accountId: '5', + replyId: '104170273348336156', + content: 'a4' + }, + { + id: '104170273388248109', + accountId: '5', + replyId: '104170273276841718', + content: 'a1a' + } + ] + + const expected = 'a b c d e b1 b2 a1 a2 a3 a4 a1a'.split(' ') + + const sorted = sortItemSummariesForThread(summaries, summaries[0].id) + const sortedContents = sorted.map(_ => _.content) + assert.deepStrictEqual(sortedContents, expected) + }) + + it('orders a complex thread correctly - original account involved', () => { + const summaries = [ + { + id: '104170273205084988', + accountId: '2', + content: 'a' + }, + { + id: '104170273237400789', + accountId: '5', + replyId: '104170273205084988', + content: 'b' + }, + { + id: '104170273276841718', + accountId: '2', + replyId: '104170273205084988', + content: 'a1' + }, + { + id: '104170273256426023', + accountId: '5', + replyId: '104170273237400789', + content: 'c' + }, + { + id: '104170273298534371', + accountId: '5', + replyId: '104170273256426023', + content: 'd' + }, + { + id: '104170273356931049', + accountId: '5', + replyId: '104170273298534371', + content: 'e' + }, + { + id: '104170273341388633', + accountId: '5', + replyId: '104170273237400789', + content: 'b1' + }, + { + id: '104170273365802755', + accountId: '5', + replyId: '104170273341388633', + content: 'b2' + }, + { + id: '104170273331400302', + accountId: '2', + replyId: '104170273276841718', + content: 'a2' + }, + { + id: '104170273348336156', + accountId: '2', + replyId: '104170273331400302', + content: 'a3' + }, + { + id: '104170273376273045', + accountId: '2', + replyId: '104170273348336156', + content: 'a4' + }, + { + id: '104170273388248109', + accountId: '5', + replyId: '104170273276841718', + content: 'a1a' + } + ] + + const expected = 'a a1 a2 a3 a4 b c d e b1 b2 a1a'.split(' ') + + const sorted = sortItemSummariesForThread(summaries, summaries[0].id) + const sortedContents = sorted.map(_ => _.content) + assert.deepStrictEqual(sortedContents, expected) + }) + + it('complex thread is in correct order - with mixed self-replies 2', () => { + const summaries = [{ + id: '104176454386581622', + accountId: '2', + content: 'a' + }, { + id: '104176454485378729', + accountId: '2', + replyId: '104176454386581622', + content: 'foobar-mixed1' + }, { + id: '104176454515584245', + accountId: '2', + replyId: '104176454386581622', + content: 'foobar-mixed1a' + }, { + id: '104176454522882883', + accountId: '2', + replyId: '104176454386581622', + content: 'foobar-mixed1b' + }, { + id: '104176454396619534', + accountId: '5', + replyId: '104176454386581622', + content: 'b' + }, { + id: '104176454413613662', + accountId: '5', + replyId: '104176454386581622', + content: 'a1' + }, { + id: '104176454529610049', + accountId: '2', + replyId: '104176454485378729', + content: 'foobar-mixed2a' + }, { + id: '104176454403991688', + accountId: '5', + replyId: '104176454396619534', + content: 'c' + }, { + id: '104176454422082616', + accountId: '5', + replyId: '104176454403991688', + content: 'd' + }, { + id: '104176454453810927', + accountId: '5', + replyId: '104176454422082616', + content: 'e' + }, { + id: '104176454437136977', + accountId: '5', + replyId: '104176454396619534', + content: 'b1' + }, { + id: '104176454461082666', + accountId: '5', + replyId: '104176454437136977', + content: 'b2' + }, { + id: '104176454429382434', + accountId: '5', + replyId: '104176454413613662', + content: 'a2' + }, { + id: '104176454446265415', + accountId: '5', + replyId: '104176454429382434', + content: 'a3' + }, { + id: '104176454468322929', + accountId: '5', + replyId: '104176454446265415', + content: 'a4' + }, { + id: '104176454477242935', + accountId: '5', + replyId: '104176454413613662', + content: 'a1a' + }, { + id: '104176454493347083', + accountId: '5', + replyId: '104176454485378729', + content: 'baz-mixed2' + }, { + id: '104176454500705115', + accountId: '2', + replyId: '104176454493347083', + content: 'foobar-mixed3' + }, { + id: '104176454508488937', + accountId: '2', + replyId: '104176454500705115', + content: 'foobar-mixed4' + }] + + const expected = ('a foobar-mixed1 foobar-mixed2a foobar-mixed1a foobar-mixed1b ' + + 'b c d e b1 b2 a1 a2 a3 a4 a1a baz-mixed2 foobar-mixed3 foobar-mixed4').split(' ') + + const sorted = sortItemSummariesForThread(summaries, summaries[0].id) + const sortedContents = sorted.map(_ => _.content) + assert.deepStrictEqual(sortedContents, expected) + }) + + it('orders another complex thread correctly', () => { + const summaries = [{ + id: '104179325085424124', + accountId: '2', + content: 'this-is-my-thread-1' + }, { + id: '104179325166234979', + accountId: '2', + replyId: '104179325085424124', + content: 'this-is-my-thread-2' + }, { + id: '104179325240180153', + accountId: '2', + replyId: '104179325166234979', + content: 'this-is-my-thread-3' + }, { + id: '104179325498778701', + accountId: '2', + replyId: '104179325240180153', + content: 'this-is-my-thread-4' + }, { + id: '104179325543709477', + accountId: '2', + replyId: '104179325498778701', + content: 'this-is-my-thread-5' + }, { + id: '104179325275861201', + accountId: '1', + replyId: '104179325240180153', + content: 'hey-i-am-replying-to-3' + }, { + id: '104179325263377436', + accountId: '3', + replyId: '104179325085424124', + content: 'hey-i-am-replying-to-1' + }, { + id: '104179325387035947', + accountId: '3', + replyId: '104179325085424124', + content: 'hey-check-this-reply' + }, { + id: '104179325564606101', + accountId: '1', + replyId: '104179325085424124', + content: 'hey-i-am-replying-to-1-again' + }] + + const expected = [ + 'this-is-my-thread-1', + 'this-is-my-thread-2', + 'this-is-my-thread-3', + 'this-is-my-thread-4', + 'this-is-my-thread-5', + 'hey-i-am-replying-to-3', + 'hey-i-am-replying-to-1', + 'hey-check-this-reply', + 'hey-i-am-replying-to-1-again' + ] + + const sorted = sortItemSummariesForThread(summaries, summaries[0].id) + const sortedContents = sorted.map(_ => _.content) + assert.deepStrictEqual(sortedContents, expected) + }) +})