From 94a47d037fdd900719b368742e4f4ec393671c05 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 15 Feb 2022 16:55:21 +0000 Subject: [PATCH] PERF: Reduce number of EXPIRE calls from CachedCounting (#15958) Previously we were calling `EXPIRE` every time we incremented a given key. Instead, we can call EXPIRE once when the key is first populated. A LUA script is used to make this as efficient as possible. Consumers of this Concern use daily keys. Since we're now calling EXPIRE only at the beginning of the day, rather than throughout the day, the expire time has been increased from 3 to 4 days. --- app/models/concerns/cached_counting.rb | 29 +++++++++++++++++++------ spec/models/application_request_spec.rb | 6 +++-- spec/models/web_crawler_request_spec.rb | 6 +++-- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/app/models/concerns/cached_counting.rb b/app/models/concerns/cached_counting.rb index dfa5a8ec645..953dc25fd25 100644 --- a/app/models/concerns/cached_counting.rb +++ b/app/models/concerns/cached_counting.rb @@ -3,6 +3,18 @@ module CachedCounting extend ActiveSupport::Concern + EXPIRE_CACHE_AFTER = 4.days.to_i + + LUA_INCR_AND_EXPIRE = DiscourseRedis::EvalHelper.new <<~LUA + local result = redis.call("INCR", KEYS[1]) + + if result == 1 then + redis.call("EXPIRE", KEYS[1], ARGV[1]) + end + + return result + LUA + included do class << self attr_accessor :autoflush, :autoflush_seconds, :last_flush @@ -19,13 +31,16 @@ module CachedCounting class_methods do def perform_increment!(key, opts = nil) - val = Discourse.redis.incr(key).to_i + val = DiscourseRedis.ignore_readonly do + LUA_INCR_AND_EXPIRE.eval( + Discourse.redis.without_namespace, + [Discourse.redis.namespace_key(key)], + [EXPIRE_CACHE_AFTER] + ).to_i + end - # readonly mode it is going to be 0, skip - return if val == 0 - - # 3.days, see: https://github.com/rails/rails/issues/21296 - Discourse.redis.expire(key, 259200) + # readonly mode it is going to be nil, skip + return if val.nil? autoflush = (opts && opts[:autoflush]) || self.autoflush if autoflush > 0 && val >= autoflush @@ -45,7 +60,7 @@ module CachedCounting # this may seem a bit fancy but in so it allows # for concurrent calls without double counting def get_and_reset(key) - Discourse.redis.set(key, '0', ex: 259200, get: true).to_i + Discourse.redis.set(key, '0', ex: EXPIRE_CACHE_AFTER, get: true).to_i end def request_id(query_params, retries = 0) diff --git a/spec/models/application_request_spec.rb b/spec/models/application_request_spec.rb index 2212a1dd8d1..0912785472b 100644 --- a/spec/models/application_request_spec.rb +++ b/spec/models/application_request_spec.rb @@ -29,14 +29,16 @@ describe ApplicationRequest do inc(:http_total) inc(:http_total) - Discourse.redis.without_namespace.stubs(:incr).raises(Redis::CommandError.new("READONLY")) + Discourse.redis.without_namespace.stubs(:eval).raises(Redis::CommandError.new("READONLY")) + Discourse.redis.without_namespace.stubs(:evalsha).raises(Redis::CommandError.new("READONLY")) Discourse.redis.without_namespace.stubs(:set).raises(Redis::CommandError.new("READONLY")) # flush will be deferred no error raised inc(:http_total, autoflush: 3) ApplicationRequest.write_cache! - Discourse.redis.without_namespace.unstub(:incr) + Discourse.redis.without_namespace.unstub(:eval) + Discourse.redis.without_namespace.unstub(:evalsha) Discourse.redis.without_namespace.unstub(:set) inc(:http_total, autoflush: 3) diff --git a/spec/models/web_crawler_request_spec.rb b/spec/models/web_crawler_request_spec.rb index 9bb42b69d8d..7bab60314c9 100644 --- a/spec/models/web_crawler_request_spec.rb +++ b/spec/models/web_crawler_request_spec.rb @@ -31,13 +31,15 @@ describe WebCrawlerRequest do inc('Googlebot') inc('Googlebot') - Discourse.redis.without_namespace.stubs(:incr).raises(Redis::CommandError.new("READONLY")) + Discourse.redis.without_namespace.stubs(:eval).raises(Redis::CommandError.new("READONLY")) + Discourse.redis.without_namespace.stubs(:evalsha).raises(Redis::CommandError.new("READONLY")) Discourse.redis.without_namespace.stubs(:set).raises(Redis::CommandError.new("READONLY")) inc('Googlebot', autoflush: 3) WebCrawlerRequest.write_cache! - Discourse.redis.without_namespace.unstub(:incr) + Discourse.redis.without_namespace.unstub(:eval) + Discourse.redis.without_namespace.unstub(:evalsha) Discourse.redis.without_namespace.unstub(:set) inc('Googlebot', autoflush: 3)