From f665901e3c0930fb8b3741f6bc6f6a15dd0343f6 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 6 Oct 2019 22:11:17 +0200 Subject: [PATCH] Fix performance of home feed regeneration (#12084) Fetching statuses from all followed accounts at once takes too long within Postgres. Fetching them one by one and merging in Ruby could be a lot less resource-intensive Because the query for dynamically fetching the home timeline is so heavy, we can no longer offer it when the home timeline is missing --- .../api/v1/timelines/home_controller.rb | 6 +- app/javascript/mastodon/actions/timelines.js | 2 +- .../mastodon/components/missing_indicator.js | 23 ++-- .../components/regeneration_indicator.js | 18 +++ .../mastodon/components/status_list.js | 15 +-- .../features/generic_not_found/index.js | 2 +- .../styles/mastodon/components.scss | 34 ++---- .../styles/mastodon/introduction.scss | 3 +- app/lib/feed_manager.rb | 106 +++++++++++++----- app/models/home_feed.rb | 16 +-- app/models/status.rb | 4 - spec/models/home_feed_spec.rb | 5 +- spec/models/status_spec.rb | 43 ------- 13 files changed, 134 insertions(+), 143 deletions(-) create mode 100644 app/javascript/mastodon/components/regeneration_indicator.js diff --git a/app/controllers/api/v1/timelines/home_controller.rb b/app/controllers/api/v1/timelines/home_controller.rb index fcd0757f1..ff5ede138 100644 --- a/app/controllers/api/v1/timelines/home_controller.rb +++ b/app/controllers/api/v1/timelines/home_controller.rb @@ -13,7 +13,7 @@ class Api::V1::Timelines::HomeController < Api::BaseController render json: @statuses, each_serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new(@statuses, current_user&.account_id), - status: regeneration_in_progress? ? 206 : 200 + status: account_home_feed.regenerating? ? 206 : 200 end private @@ -62,8 +62,4 @@ class Api::V1::Timelines::HomeController < Api::BaseController def pagination_since_id @statuses.first.id end - - def regeneration_in_progress? - Redis.current.exists("account:#{current_account.id}:regeneration") - end end diff --git a/app/javascript/mastodon/actions/timelines.js b/app/javascript/mastodon/actions/timelines.js index 7eeba2aa7..bc2ac5e82 100644 --- a/app/javascript/mastodon/actions/timelines.js +++ b/app/javascript/mastodon/actions/timelines.js @@ -97,7 +97,7 @@ export function expandTimeline(timelineId, path, params = {}, done = noOp) { api(getState).get(path, { params }).then(response => { const next = getLinks(response).refs.find(link => link.rel === 'next'); dispatch(importFetchedStatuses(response.data)); - dispatch(expandTimelineSuccess(timelineId, response.data, next ? next.uri : null, response.code === 206, isLoadingRecent, isLoadingMore, isLoadingRecent && preferPendingItems)); + dispatch(expandTimelineSuccess(timelineId, response.data, next ? next.uri : null, response.status === 206, isLoadingRecent, isLoadingMore, isLoadingRecent && preferPendingItems)); done(); }).catch(error => { dispatch(expandTimelineFail(timelineId, error, isLoadingMore)); diff --git a/app/javascript/mastodon/components/missing_indicator.js b/app/javascript/mastodon/components/missing_indicator.js index 70d8c3b98..7b0101bab 100644 --- a/app/javascript/mastodon/components/missing_indicator.js +++ b/app/javascript/mastodon/components/missing_indicator.js @@ -1,17 +1,24 @@ import React from 'react'; +import PropTypes from 'prop-types'; import { FormattedMessage } from 'react-intl'; +import illustration from 'mastodon/../images/elephant_ui_disappointed.svg'; +import classNames from 'classnames'; -const MissingIndicator = () => ( -
-
-
+const MissingIndicator = ({ fullPage }) => ( +
+
+ +
-
- - -
+
+ +
); +MissingIndicator.propTypes = { + fullPage: PropTypes.bool, +}; + export default MissingIndicator; diff --git a/app/javascript/mastodon/components/regeneration_indicator.js b/app/javascript/mastodon/components/regeneration_indicator.js new file mode 100644 index 000000000..faf88c6b5 --- /dev/null +++ b/app/javascript/mastodon/components/regeneration_indicator.js @@ -0,0 +1,18 @@ +import React from 'react'; +import { FormattedMessage } from 'react-intl'; +import illustration from 'mastodon/../images/elephant_ui_working.svg'; + +const MissingIndicator = () => ( +
+
+ +
+ +
+ + +
+
+); + +export default MissingIndicator; diff --git a/app/javascript/mastodon/components/status_list.js b/app/javascript/mastodon/components/status_list.js index 745e6422d..e1b370c91 100644 --- a/app/javascript/mastodon/components/status_list.js +++ b/app/javascript/mastodon/components/status_list.js @@ -1,12 +1,12 @@ import { debounce } from 'lodash'; import React from 'react'; -import { FormattedMessage } from 'react-intl'; import ImmutablePropTypes from 'react-immutable-proptypes'; import PropTypes from 'prop-types'; import StatusContainer from '../containers/status_container'; import ImmutablePureComponent from 'react-immutable-pure-component'; import LoadGap from './load_gap'; import ScrollableList from './scrollable_list'; +import RegenerationIndicator from 'mastodon/components/regeneration_indicator'; export default class StatusList extends ImmutablePureComponent { @@ -81,18 +81,7 @@ export default class StatusList extends ImmutablePureComponent { const { isLoading, isPartial } = other; if (isPartial) { - return ( -
-
-
- -
- - -
-
-
- ); + return ; } let scrollableContent = (isLoading || statusIds.size > 0) ? ( diff --git a/app/javascript/mastodon/features/generic_not_found/index.js b/app/javascript/mastodon/features/generic_not_found/index.js index 0290be47f..41cd61a5f 100644 --- a/app/javascript/mastodon/features/generic_not_found/index.js +++ b/app/javascript/mastodon/features/generic_not_found/index.js @@ -4,7 +4,7 @@ import MissingIndicator from '../../components/missing_indicator'; const GenericNotFound = () => ( - + ); diff --git a/app/javascript/styles/mastodon/components.scss b/app/javascript/styles/mastodon/components.scss index a6675b8ed..d9182ade9 100644 --- a/app/javascript/styles/mastodon/components.scss +++ b/app/javascript/styles/mastodon/components.scss @@ -3127,37 +3127,27 @@ a.status-card.compact:hover { cursor: default; display: flex; flex: 1 1 auto; + flex-direction: column; align-items: center; justify-content: center; padding: 20px; - & > div { - width: 100%; - background: transparent; - padding-top: 0; - } - &__figure { - background: url('../images/elephant_ui_working.svg') no-repeat center 0; - width: 100%; - height: 160px; - background-size: contain; - position: absolute; - top: 50%; - left: 50%; - transform: translate(-50%, -50%); - } - - &.missing-indicator { - padding-top: 20px + 48px; - - .regeneration-indicator__figure { - background-image: url('../images/elephant_ui_disappointed.svg'); + &, + img { + display: block; + width: auto; + height: 160px; + margin: 0; } } + &--without-header { + padding-top: 20px + 48px; + } + &__label { - margin-top: 200px; + margin-top: 30px; strong { display: block; diff --git a/app/javascript/styles/mastodon/introduction.scss b/app/javascript/styles/mastodon/introduction.scss index 222d8f60e..b44ae7306 100644 --- a/app/javascript/styles/mastodon/introduction.scss +++ b/app/javascript/styles/mastodon/introduction.scss @@ -3,9 +3,10 @@ flex-direction: column; justify-content: center; align-items: center; + height: 100vh; + background: $ui-base-color; @media screen and (max-width: 920px) { - background: darken($ui-base-color, 8%); display: block !important; } diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index 871ec5c19..d8b486b60 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -19,7 +19,7 @@ class FeedManager def filter?(timeline_type, status, receiver_id) if timeline_type == :home - filter_from_home?(status, receiver_id) + filter_from_home?(status, receiver_id, build_crutches(receiver_id, [status])) elsif timeline_type == :mentions filter_from_mentions?(status, receiver_id) else @@ -29,6 +29,7 @@ class FeedManager def push_to_home(account, status) return false unless add_to_feed(:home, account.id, status, account.user&.aggregates_reblogs?) + trim(:home, account.id) PushUpdateWorker.perform_async(account.id, status.id, "timeline:#{account.id}") if push_update_required?("timeline:#{account.id}") true @@ -36,6 +37,7 @@ class FeedManager def unpush_from_home(account, status) return false unless remove_from_feed(:home, account.id, status, account.user&.aggregates_reblogs?) + redis.publish("timeline:#{account.id}", Oj.dump(event: :delete, payload: status.id.to_s)) true end @@ -46,7 +48,9 @@ class FeedManager should_filter &&= !ListAccount.where(list_id: list.id, account_id: status.in_reply_to_account_id).exists? return false if should_filter end + return false unless add_to_feed(:list, list.id, status, list.account.user&.aggregates_reblogs?) + trim(:list, list.id) PushUpdateWorker.perform_async(list.account_id, status.id, "timeline:list:#{list.id}") if push_update_required?("timeline:list:#{list.id}") true @@ -54,6 +58,7 @@ class FeedManager def unpush_from_list(list, status) return false unless remove_from_feed(:list, list.id, status, list.account.user&.aggregates_reblogs?) + redis.publish("timeline:list:#{list.id}", Oj.dump(event: :delete, payload: status.id.to_s)) true end @@ -85,16 +90,21 @@ class FeedManager def merge_into_timeline(from_account, into_account) timeline_key = key(:home, into_account.id) - query = from_account.statuses.limit(FeedManager::MAX_ITEMS / 4) + aggregate = into_account.user&.aggregates_reblogs? + query = from_account.statuses.where(visibility: [:public, :unlisted, :private]).includes(:preloadable_poll, reblog: :account).limit(FeedManager::MAX_ITEMS / 4) if redis.zcard(timeline_key) >= FeedManager::MAX_ITEMS / 4 - oldest_home_score = redis.zrange(timeline_key, 0, 0, with_scores: true)&.first&.last&.to_i || 0 + oldest_home_score = redis.zrange(timeline_key, 0, 0, with_scores: true).first.last.to_i query = query.where('id > ?', oldest_home_score) end - query.each do |status| - next if status.direct_visibility? || status.limited_visibility? || filter?(:home, status, into_account) - add_to_feed(:home, into_account.id, status, into_account.user&.aggregates_reblogs?) + statuses = query.to_a + crutches = build_crutches(into_account.id, statuses) + + statuses.each do |status| + next if filter_from_home?(status, into_account, crutches) + + add_to_feed(:home, into_account.id, status, aggregate) end trim(:home, into_account.id) @@ -120,24 +130,35 @@ class FeedManager end def populate_feed(account) - added = 0 - limit = FeedManager::MAX_ITEMS / 2 - max_id = nil + limit = FeedManager::MAX_ITEMS / 2 + aggregate = account.user&.aggregates_reblogs? + timeline_key = key(:home, account.id) - loop do - statuses = Status.as_home_timeline(account) - .paginate_by_max_id(limit, max_id) + account.statuses.where.not(visibility: :direct).limit(limit).each do |status| + add_to_feed(:home, account.id, status, aggregate) + end - break if statuses.empty? + account.following.includes(:account_stat).find_each do |target_account| + if redis.zcard(timeline_key) >= limit + oldest_home_score = redis.zrange(timeline_key, 0, 0, with_scores: true).first.last.to_i + last_status_score = Mastodon::Snowflake.id_at(account.last_status_at) - statuses.each do |status| - next if filter_from_home?(status, account) - added += 1 if add_to_feed(:home, account.id, status, account.user&.aggregates_reblogs?) + # If the feed is full and this account has not posted more recently + # than the last item on the feed, then we can skip the whole account + # because none of its statuses would stay on the feed anyway + next if last_status_score < oldest_home_score end - break unless added.zero? + statuses = target_account.statuses.where(visibility: [:public, :unlisted, :private]).includes(:preloadable_poll, reblog: :account).limit(limit) + crutches = build_crutches(account.id, statuses) - max_id = statuses.last.id + statuses.each do |status| + next if filter_from_home?(status, account, crutches) + + add_to_feed(:home, account.id, status, aggregate) + end + + trim(:home, account.id) end end @@ -152,31 +173,33 @@ class FeedManager (context == :home ? Mute.where(account_id: receiver_id, target_account_id: account_ids).any? : Mute.where(account_id: receiver_id, target_account_id: account_ids, hide_notifications: true).any?) end - def filter_from_home?(status, receiver_id) + def filter_from_home?(status, receiver_id, crutches) return false if receiver_id == status.account_id return true if status.reply? && (status.in_reply_to_id.nil? || status.in_reply_to_account_id.nil?) return true if phrase_filtered?(status, receiver_id, :home) - check_for_blocks = status.active_mentions.pluck(:account_id) + check_for_blocks = crutches[:active_mentions][status.id] || [] check_for_blocks.concat([status.account_id]) if status.reblog? check_for_blocks.concat([status.reblog.account_id]) - check_for_blocks.concat(status.reblog.active_mentions.pluck(:account_id)) + check_for_blocks.concat(crutches[:active_mentions][status.reblog_of_id] || []) end - return true if blocks_or_mutes?(receiver_id, check_for_blocks, :home) + return true if check_for_blocks.any? { |target_account_id| crutches[:blocking][target_account_id] || crutches[:muting][target_account_id] } if status.reply? && !status.in_reply_to_account_id.nil? # Filter out if it's a reply - should_filter = !Follow.where(account_id: receiver_id, target_account_id: status.in_reply_to_account_id).exists? # and I'm not following the person it's a reply to + should_filter = !crutches[:following][status.in_reply_to_account_id] # and I'm not following the person it's a reply to should_filter &&= receiver_id != status.in_reply_to_account_id # and it's not a reply to me should_filter &&= status.account_id != status.in_reply_to_account_id # and it's not a self-reply - return should_filter + + return !!should_filter elsif status.reblog? # Filter out a reblog - should_filter = Follow.where(account_id: receiver_id, target_account_id: status.account_id, show_reblogs: false).exists? # if the reblogger's reblogs are suppressed - should_filter ||= Block.where(account_id: status.reblog.account_id, target_account_id: receiver_id).exists? # or if the author of the reblogged status is blocking me - should_filter ||= AccountDomainBlock.where(account_id: receiver_id, domain: status.reblog.account.domain).exists? # or the author's domain is blocked - return should_filter + should_filter = crutches[:hiding_reblogs][status.account_id] # if the reblogger's reblogs are suppressed + should_filter ||= crutches[:blocked_by][status.reblog.account_id] # or if the author of the reblogged status is blocking me + should_filter ||= crutches[:domain_blocking][status.reblog.account.domain] # or the author's domain is blocked + + return !!should_filter end false @@ -308,4 +331,31 @@ class FeedManager redis.zrem(timeline_key, status.id) end + + def build_crutches(receiver_id, statuses) + crutches = {} + + crutches[:active_mentions] = Mention.active.where(status_id: statuses.flat_map { |s| [s.id, s.reblog_of_id] }.compact).pluck(:status_id, :account_id).each_with_object({}) { |(id, account_id), mapping| (mapping[id] ||= []).push(account_id) } + + check_for_blocks = statuses.flat_map do |s| + arr = crutches[:active_mentions][s.id] || [] + arr.concat([s.account_id]) + + if s.reblog? + arr.concat([s.reblog.account_id]) + arr.concat(crutches[:active_mentions][s.reblog_of_id] || []) + end + + arr + end + + crutches[:following] = Follow.where(account_id: receiver_id, target_account_id: statuses.map(&:in_reply_to_account_id).compact).pluck(:target_account_id).each_with_object({}) { |id, mapping| mapping[id] = true } + crutches[:hiding_reblogs] = Follow.where(account_id: receiver_id, target_account_id: statuses.map { |s| s.account_id if s.reblog? }.compact, show_reblogs: false).pluck(:target_account_id).each_with_object({}) { |id, mapping| mapping[id] = true } + crutches[:blocking] = Block.where(account_id: receiver_id, target_account_id: check_for_blocks).pluck(:target_account_id).each_with_object({}) { |id, mapping| mapping[id] = true } + crutches[:muting] = Mute.where(account_id: receiver_id, target_account_id: check_for_blocks).pluck(:target_account_id).each_with_object({}) { |id, mapping| mapping[id] = true } + crutches[:domain_blocking] = AccountDomainBlock.where(account_id: receiver_id, domain: statuses.map { |s| s.reblog&.account&.domain }.compact).pluck(:domain).each_with_object({}) { |domain, mapping| mapping[domain] = true } + crutches[:blocked_by] = Block.where(target_account_id: receiver_id, account_id: statuses.map { |s| s.reblog&.account_id }.compact).pluck(:account_id).each_with_object({}) { |id, mapping| mapping[id] = true } + + crutches + end end diff --git a/app/models/home_feed.rb b/app/models/home_feed.rb index ba7564983..1fd506138 100644 --- a/app/models/home_feed.rb +++ b/app/models/home_feed.rb @@ -7,19 +7,7 @@ class HomeFeed < Feed @account = account end - def get(limit, max_id = nil, since_id = nil, min_id = nil) - if redis.exists("account:#{@account.id}:regeneration") - from_database(limit, max_id, since_id, min_id) - else - super - end - end - - private - - def from_database(limit, max_id, since_id, min_id) - Status.as_home_timeline(@account) - .paginate_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) - .reject { |status| FeedManager.instance.filter?(:home, status, @account.id) } + def regenerating? + redis.exists("account:#{@id}:regeneration") end end diff --git a/app/models/status.rb b/app/models/status.rb index 3504ac370..0c01a5389 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -282,10 +282,6 @@ class Status < ApplicationRecord where(language: nil).or where(language: account.chosen_languages) end - def as_home_timeline(account) - where(account: [account] + account.following).where(visibility: [:public, :unlisted, :private]) - end - def as_public_timeline(account = nil, local_only = false) query = timeline_scope(local_only).without_replies diff --git a/spec/models/home_feed_spec.rb b/spec/models/home_feed_spec.rb index 3acb997f1..ee7a83960 100644 --- a/spec/models/home_feed_spec.rb +++ b/spec/models/home_feed_spec.rb @@ -34,11 +34,10 @@ RSpec.describe HomeFeed, type: :model do Redis.current.set("account:#{account.id}:regeneration", true) end - it 'gets statuses with ids in the range from database' do + it 'returns nothing' do results = subject.get(3) - expect(results.map(&:id)).to eq [10, 3, 2] - expect(results.first.attributes.keys).to include('id', 'updated_at') + expect(results.map(&:id)).to eq [] end end end diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index a8c567414..51a10cd17 100644 --- a/spec/models/status_spec.rb +++ b/spec/models/status_spec.rb @@ -296,49 +296,6 @@ RSpec.describe Status, type: :model do end end - describe '.as_home_timeline' do - let(:account) { Fabricate(:account) } - let(:followed) { Fabricate(:account) } - let(:not_followed) { Fabricate(:account) } - - before do - Fabricate(:follow, account: account, target_account: followed) - - @self_status = Fabricate(:status, account: account, visibility: :public) - @self_direct_status = Fabricate(:status, account: account, visibility: :direct) - @followed_status = Fabricate(:status, account: followed, visibility: :public) - @followed_direct_status = Fabricate(:status, account: followed, visibility: :direct) - @not_followed_status = Fabricate(:status, account: not_followed, visibility: :public) - - @results = Status.as_home_timeline(account) - end - - it 'includes statuses from self' do - expect(@results).to include(@self_status) - end - - it 'does not include direct statuses from self' do - expect(@results).to_not include(@self_direct_status) - end - - it 'includes statuses from followed' do - expect(@results).to include(@followed_status) - end - - it 'does not include direct statuses mentioning recipient from followed' do - Fabricate(:mention, account: account, status: @followed_direct_status) - expect(@results).to_not include(@followed_direct_status) - end - - it 'does not include direct statuses not mentioning recipient from followed' do - expect(@results).not_to include(@followed_direct_status) - end - - it 'does not include statuses from non-followed' do - expect(@results).not_to include(@not_followed_status) - end - end - describe '.as_public_timeline' do it 'only includes statuses with public visibility' do public_status = Fabricate(:status, visibility: :public)