From 45c7ee39b3d45159de382e8596110af04ee1ed8c Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 22 Nov 2016 17:32:51 +0100 Subject: [PATCH 1/8] Remove unneeded indices, improve error handling in background workers, don't needlessly reload reblogged status, send Devise e-mails asynchronously --- app/controllers/api/v1/statuses_controller.rb | 2 +- app/models/notification.rb | 2 ++ app/models/user.rb | 4 ++++ app/services/notify_service.rb | 2 ++ app/services/process_feed_service.rb | 2 +- app/workers/salmon_worker.rb | 2 ++ db/migrate/20161122163057_remove_unneeded_indexes.rb | 7 +++++++ db/schema.rb | 5 +---- 8 files changed, 20 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20161122163057_remove_unneeded_indexes.rb diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 53578b2f7..604e2969d 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -54,7 +54,7 @@ class Api::V1::StatusesController < ApiController end def reblog - @status = ReblogService.new.call(current_user.account, Status.find(params[:id])).reload + @status = ReblogService.new.call(current_user.account, Status.find(params[:id])) render action: :show end diff --git a/app/models/notification.rb b/app/models/notification.rb index 419f31230..c0537397c 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -11,6 +11,8 @@ class Notification < ApplicationRecord belongs_to :follow, foreign_type: 'Follow', foreign_key: 'activity_id' belongs_to :favourite, foreign_type: 'Favourite', foreign_key: 'activity_id' + validates :account_id, uniqueness: { scope: [:activity_type, :activity_id] } + STATUS_INCLUDES = [:account, :stream_entry, :media_attachments, :tags, mentions: :account, reblog: [:stream_entry, :account, :media_attachments, :tags, mentions: :account]].freeze scope :with_includes, -> { includes(status: STATUS_INCLUDES, mention: [status: STATUS_INCLUDES], favourite: [:account, status: STATUS_INCLUDES], follow: :account) } diff --git a/app/models/user.rb b/app/models/user.rb index 4eb1d20a5..7e3547dff 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -16,4 +16,8 @@ class User < ApplicationRecord has_settings do |s| s.key :notification_emails, defaults: { follow: true, reblog: true, favourite: true, mention: true } end + + def send_devise_notification(notification, *args) + devise_mailer.send(notification, self, *args).deliver_later + end end diff --git a/app/services/notify_service.rb b/app/services/notify_service.rb index c0f1d4c53..772adfb90 100644 --- a/app/services/notify_service.rb +++ b/app/services/notify_service.rb @@ -10,6 +10,8 @@ class NotifyService < BaseService create_notification send_email if email_enabled? + rescue ActiveRecord::RecordInvalid + return end private diff --git a/app/services/process_feed_service.rb b/app/services/process_feed_service.rb index 8daafd444..1cd801b80 100644 --- a/app/services/process_feed_service.rb +++ b/app/services/process_feed_service.rb @@ -61,7 +61,7 @@ class ProcessFeedService < BaseService status.save! - NotifyService.new.call(status.reblog.account, status) if status.reblog? + NotifyService.new.call(status.reblog.account, status) if status.reblog? && status.reblog.account.local? Rails.logger.debug "Queuing remote status #{status.id} (#{id}) for distribution" DistributionWorker.perform_async(status.id) status diff --git a/app/workers/salmon_worker.rb b/app/workers/salmon_worker.rb index 12b46e92f..24fb94012 100644 --- a/app/workers/salmon_worker.rb +++ b/app/workers/salmon_worker.rb @@ -5,5 +5,7 @@ class SalmonWorker def perform(account_id, body) ProcessInteractionService.new.call(body, Account.find(account_id)) + rescue ActiveRecord::RecordNotFound + true end end diff --git a/db/migrate/20161122163057_remove_unneeded_indexes.rb b/db/migrate/20161122163057_remove_unneeded_indexes.rb new file mode 100644 index 000000000..3832b878d --- /dev/null +++ b/db/migrate/20161122163057_remove_unneeded_indexes.rb @@ -0,0 +1,7 @@ +class RemoveUnneededIndexes < ActiveRecord::Migration[5.0] + def change + remove_index :notifications, name: "index_notifications_on_account_id" + remove_index :settings, name: "index_settings_on_target_type_and_target_id" + remove_index :statuses_tags, name: "index_statuses_tags_on_tag_id" + end +end diff --git a/db/schema.rb b/db/schema.rb index 20bfb36a8..42e4e081c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20161119211120) do +ActiveRecord::Schema.define(version: 20161122163057) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -103,7 +103,6 @@ ActiveRecord::Schema.define(version: 20161119211120) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.index ["account_id", "activity_id", "activity_type"], name: "account_activity", unique: true, using: :btree - t.index ["account_id"], name: "index_notifications_on_account_id", using: :btree end create_table "oauth_access_grants", force: :cascade do |t| @@ -152,7 +151,6 @@ ActiveRecord::Schema.define(version: 20161119211120) do t.datetime "created_at" t.datetime "updated_at" t.index ["target_type", "target_id", "var"], name: "index_settings_on_target_type_and_target_id_and_var", unique: true, using: :btree - t.index ["target_type", "target_id"], name: "index_settings_on_target_type_and_target_id", using: :btree end create_table "statuses", force: :cascade do |t| @@ -174,7 +172,6 @@ ActiveRecord::Schema.define(version: 20161119211120) do t.integer "status_id", null: false t.integer "tag_id", null: false t.index ["tag_id", "status_id"], name: "index_statuses_tags_on_tag_id_and_status_id", unique: true, using: :btree - t.index ["tag_id"], name: "index_statuses_tags_on_tag_id", using: :btree end create_table "stream_entries", force: :cascade do |t| From 4b5b953d4223776860bcccb56d2f9c7144dc302e Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 22 Nov 2016 22:26:44 +0100 Subject: [PATCH 2/8] Upgrade to latest Rails, which apparently improves ActionCable performance --- Gemfile | 2 +- Gemfile.lock | 54 +++++++++++++++++++++++++++++----------------------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/Gemfile b/Gemfile index 9654c8828..4abed33ff 100644 --- a/Gemfile +++ b/Gemfile @@ -2,7 +2,7 @@ source 'https://rubygems.org' -gem 'rails', '5.0.0.1' +gem 'rails', git: 'https://github.com/rails/rails.git', branch: '5-0-stable' gem 'sass-rails', '~> 5.0' gem 'uglifier', '>= 1.3.0' gem 'coffee-rails', '~> 4.1.0' diff --git a/Gemfile.lock b/Gemfile.lock index a31573af6..b058ae940 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,5 +1,7 @@ -GEM - remote: https://rubygems.org/ +GIT + remote: https://github.com/rails/rails.git + revision: ecb394a31420f6fd1d0ab692c79f2dd44176e2c9 + branch: 5-0-stable specs: actioncable (5.0.0.1) actionpack (= 5.0.0.1) @@ -24,7 +26,6 @@ GEM erubis (~> 2.7.0) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.0.2) - active_record_query_trace (1.5.3) activejob (5.0.0.1) activesupport (= 5.0.0.1) globalid (>= 0.3.6) @@ -39,8 +40,31 @@ GEM i18n (~> 0.7) minitest (~> 5.1) tzinfo (~> 1.1) + rails (5.0.0.1) + actioncable (= 5.0.0.1) + actionmailer (= 5.0.0.1) + actionpack (= 5.0.0.1) + actionview (= 5.0.0.1) + activejob (= 5.0.0.1) + activemodel (= 5.0.0.1) + activerecord (= 5.0.0.1) + activesupport (= 5.0.0.1) + bundler (>= 1.3.0, < 2.0) + railties (= 5.0.0.1) + sprockets-rails (>= 2.0.0) + railties (5.0.0.1) + actionpack (= 5.0.0.1) + activesupport (= 5.0.0.1) + method_source + rake (>= 0.8.7) + thor (>= 0.18.1, < 2.0) + +GEM + remote: https://rubygems.org/ + specs: + active_record_query_trace (1.5.3) addressable (2.4.0) - arel (7.1.1) + arel (7.1.4) ast (2.3.0) autoprefixer-rails (6.5.0.2) execjs @@ -242,18 +266,6 @@ GEM rack rack-test (0.6.3) rack (>= 1.0) - rails (5.0.0.1) - actioncable (= 5.0.0.1) - actionmailer (= 5.0.0.1) - actionpack (= 5.0.0.1) - actionview (= 5.0.0.1) - activejob (= 5.0.0.1) - activemodel (= 5.0.0.1) - activerecord (= 5.0.0.1) - activesupport (= 5.0.0.1) - bundler (>= 1.3.0, < 2.0) - railties (= 5.0.0.1) - sprockets-rails (>= 2.0.0) rails-dom-testing (2.0.1) activesupport (>= 4.2.0, < 6.0) nokogiri (~> 1.6.0) @@ -266,12 +278,6 @@ GEM rails (> 3.1) rails_serve_static_assets (0.0.5) rails_stdout_logging (0.0.5) - railties (5.0.0.1) - actionpack (= 5.0.0.1) - activesupport (= 5.0.0.1) - method_source - rake (>= 0.8.7) - thor (>= 0.18.1, < 2.0) rainbow (2.1.0) rake (11.3.0) rdoc (4.2.2) @@ -362,7 +368,7 @@ GEM sprockets (3.7.0) concurrent-ruby (~> 1.0) rack (> 1, < 3) - sprockets-rails (3.1.1) + sprockets-rails (3.2.0) actionpack (>= 4.0) activesupport (>= 4.0) sprockets (>= 3.0.0) @@ -443,7 +449,7 @@ DEPENDENCIES rabl rack-attack rack-cors - rails (= 5.0.0.1) + rails! rails_12factor rails_autolink react-rails From fc90d38893ae81b83884c3f5c85d05d564f1de33 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 22 Nov 2016 22:59:54 +0100 Subject: [PATCH 3/8] Moving some counter queries out of subqueries in the API --- Gemfile | 5 +++++ Gemfile.lock | 9 +++++++++ .../components/middleware/errors.jsx | 2 ++ app/controllers/api/v1/accounts_controller.rb | 14 +++++++++++--- app/controllers/api/v1/statuses_controller.rb | 13 ++++++++++--- app/controllers/application_controller.rb | 5 +++++ app/models/status.rb | 4 ++-- app/services/search_service.rb | 2 +- config/initializers/mini_profiler.rb | 17 +++++++++++++++++ 9 files changed, 62 insertions(+), 9 deletions(-) create mode 100644 config/initializers/mini_profiler.rb diff --git a/Gemfile b/Gemfile index 4abed33ff..327a17ee9 100644 --- a/Gemfile +++ b/Gemfile @@ -50,6 +50,11 @@ gem 'react-rails' gem 'browserify-rails' gem 'autoprefixer-rails' +gem 'rack-mini-profiler', require: false +gem 'flamegraph' +gem 'stackprof' +gem 'memory_profiler' + group :development, :test do gem 'rspec-rails' gem 'pry-rails' diff --git a/Gemfile.lock b/Gemfile.lock index b058ae940..28ad1abb6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -136,6 +136,7 @@ GEM execjs (2.7.0) fabrication (2.15.2) fast_blank (1.0.0) + flamegraph (0.9.5) font-awesome-rails (4.6.3.1) railties (>= 3.2, < 5.1) fuubar (2.1.1) @@ -205,6 +206,7 @@ GEM nokogiri (>= 1.5.9) mail (2.6.4) mime-types (>= 1.16, < 4) + memory_profiler (0.9.7) method_source (0.8.2) mime-types (3.1) mime-types-data (~> 3.2015) @@ -262,6 +264,8 @@ GEM rack-attack (5.0.1) rack rack-cors (0.4.0) + rack-mini-profiler (0.10.1) + rack (>= 1.2.0) rack-protection (1.5.3) rack rack-test (0.6.3) @@ -372,6 +376,7 @@ GEM actionpack (>= 4.0) activesupport (>= 4.0) sprockets (>= 3.0.0) + stackprof (0.2.10) temple (0.7.7) term-ansicolor (1.4.0) tins (~> 1.0) @@ -420,6 +425,7 @@ DEPENDENCIES dotenv-rails fabrication fast_blank + flamegraph font-awesome-rails fuubar goldfinger @@ -435,6 +441,7 @@ DEPENDENCIES letter_opener link_header lograge + memory_profiler neography nokogiri oj @@ -449,6 +456,7 @@ DEPENDENCIES rabl rack-attack rack-cors + rack-mini-profiler rails! rails_12factor rails_autolink @@ -463,6 +471,7 @@ DEPENDENCIES sidekiq simple_form simplecov + stackprof uglifier (>= 1.3.0) webmock will_paginate diff --git a/app/assets/javascripts/components/middleware/errors.jsx b/app/assets/javascripts/components/middleware/errors.jsx index fb161fc4c..3a1473bc1 100644 --- a/app/assets/javascripts/components/middleware/errors.jsx +++ b/app/assets/javascripts/components/middleware/errors.jsx @@ -1,11 +1,13 @@ import { showAlert } from '../actions/alerts'; +const defaultSuccessSuffix = 'SUCCESS'; const defaultFailSuffix = 'FAIL'; export default function errorsMiddleware() { return ({ dispatch }) => next => action => { if (action.type) { const isFail = new RegExp(`${defaultFailSuffix}$`, 'g'); + const isSuccess = new RegExp(`${defaultSuccessSuffix}$`, 'g'); if (action.type.match(isFail)) { if (action.error.response) { diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index 2dfab0831..97d626af2 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -18,9 +18,11 @@ class Api::V1::AccountsController < ApiController def following results = Follow.where(account: @account).paginate_by_max_id(DEFAULT_ACCOUNTS_LIMIT, params[:max_id], params[:since_id]) - accounts = Account.where(id: results.map(&:target_account_id)).with_counters.map { |a| [a.id, a] }.to_h + accounts = Account.where(id: results.map(&:target_account_id)).map { |a| [a.id, a] }.to_h @accounts = results.map { |f| accounts[f.target_account_id] } + set_account_counters_maps(@accounts) + next_path = following_api_v1_account_url(max_id: results.last.id) if results.size == DEFAULT_ACCOUNTS_LIMIT prev_path = following_api_v1_account_url(since_id: results.first.id) unless results.empty? @@ -31,9 +33,11 @@ class Api::V1::AccountsController < ApiController def followers results = Follow.where(target_account: @account).paginate_by_max_id(DEFAULT_ACCOUNTS_LIMIT, params[:max_id], params[:since_id]) - accounts = Account.where(id: results.map(&:account_id)).with_counters.map { |a| [a.id, a] }.to_h + accounts = Account.where(id: results.map(&:account_id)).map { |a| [a.id, a] }.to_h @accounts = results.map { |f| accounts[f.account_id] } + set_account_counters_maps(@accounts) + next_path = followers_api_v1_account_url(max_id: results.last.id) if results.size == DEFAULT_ACCOUNTS_LIMIT prev_path = followers_api_v1_account_url(since_id: results.first.id) unless results.empty? @@ -53,9 +57,10 @@ class Api::V1::AccountsController < ApiController end def statuses - @statuses = @account.statuses.with_includes.with_counters.paginate_by_max_id(DEFAULT_STATUSES_LIMIT, params[:max_id], params[:since_id]).to_a + @statuses = @account.statuses.with_includes.paginate_by_max_id(DEFAULT_STATUSES_LIMIT, params[:max_id], params[:since_id]).to_a set_maps(@statuses) + set_counters_maps(@statuses) next_path = statuses_api_v1_account_url(max_id: @statuses.last.id) if @statuses.size == DEFAULT_STATUSES_LIMIT prev_path = statuses_api_v1_account_url(since_id: @statuses.first.id) unless @statuses.empty? @@ -98,6 +103,9 @@ class Api::V1::AccountsController < ApiController def search limit = params[:limit] ? [DEFAULT_ACCOUNTS_LIMIT, params[:limit].to_i].min : DEFAULT_ACCOUNTS_LIMIT @accounts = SearchService.new.call(params[:q], limit, params[:resolve] == 'true') + + set_account_counters_maps(@accounts) + render action: :index end diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 604e2969d..b2b432a6b 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -13,14 +13,19 @@ class Api::V1::StatusesController < ApiController def context @context = OpenStruct.new(ancestors: @status.ancestors(current_account), descendants: @status.descendants(current_account)) - set_maps([@status] + @context[:ancestors] + @context[:descendants]) + statuses = [@status] + @context[:ancestors] + @context[:descendants] + + set_maps(statuses) + set_counters_maps(statuses) end def reblogged_by results = @status.reblogs.paginate_by_max_id(DEFAULT_ACCOUNTS_LIMIT, params[:max_id], params[:since_id]) - accounts = Account.where(id: results.map(&:account_id)).with_counters.map { |a| [a.id, a] }.to_h + accounts = Account.where(id: results.map(&:account_id)).map { |a| [a.id, a] }.to_h @accounts = results.map { |r| accounts[r.account_id] } + set_account_counters_maps(@accounts) + next_path = reblogged_by_api_v1_status_url(max_id: results.last.id) if results.size == DEFAULT_ACCOUNTS_LIMIT prev_path = reblogged_by_api_v1_status_url(since_id: results.first.id) unless results.empty? @@ -31,9 +36,11 @@ class Api::V1::StatusesController < ApiController def favourited_by results = @status.favourites.paginate_by_max_id(DEFAULT_ACCOUNTS_LIMIT, params[:max_id], params[:since_id]) - accounts = Account.where(id: results.map(&:account_id)).with_counters.map { |a| [a.id, a] }.to_h + accounts = Account.where(id: results.map(&:account_id)).map { |a| [a.id, a] }.to_h @accounts = results.map { |f| accounts[f.account_id] } + set_account_counters_maps(@accounts) + next_path = favourited_by_api_v1_status_url(max_id: results.last.id) if results.size == DEFAULT_ACCOUNTS_LIMIT prev_path = favourited_by_api_v1_status_url(since_id: results.first.id) unless results.empty? diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3a4c95db4..283f950cc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -14,6 +14,7 @@ class ApplicationController < ActionController::Base before_action :store_current_location, except: :raise_not_found, unless: :devise_controller? before_action :set_locale + before_action :check_rack_mini_profiler def raise_not_found raise ActionController::RoutingError, "No route matches #{params[:unmatched_route]}" @@ -31,6 +32,10 @@ class ApplicationController < ActionController::Base I18n.locale = I18n.default_locale end + def check_rack_mini_profiler + Rack::MiniProfiler.authorize_request if current_user && current_user.admin? + end + protected def not_found diff --git a/app/models/status.rb b/app/models/status.rb index 1bf4b49c9..53987c3ec 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -71,7 +71,7 @@ class Status < ApplicationRecord def ancestors(account = nil) ids = (Status.find_by_sql(['WITH RECURSIVE search_tree(id, in_reply_to_id, path) AS (SELECT id, in_reply_to_id, ARRAY[id] FROM statuses WHERE id = ? UNION ALL SELECT statuses.id, statuses.in_reply_to_id, path || statuses.id FROM search_tree JOIN statuses ON statuses.id = search_tree.in_reply_to_id WHERE NOT statuses.id = ANY(path)) SELECT id FROM search_tree ORDER BY path DESC', id]) - [self]).pluck(:id) - statuses = Status.where(id: ids).with_counters.with_includes.group_by(&:id) + statuses = Status.where(id: ids).with_includes.group_by(&:id) results = ids.map { |id| statuses[id].first } results = results.reject { |status| account.blocking?(status.account) } unless account.nil? @@ -80,7 +80,7 @@ class Status < ApplicationRecord def descendants(account = nil) ids = (Status.find_by_sql(['WITH RECURSIVE search_tree(id, path) AS (SELECT id, ARRAY[id] FROM statuses WHERE id = ? UNION ALL SELECT statuses.id, path || statuses.id FROM search_tree JOIN statuses ON statuses.in_reply_to_id = search_tree.id WHERE NOT statuses.id = ANY(path)) SELECT id FROM search_tree ORDER BY path', id]) - [self]).pluck(:id) - statuses = Status.where(id: ids).with_counters.with_includes.group_by(&:id) + statuses = Status.where(id: ids).with_includes.group_by(&:id) results = ids.map { |id| statuses[id].first } results = results.reject { |status| account.blocking?(status.account) } unless account.nil? diff --git a/app/services/search_service.rb b/app/services/search_service.rb index c4cffda13..598c7d02c 100644 --- a/app/services/search_service.rb +++ b/app/services/search_service.rb @@ -12,7 +12,7 @@ class SearchService < BaseService Account.search_for("#{username} #{domain}") end - results = results.limit(limit).with_counters + results = results.limit(limit) if resolve && results.empty? && !domain.nil? results = [FollowRemoteAccountService.new.call("#{username}@#{domain}")] diff --git a/config/initializers/mini_profiler.rb b/config/initializers/mini_profiler.rb new file mode 100644 index 000000000..265783618 --- /dev/null +++ b/config/initializers/mini_profiler.rb @@ -0,0 +1,17 @@ +require 'rack-mini-profiler' + +Rack::MiniProfilerRails.initialize!(Rails.application) + +Rails.application.middleware.swap(Rack::Deflater, Rack::MiniProfiler) +Rails.application.middleware.swap(Rack::MiniProfiler, Rack::Deflater) + +Rack::MiniProfiler.config.storage = Rack::MiniProfiler::MemoryStore + +if Rails.env.production? + Rack::MiniProfiler.config.storage_options = { + host: ENV.fetch('REDIS_HOST') { 'localhost' }, + port: ENV.fetch('REDIS_PORT') { 6379 }, + } + + Rack::MiniProfiler.config.storage = Rack::MiniProfiler::RedisStore +end From 5c78547198de20e7f367adb142acc82cd80b4899 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 22 Nov 2016 23:18:54 +0100 Subject: [PATCH 4/8] More query optimizations --- app/controllers/api/v1/timelines_controller.rb | 8 ++++++++ app/controllers/api_controller.rb | 4 ++-- app/controllers/application_controller.rb | 2 +- app/models/feed.rb | 2 +- app/models/status.rb | 4 ++-- 5 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/v1/timelines_controller.rb b/app/controllers/api/v1/timelines_controller.rb index 19b76f11d..c5ed315d9 100644 --- a/app/controllers/api/v1/timelines_controller.rb +++ b/app/controllers/api/v1/timelines_controller.rb @@ -10,6 +10,8 @@ class Api::V1::TimelinesController < ApiController @statuses = Feed.new(:home, current_account).get(DEFAULT_STATUSES_LIMIT, params[:max_id], params[:since_id]).to_a set_maps(@statuses) + set_counters_maps(@statuses) + set_account_counters_maps(@statuses.flat_map { |s| [s.account, s.reblog? ? s.reblog.account : nil] }.compact.uniq) next_path = api_v1_home_timeline_url(max_id: @statuses.last.id) if @statuses.size == DEFAULT_STATUSES_LIMIT prev_path = api_v1_home_timeline_url(since_id: @statuses.first.id) unless @statuses.empty? @@ -23,6 +25,8 @@ class Api::V1::TimelinesController < ApiController @statuses = Feed.new(:mentions, current_account).get(DEFAULT_STATUSES_LIMIT, params[:max_id], params[:since_id]).to_a set_maps(@statuses) + set_counters_maps(@statuses) + set_account_counters_maps(@statuses.flat_map { |s| [s.account, s.reblog? ? s.reblog.account : nil] }.compact.uniq) next_path = api_v1_mentions_timeline_url(max_id: @statuses.last.id) if @statuses.size == DEFAULT_STATUSES_LIMIT prev_path = api_v1_mentions_timeline_url(since_id: @statuses.first.id) unless @statuses.empty? @@ -36,6 +40,8 @@ class Api::V1::TimelinesController < ApiController @statuses = Status.as_public_timeline(current_account).paginate_by_max_id(DEFAULT_STATUSES_LIMIT, params[:max_id], params[:since_id]).to_a set_maps(@statuses) + set_counters_maps(@statuses) + set_account_counters_maps(@statuses.flat_map { |s| [s.account, s.reblog? ? s.reblog.account : nil] }.compact.uniq) next_path = api_v1_public_timeline_url(max_id: @statuses.last.id) if @statuses.size == DEFAULT_STATUSES_LIMIT prev_path = api_v1_public_timeline_url(since_id: @statuses.first.id) unless @statuses.empty? @@ -50,6 +56,8 @@ class Api::V1::TimelinesController < ApiController @statuses = @tag.nil? ? [] : Status.as_tag_timeline(@tag, current_account).paginate_by_max_id(DEFAULT_STATUSES_LIMIT, params[:max_id], params[:since_id]).to_a set_maps(@statuses) + set_counters_maps(@statuses) + set_account_counters_maps(@statuses.flat_map { |s| [s.account, s.reblog? ? s.reblog.account : nil] }.compact.uniq) next_path = api_v1_hashtag_timeline_url(params[:id], max_id: @statuses.last.id) if @statuses.size == DEFAULT_STATUSES_LIMIT prev_path = api_v1_hashtag_timeline_url(params[:id], since_id: @statuses.first.id) unless @statuses.empty? diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 862358d6e..a008f4c1b 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -59,11 +59,11 @@ class ApiController < ApplicationController end def current_resource_owner - User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token + @user ||= User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token end def current_user - super || current_resource_owner + @user ||= super || current_resource_owner rescue ActiveRecord::RecordNotFound nil end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 283f950cc..07526d46e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -51,6 +51,6 @@ class ApplicationController < ActionController::Base end def current_account - current_user.try(:account) + @account ||= current_user.try(:account) end end diff --git a/app/models/feed.rb b/app/models/feed.rb index e7f2ab3a5..6337726be 100644 --- a/app/models/feed.rb +++ b/app/models/feed.rb @@ -16,7 +16,7 @@ class Feed RegenerationWorker.perform_async(@account.id, @type) @statuses = Status.send("as_#{@type}_timeline", @account).paginate_by_max_id(limit, nil, nil) else - status_map = Status.where(id: unhydrated).with_includes.with_counters.map { |status| [status.id, status] }.to_h + status_map = Status.where(id: unhydrated).with_includes.map { |status| [status.id, status] }.to_h @statuses = unhydrated.map { |id| status_map[id] }.compact end diff --git a/app/models/status.rb b/app/models/status.rb index 53987c3ec..708fef17f 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -4,7 +4,7 @@ class Status < ApplicationRecord include Paginable include Streamable - belongs_to :account, -> { with_counters }, inverse_of: :statuses + belongs_to :account, inverse_of: :statuses belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, touch: true @@ -27,7 +27,7 @@ class Status < ApplicationRecord default_scope { order('id desc') } scope :with_counters, -> { select('statuses.*, (select count(r.id) from statuses as r where r.reblog_of_id = statuses.id) as reblogs_count, (select count(f.id) from favourites as f where f.status_id = statuses.id) as favourites_count') } - scope :with_includes, -> { includes(:account, :media_attachments, :tags, :stream_entry, mentions: :account, reblog: [:account, mentions: :account], thread: :account) } + scope :with_includes, -> { includes(:account, :media_attachments, :tags, :stream_entry, mentions: :account, reblog: [:account, :stream_entry, :tags, mentions: :account], thread: :account) } def local? uri.nil? From cf912e01fd74e7320eb4027c35f7744a36d2f2b8 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 23 Nov 2016 08:34:35 +0100 Subject: [PATCH 5/8] Implement includes caching for timelines APIs --- .../api/v1/timelines_controller.rb | 23 +++++++++++++++++ app/models/feed.rb | 25 ++++++++++++++++++- app/models/status.rb | 13 ++++------ 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/v1/timelines_controller.rb b/app/controllers/api/v1/timelines_controller.rb index c5ed315d9..b1d7c3052 100644 --- a/app/controllers/api/v1/timelines_controller.rb +++ b/app/controllers/api/v1/timelines_controller.rb @@ -38,6 +38,7 @@ class Api::V1::TimelinesController < ApiController def public @statuses = Status.as_public_timeline(current_account).paginate_by_max_id(DEFAULT_STATUSES_LIMIT, params[:max_id], params[:since_id]).to_a + @statuses = cache(@statuses) set_maps(@statuses) set_counters_maps(@statuses) @@ -54,6 +55,7 @@ class Api::V1::TimelinesController < ApiController def tag @tag = Tag.find_by(name: params[:id].downcase) @statuses = @tag.nil? ? [] : Status.as_tag_timeline(@tag, current_account).paginate_by_max_id(DEFAULT_STATUSES_LIMIT, params[:max_id], params[:since_id]).to_a + @statuses = cache(@statuses) set_maps(@statuses) set_counters_maps(@statuses) @@ -66,4 +68,25 @@ class Api::V1::TimelinesController < ApiController render action: :index end + + private + + def cache(raw) + uncached_ids = [] + cached_keys_with_value = Rails.cache.read_multi(*raw.map(&:cache_key)) + + raw.each do |status| + uncached_ids << status.id unless cached_keys_with_value.key?(status.cache_key) + end + + unless uncached_ids.empty? + uncached = Status.where(id: uncached_ids).with_includes.map { |s| [s.id, s] }.to_h + + uncached.values.each do |status| + Rails.cache.write(status.cache_key, status) + end + end + + raw.map { |status| cached_keys_with_value[status.cache_key] || uncached[status.id] } + end end diff --git a/app/models/feed.rb b/app/models/feed.rb index 6337726be..45cb923d1 100644 --- a/app/models/feed.rb +++ b/app/models/feed.rb @@ -16,7 +16,7 @@ class Feed RegenerationWorker.perform_async(@account.id, @type) @statuses = Status.send("as_#{@type}_timeline", @account).paginate_by_max_id(limit, nil, nil) else - status_map = Status.where(id: unhydrated).with_includes.map { |status| [status.id, status] }.to_h + status_map = cache(unhydrated) @statuses = unhydrated.map { |id| status_map[id] }.compact end @@ -25,6 +25,29 @@ class Feed private + def cache(ids) + raw = Status.where(id: ids).to_a + uncached_ids = [] + cached_keys_with_value = Rails.cache.read_multi(*raw.map(&:cache_key)) + + raw.each do |status| + uncached_ids << status.id unless cached_keys_with_value.key?(status.cache_key) + end + + unless uncached_ids.empty? + uncached = Status.where(id: uncached_ids).with_includes.map { |s| [s.id, s] }.to_h + + uncached.values.each do |status| + Rails.cache.write(status.cache_key, status) + end + end + + cached = cached_keys_with_value.values.map { |s| [s.id, s] }.to_h + cached.merge!(uncached) unless uncached_ids.empty? + + cached + end + def key FeedManager.instance.key(@type, @account.id) end diff --git a/app/models/status.rb b/app/models/status.rb index 708fef17f..3402929bf 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -27,7 +27,7 @@ class Status < ApplicationRecord default_scope { order('id desc') } scope :with_counters, -> { select('statuses.*, (select count(r.id) from statuses as r where r.reblog_of_id = statuses.id) as reblogs_count, (select count(f.id) from favourites as f where f.status_id = statuses.id) as favourites_count') } - scope :with_includes, -> { includes(:account, :media_attachments, :tags, :stream_entry, mentions: :account, reblog: [:account, :stream_entry, :tags, mentions: :account], thread: :account) } + scope :with_includes, -> { includes(:account, :media_attachments, :tags, :stream_entry, mentions: :account, reblog: [:account, :stream_entry, :tags, :media_attachments, mentions: :account], thread: :account) } def local? uri.nil? @@ -89,28 +89,25 @@ class Status < ApplicationRecord class << self def as_home_timeline(account) - where(account: [account] + account.following).with_includes.with_counters + where(account: [account] + account.following).with_includes end def as_mentions_timeline(account) - where(id: Mention.where(account: account).pluck(:status_id)).with_includes.with_counters + where(id: Mention.where(account: account).pluck(:status_id)).with_includes end def as_public_timeline(account = nil) query = joins('LEFT OUTER JOIN accounts ON statuses.account_id = accounts.id').where('accounts.silenced = FALSE') query = filter_timeline(query, account) unless account.nil? - - query.with_includes.with_counters + query end def as_tag_timeline(tag, account = nil) query = tag.statuses .joins('LEFT OUTER JOIN accounts ON statuses.account_id = accounts.id') .where('accounts.silenced = FALSE') - query = filter_timeline(query, account) unless account.nil? - - query.with_includes.with_counters + query end def favourites_map(status_ids, account_id) From 9b53c7d353477e21ce5f3c6b56beb853cc1c0ef2 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 23 Nov 2016 08:39:04 +0100 Subject: [PATCH 6/8] Fix #168 - Turn off e-mail notifications by default --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 7e3547dff..366172e9a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -14,7 +14,7 @@ class User < ApplicationRecord scope :admins, -> { where(admin: true) } has_settings do |s| - s.key :notification_emails, defaults: { follow: true, reblog: true, favourite: true, mention: true } + s.key :notification_emails, defaults: { follow: false, reblog: false, favourite: false, mention: false } end def send_devise_notification(notification, *args) From 4bdb6a0eaffb39fb953689729c9fcf772c7128d6 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 23 Nov 2016 09:20:34 +0100 Subject: [PATCH 7/8] Rename "publish" to "toot" in english locale, fix lightbox showing old image before loading new one, cache notifications API, fix missing follow button on public profiles --- .../components/components/lightbox.jsx | 4 +++- .../javascripts/components/locales/en.jsx | 2 +- .../api/v1/notifications_controller.rb | 24 ++++++++++++++++++- app/controllers/api_controller.rb | 4 ++-- app/controllers/application_controller.rb | 2 +- 5 files changed, 30 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/components/components/lightbox.jsx b/app/assets/javascripts/components/components/lightbox.jsx index 537bab954..36f078a3a 100644 --- a/app/assets/javascripts/components/components/lightbox.jsx +++ b/app/assets/javascripts/components/components/lightbox.jsx @@ -43,13 +43,15 @@ const Lightbox = React.createClass({ render () { const { intl, isVisible, onOverlayClicked, onCloseClicked, children } = this.props; + const content = isVisible ? children :
; + return (
{({ y }) =>
- {children} + {content}
} diff --git a/app/assets/javascripts/components/locales/en.jsx b/app/assets/javascripts/components/locales/en.jsx index b2c8390c1..f3326bf90 100644 --- a/app/assets/javascripts/components/locales/en.jsx +++ b/app/assets/javascripts/components/locales/en.jsx @@ -34,7 +34,7 @@ const en = { "tabs_bar.public": "Public", "tabs_bar.notifications": "Notifications", "compose_form.placeholder": "What is on your mind?", - "compose_form.publish": "Publish", + "compose_form.publish": "Toot", "navigation_bar.settings": "Settings", "navigation_bar.public_timeline": "Public timeline", "navigation_bar.logout": "Logout", diff --git a/app/controllers/api/v1/notifications_controller.rb b/app/controllers/api/v1/notifications_controller.rb index c76189e87..d74b99a86 100644 --- a/app/controllers/api/v1/notifications_controller.rb +++ b/app/controllers/api/v1/notifications_controller.rb @@ -7,7 +7,8 @@ class Api::V1::NotificationsController < ApiController respond_to :json def index - @notifications = Notification.where(account: current_account).with_includes.paginate_by_max_id(20, params[:max_id], params[:since_id]) + @notifications = Notification.where(account: current_account).paginate_by_max_id(20, params[:max_id], params[:since_id]) + @notifications = cache(@notifications) statuses = @notifications.select { |n| !n.target_status.nil? }.map(&:target_status) set_maps(statuses) @@ -19,4 +20,25 @@ class Api::V1::NotificationsController < ApiController set_pagination_headers(next_path, prev_path) end + + private + + def cache(raw) + uncached_ids = [] + cached_keys_with_value = Rails.cache.read_multi(*raw.map(&:cache_key)) + + raw.each do |notification| + uncached_ids << notification.id unless cached_keys_with_value.key?(notification.cache_key) + end + + unless uncached_ids.empty? + uncached = Notification.where(id: uncached_ids).with_includes.map { |n| [n.id, n] }.to_h + + uncached.values.each do |notification| + Rails.cache.write(notification.cache_key, notification) + end + end + + raw.map { |notification| cached_keys_with_value[notification.cache_key] || uncached[notification.id] } + end end diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index a008f4c1b..a3a2a3275 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -59,11 +59,11 @@ class ApiController < ApplicationController end def current_resource_owner - @user ||= User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token + @current_user ||= User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token end def current_user - @user ||= super || current_resource_owner + super || current_resource_owner rescue ActiveRecord::RecordNotFound nil end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 07526d46e..effb4ed78 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -51,6 +51,6 @@ class ApplicationController < ActionController::Base end def current_account - @account ||= current_user.try(:account) + @current_account ||= current_user.try(:account) end end From 82191b33831f26e071a27dd59ce2e46fb467589c Mon Sep 17 00:00:00 2001 From: Andrea Faulds Date: Wed, 23 Nov 2016 08:30:53 +0000 Subject: [PATCH 8/8] Explain application secrets (#217) --- .env.production.sample | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.env.production.sample b/.env.production.sample index b68ba523c..a3da10b97 100644 --- a/.env.production.sample +++ b/.env.production.sample @@ -14,6 +14,8 @@ LOCAL_DOMAIN=example.com LOCAL_HTTPS=true # Application secrets +# These are arbitrary strings. They should be long and cryptographically secure. +# For Docker, `docker-compose run --rm web rake secret` will generate them. PAPERCLIP_SECRET= SECRET_KEY_BASE=