From b0ea6764e0f1282e262182a3e1f048bd4b59d34f Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 7 Jul 2015 12:52:19 +0800 Subject: [PATCH] PERF: Cache About#stats. --- app/jobs/scheduled/about_stats.rb | 13 ++++++++++ app/jobs/scheduled/dashboard_stats.rb | 11 ++++----- app/jobs/stats.rb | 9 +++++++ app/models/about.rb | 9 +++++++ app/models/admin_dashboard_data.rb | 14 ++--------- app/models/concerns/stats_cacheable.rb | 24 +++++++++++++++++++ app/serializers/about_serializer.rb | 4 ++++ spec/jobs/about_stats_spec.rb | 10 ++++++++ spec/jobs/dashboard_stats_spec.rb | 10 ++++++++ spec/models/about_spec.rb | 9 +++++++ spec/models/admin_dashboard_data_spec.rb | 4 ++++ .../shared_examples_for_stats_cacheable.rb | 23 ++++++++++++++++++ 12 files changed, 121 insertions(+), 19 deletions(-) create mode 100644 app/jobs/scheduled/about_stats.rb create mode 100644 app/jobs/stats.rb create mode 100644 app/models/concerns/stats_cacheable.rb create mode 100644 spec/jobs/about_stats_spec.rb create mode 100644 spec/jobs/dashboard_stats_spec.rb create mode 100644 spec/models/about_spec.rb create mode 100644 spec/support/shared_examples_for_stats_cacheable.rb diff --git a/app/jobs/scheduled/about_stats.rb b/app/jobs/scheduled/about_stats.rb new file mode 100644 index 000000000..d745f8a0d --- /dev/null +++ b/app/jobs/scheduled/about_stats.rb @@ -0,0 +1,13 @@ +module Jobs + class AboutStats < Jobs::Scheduled + include Jobs::Stats + + every 30.minutes + + def execute(args) + stats = About.new.stats + set_cache(About, stats) + stats + end + end +end diff --git a/app/jobs/scheduled/dashboard_stats.rb b/app/jobs/scheduled/dashboard_stats.rb index b7123f5d5..ab716b575 100644 --- a/app/jobs/scheduled/dashboard_stats.rb +++ b/app/jobs/scheduled/dashboard_stats.rb @@ -1,16 +1,13 @@ module Jobs class DashboardStats < Jobs::Scheduled + include Jobs::Stats + every 30.minutes def execute(args) - stats = AdminDashboardData.fetch_stats.as_json - - # Add some extra time to the expiry so that the next job run has plenty of time to - # finish before previous cached value expires. - $redis.setex AdminDashboardData.stats_cache_key, (AdminDashboardData.recalculate_interval + 5).minutes, stats.to_json - + stats = AdminDashboardData.new.as_json + set_cache(AdminDashboardData, stats) stats end - end end diff --git a/app/jobs/stats.rb b/app/jobs/stats.rb new file mode 100644 index 000000000..637091cc6 --- /dev/null +++ b/app/jobs/stats.rb @@ -0,0 +1,9 @@ +module Jobs + module Stats + def set_cache(klass, stats) + # Add some extra time to the expiry so that the next job run has plenty of time to + # finish before previous cached value expires. + $redis.setex klass.stats_cache_key, (klass.recalculate_stats_interval + 5).minutes, stats.to_json + end + end +end diff --git a/app/models/about.rb b/app/models/about.rb index 34fb8eadf..1c96d032e 100644 --- a/app/models/about.rb +++ b/app/models/about.rb @@ -1,9 +1,18 @@ class About include ActiveModel::Serialization + include StatsCacheable attr_accessor :moderators, :admins + def self.stats_cache_key + 'about-stats' + end + + def self.fetch_stats + About.new.stats + end + def version Discourse::VERSION::STRING end diff --git a/app/models/admin_dashboard_data.rb b/app/models/admin_dashboard_data.rb index bee0d49b8..148faa9c4 100644 --- a/app/models/admin_dashboard_data.rb +++ b/app/models/admin_dashboard_data.rb @@ -1,6 +1,7 @@ require_dependency 'mem_info' class AdminDashboardData + include StatsCacheable GLOBAL_REPORTS ||= [ 'visits', @@ -57,13 +58,7 @@ class AdminDashboardData def self.fetch_stats - AdminDashboardData.new - end - - def self.fetch_cached_stats - # The DashboardStats job is responsible for generating and caching this. - stats = $redis.get(stats_cache_key) - stats ? JSON.parse(stats) : nil + AdminDashboardData.new.as_json end def self.stats_cache_key @@ -96,11 +91,6 @@ class AdminDashboardData source.map { |type| Report.find(type).as_json } end - # Could be configurable, multisite need to support it. - def self.recalculate_interval - 30 # minutes - end - def rails_env_check I18n.t("dashboard.rails_env_warning", env: Rails.env) unless Rails.env.production? end diff --git a/app/models/concerns/stats_cacheable.rb b/app/models/concerns/stats_cacheable.rb new file mode 100644 index 000000000..d7364111f --- /dev/null +++ b/app/models/concerns/stats_cacheable.rb @@ -0,0 +1,24 @@ +module StatsCacheable + extend ActiveSupport::Concern + + module ClassMethods + def stats_cache_key + raise 'Stats cache key has not been set.' + end + + def fetch_stats + raise 'Not implemented.' + end + + # Could be configurable, multisite need to support it. + def recalculate_stats_interval + 30 # minutes + end + + def fetch_cached_stats + # The scheduled Stats job is responsible for generating and caching this. + stats = $redis.get(stats_cache_key) + stats ? JSON.parse(stats) : nil + end + end +end diff --git a/app/serializers/about_serializer.rb b/app/serializers/about_serializer.rb index df8eebc17..7111c6ac4 100644 --- a/app/serializers/about_serializer.rb +++ b/app/serializers/about_serializer.rb @@ -8,4 +8,8 @@ class AboutSerializer < ApplicationSerializer :locale, :version, :https + + def stats + object.class.fetch_cached_stats || Jobs::AboutStats.new.execute({}) + end end diff --git a/spec/jobs/about_stats_spec.rb b/spec/jobs/about_stats_spec.rb new file mode 100644 index 000000000..68e6393e2 --- /dev/null +++ b/spec/jobs/about_stats_spec.rb @@ -0,0 +1,10 @@ +require 'spec_helper' + +describe Jobs::AboutStats do + it 'caches the stats' do + stats = { "visited" => 10 } + About.any_instance.expects(:stats).returns(stats) + $redis.expects(:setex).with(About.stats_cache_key, 35.minutes, stats.to_json) + expect(described_class.new.execute({})).to eq(stats) + end +end diff --git a/spec/jobs/dashboard_stats_spec.rb b/spec/jobs/dashboard_stats_spec.rb new file mode 100644 index 000000000..79f986557 --- /dev/null +++ b/spec/jobs/dashboard_stats_spec.rb @@ -0,0 +1,10 @@ +require 'spec_helper' + +describe Jobs::DashboardStats do + it 'caches the stats' do + json = { "visited" => 10 } + AdminDashboardData.any_instance.expects(:as_json).returns(json) + $redis.expects(:setex).with(AdminDashboardData.stats_cache_key, 35.minutes, json.to_json) + expect(described_class.new.execute({})).to eq(json) + end +end diff --git a/spec/models/about_spec.rb b/spec/models/about_spec.rb new file mode 100644 index 000000000..3a217eabe --- /dev/null +++ b/spec/models/about_spec.rb @@ -0,0 +1,9 @@ +require 'spec_helper' + +describe About do + + describe 'stats cache' do + include_examples 'stats cachable' + end + +end diff --git a/spec/models/admin_dashboard_data_spec.rb b/spec/models/admin_dashboard_data_spec.rb index 0845d9313..30dd8ec2f 100644 --- a/spec/models/admin_dashboard_data_spec.rb +++ b/spec/models/admin_dashboard_data_spec.rb @@ -244,4 +244,8 @@ describe AdminDashboardData do end end + describe 'stats cache' do + include_examples 'stats cachable' + end + end diff --git a/spec/support/shared_examples_for_stats_cacheable.rb b/spec/support/shared_examples_for_stats_cacheable.rb new file mode 100644 index 000000000..d47877c58 --- /dev/null +++ b/spec/support/shared_examples_for_stats_cacheable.rb @@ -0,0 +1,23 @@ +shared_examples_for 'stats cachable' do + describe 'fetch_cached_stats' do + it 'returns the cached stats' do + begin + stats = { "visits" => 10 } + $redis.set(described_class.stats_cache_key, stats.to_json) + expect(described_class.fetch_cached_stats).to eq(stats) + ensure + $redis.del(described_class.stats_cache_key) + end + end + + it 'returns nil if no stats has been cached' do + expect(described_class.fetch_cached_stats).to eq(nil) + end + end + + describe 'fetch_stats' do + it 'has been implemented' do + expect{ described_class.fetch_stats }.to_not raise_error + end + end +end