From 88ecb650a98921e5e3cd345e29f84d9dd204b777 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Wed, 27 Nov 2019 16:11:49 +1100 Subject: [PATCH] DEV: Implement a faster Discourse.cache This is a bottom up rewrite of Discourse cache to support faster performance and a limited surface area. ActiveSupport::Cache::Store accepts many options we do not use, this partial implementation only picks the bits out that we do use and want to support. Additionally params are named which avoids typos such as "expires_at" vs "expires_in" This also moves a few spots in Discourse to use Discourse.cache over setex Performance of setex and Discourse.cache.write is similar. --- app/controllers/email_controller.rb | 4 +- app/models/discourse_single_sign_on.rb | 8 +- app/models/report.rb | 2 +- lib/cache.rb | 107 ++++++++++++++++++++----- lib/discourse_tagging.rb | 2 +- script/benchmarks/cache/bench.rb | 14 ++++ spec/components/cache_spec.rb | 11 +++ spec/components/redis_store_spec.rb | 4 + spec/requests/email_controller_spec.rb | 4 +- 9 files changed, 127 insertions(+), 29 deletions(-) diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb index 0e82707e365..5e8b8d63afb 100644 --- a/app/controllers/email_controller.rb +++ b/app/controllers/email_controller.rb @@ -112,7 +112,7 @@ class EmailController < ApplicationController else key = "unsub_#{SecureRandom.hex}" - $redis.setex key, 1.hour, user.email + Discourse.cache.write key, user.email, expires_in: 1.hour url = path("/email/unsubscribed?key=#{key}") if topic @@ -125,7 +125,7 @@ class EmailController < ApplicationController end def unsubscribed - @email = $redis.get(params[:key]) + @email = Discourse.cache.read(params[:key]) @topic_id = params[:topic_id] user = User.find_by_email(@email) raise Discourse::NotFound unless user diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index 6b6e392683f..9d72a9cc981 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/app/models/discourse_single_sign_on.rb @@ -27,21 +27,21 @@ class DiscourseSingleSignOn < SingleSignOn def register_nonce(return_path) if nonce - $redis.setex(nonce_key, SingleSignOn.nonce_expiry_time, return_path) + Discourse.cache.write(nonce_key, return_path, expires_in: SingleSignOn.nonce_expiry_time) end end def nonce_valid? - nonce && $redis.get(nonce_key).present? + nonce && Discourse.cache.read(nonce_key).present? end def return_path - $redis.get(nonce_key) || "/" + Discourse.cache.read(nonce_key) || "/" end def expire_nonce! if nonce - $redis.del nonce_key + Discourse.cache.delete nonce_key end end diff --git a/app/models/report.rb b/app/models/report.rb index 5271e0bef33..7eac9ffd2fc 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -163,7 +163,7 @@ class Report end def self.cache(report, duration) - Discourse.cache.write(cache_key(report), report.as_json, force: true, expires_in: duration) + Discourse.cache.write(cache_key(report), report.as_json, expires_in: duration) end def self.find(type, opts = nil) diff --git a/lib/cache.rb b/lib/cache.rb index f7df05ed6f3..9c0863eba93 100644 --- a/lib/cache.rb +++ b/lib/cache.rb @@ -1,8 +1,20 @@ # frozen_string_literal: true -# Discourse specific cache, enforces 1 day expiry +# Discourse specific cache, enforces 1 day expiry by default -class Cache < ActiveSupport::Cache::Store +# This is a bottom up implementation of ActiveSupport::Cache::Store +# this allows us to cleanly implement without using cache entries and version +# support which we do not use, in tern this makes the cache as fast as simply +# using `$redis.setex` with a more convenient API +# +# It only implements a subset of ActiveSupport::Cache::Store as we make no use +# of large parts of the interface. +# +# An additional advantage of this class is that all methods have named params +# Rails tends to use options hash for lots of stuff due to legacy reasons +# this makes it harder to reason about the API + +class Cache # nothing is cached for longer than 1 day EVER # there is no reason to have data older than this clogging redis @@ -10,9 +22,14 @@ class Cache < ActiveSupport::Cache::Store # pointless data MAX_CACHE_AGE = 1.day unless defined? MAX_CACHE_AGE - def initialize(opts = {}) - @namespace = opts[:namespace] || "_CACHE_" - super(opts) + # we don't need this feature, 1 day expiry is enough + # it makes lookups a tad cheaper + def self.supports_cache_versioning? + false + end + + def initialize(namespace: "_CACHE") + @namespace = namespace end def redis @@ -33,30 +50,82 @@ class Cache < ActiveSupport::Cache::Store end end - def normalize_key(key, opts = nil) + def normalize_key(key) "#{@namespace}:#{key}" end + def exist?(name) + key = normalize_key(name) + redis.exists(key) + end + + # this removes a bunch of stuff we do not need like instrumentation and versioning + def read(name) + key = normalize_key(name) + read_entry(key) + end + + def write(name, value, expires_in: nil) + write_entry(normalize_key(name), value, expires_in: nil) + end + + def delete(name) + redis.del(normalize_key(name)) + end + + def fetch(name, expires_in: nil, force: nil, &blk) + if block_given? + key = normalize_key(name) + raw = nil + + if !force + raw = redis.get(key) + end + + if raw + begin + Marshal.load(raw) + rescue => e + log_first_exception(e) + end + else + val = blk.call + write_entry(key, val, expires_in: expires_in) + val + end + elsif force + raise ArgumentError, "Missing block: Calling `Cache#fetch` with `force: true` requires a block." + else + read(name) + end + end + protected - def read_entry(key, options) - if data = redis.get(key) - data = Marshal.load(data) - ActiveSupport::Cache::Entry.new data + def log_first_exception(e) + if !defined? @logged_a_warning + @logged_a_warning = true + Discourse.warn_exception(e, "Corrupt cache... skipping entry for key #{key}") end - rescue - # corrupt cache, fail silently for now, remove rescue later end - def write_entry(key, entry, options) - dumped = Marshal.dump(entry.value) - expiry = options[:expires_in] || MAX_CACHE_AGE + def read_entry(key) + if data = redis.get(key) + Marshal.load(data) + end + rescue => e + # corrupt cache, this can happen if Marshal version + # changes. Log it once so we can tell it is happening. + # should not happen under any normal circumstances, but we + # do not want to flood logs + log_first_exception(e) + end + + def write_entry(key, value, expires_in: nil) + dumped = Marshal.dump(value) + expiry = expires_in || MAX_CACHE_AGE redis.setex(key, expiry, dumped) true end - def delete_entry(key, options) - redis.del key - end - end diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 4711a8b74b9..5b7996a946b 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -336,7 +336,7 @@ module DiscourseTagging end def self.staff_tag_names - tag_names = Discourse.cache.read(TAGS_STAFF_CACHE_KEY, tag_names) + tag_names = Discourse.cache.read(TAGS_STAFF_CACHE_KEY) if !tag_names tag_names = Tag.joins(tag_groups: :tag_group_permissions) diff --git a/script/benchmarks/cache/bench.rb b/script/benchmarks/cache/bench.rb index 8f16f4bfcf6..572ee5ec1c1 100644 --- a/script/benchmarks/cache/bench.rb +++ b/script/benchmarks/cache/bench.rb @@ -79,3 +79,17 @@ end # redis get string marshal: 12789.2 i/s - same-ish: difference falls within error # Rails read cache string: 10486.4 i/s - 1.25x slower # Discourse read cache string: 10457.1 i/s - 1.26x slower +# +# After Cache re-write +# +# Comparison: +# redis setex string: 13390.9 i/s +# redis setex marshal string: 13202.0 i/s - same-ish: difference falls within error +# Discourse cache string: 12406.5 i/s - same-ish: difference falls within error +# Rails cache string: 12289.2 i/s - same-ish: difference falls within error +# +# Comparison: +# redis get string: 13589.6 i/s +# redis get string marshal: 13118.3 i/s - same-ish: difference falls within error +# Rails read cache string: 12482.2 i/s - same-ish: difference falls within error +# Discourse read cache string: 12296.8 i/s - same-ish: difference falls within error diff --git a/spec/components/cache_spec.rb b/spec/components/cache_spec.rb index 178905a77a3..7b367743cfd 100644 --- a/spec/components/cache_spec.rb +++ b/spec/components/cache_spec.rb @@ -9,6 +9,12 @@ describe Cache do Cache.new end + it "supports exist?" do + cache.write("testing", 1.1) + expect(cache.exist?("testing")).to eq(true) + expect(cache.exist?(SecureRandom.hex)).to eq(false) + end + it "supports float" do cache.write("float", 1.1) expect(cache.read("float")).to eq(1.1) @@ -36,10 +42,14 @@ describe Cache do end it "can delete correctly" do + cache.delete("key") + cache.fetch("key", expires_in: 1.minute) do "test" end + expect(cache.fetch("key")).to eq("test") + cache.delete("key") expect(cache.fetch("key")).to eq(nil) end @@ -69,6 +79,7 @@ describe Cache do r = cache.fetch "key" do "bob" end + expect(r).to eq("bob") end diff --git a/spec/components/redis_store_spec.rb b/spec/components/redis_store_spec.rb index 86f37c0e192..ab7ec0c0c20 100644 --- a/spec/components/redis_store_spec.rb +++ b/spec/components/redis_store_spec.rb @@ -44,6 +44,9 @@ describe "Redis Store" do end it "can be cleared without clearing our cache" do + cache.clear + store.clear + store.fetch "key" do "key in store" end @@ -53,6 +56,7 @@ describe "Redis Store" do end store.clear + expect(store.read("key")).to eq(nil) expect(cache.fetch("key")).to eq("key in cache") diff --git a/spec/requests/email_controller_spec.rb b/spec/requests/email_controller_spec.rb index a5330e39dd2..10658907b63 100644 --- a/spec/requests/email_controller_spec.rb +++ b/spec/requests/email_controller_spec.rb @@ -151,7 +151,7 @@ RSpec.describe EmailController do describe 'when topic is public' do it 'should return the right response' do key = SecureRandom.hex - $redis.set(key, user.email) + Discourse.cache.write(key, user.email) get '/email/unsubscribed', params: { key: key, topic_id: topic.id } expect(response.status).to eq(200) expect(response.body).to include(topic.title) @@ -161,7 +161,7 @@ RSpec.describe EmailController do describe 'when topic is private' do it 'should return the right response' do key = SecureRandom.hex - $redis.set(key, user.email) + Discourse.cache.write(key, user.email) get '/email/unsubscribed', params: { key: key, topic_id: private_topic.id } expect(response.status).to eq(200) expect(response.body).to_not include(private_topic.title)