From f1d8cd529e173a098847b8ce7afe3ed8f7c23ccd Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 31 Aug 2023 21:12:03 +0200 Subject: [PATCH] Revert "Revert "PERF: Cache each theme field value once (#23192)" (#23354)" (#23356) This reverts commit 9821ca9413d2694fe0ff856fe0a307a27fb2dc0a. --- app/models/theme.rb | 83 +++++++++++++++++++++++-------- lib/distributed_cache.rb | 18 +++++++ spec/models/theme_spec.rb | 100 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+), 21 deletions(-) diff --git a/app/models/theme.rb b/app/models/theme.rb index 347a3a609fb..23cb45fedf8 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -350,12 +350,7 @@ class Theme < ActiveRecord::Base return "" if theme_id.blank? theme_ids = !skip_transformation ? transform_ids(theme_id) : [theme_id] - cache_key = "#{theme_ids.join(",")}:#{target}:#{field}:#{Theme.compiler_version}" - - get_set_cache(cache_key) do - target = target.to_sym - resolve_baked_field(theme_ids, target, field) || "" - end.html_safe + (resolve_baked_field(theme_ids, target.to_sym, field) || "").html_safe end def self.lookup_modifier(theme_ids, modifier_name) @@ -434,25 +429,71 @@ class Theme < ActiveRecord::Base end def self.resolve_baked_field(theme_ids, target, name) - if target == :extra_js - require_rebake = - ThemeField.where(theme_id: theme_ids, target_id: Theme.targets[:extra_js]).where( - "compiler_version <> ?", - Theme.compiler_version, - ) - require_rebake.each { |tf| tf.ensure_baked! } - require_rebake - .map(&:theme_id) - .uniq - .each { |theme_id| Theme.find(theme_id).update_javascript_cache! } - caches = JavascriptCache.where(theme_id: theme_ids) - caches = caches.sort_by { |cache| theme_ids.index(cache.theme_id) } - return caches.map { |c| <<~HTML.html_safe }.join("\n") + target = target.to_sym + name = name&.to_sym + + target = :mobile if target == :mobile_theme + target = :desktop if target == :desktop_theme + + case target + when :extra_js + get_set_cache("#{theme_ids.join(",")}:extra_js:#{Theme.compiler_version}") do + require_rebake = + ThemeField.where(theme_id: theme_ids, target_id: Theme.targets[:extra_js]).where( + "compiler_version <> ?", + Theme.compiler_version, + ) + + ActiveRecord::Base.transaction do + require_rebake.each { |tf| tf.ensure_baked! } + + Theme.where(id: require_rebake.map(&:theme_id)).each(&:update_javascript_cache!) + end + + caches = + JavascriptCache + .where(theme_id: theme_ids) + .index_by(&:theme_id) + .values_at(*theme_ids) + .compact + + caches.map { |c| <<~HTML.html_safe }.join("\n") HTML + end + when :translations + theme_field_values(theme_ids, :translations, I18n.fallbacks[name]) + .to_a + .select(&:second) + .uniq { |((theme_id, _, _), _)| theme_id } + .flat_map(&:second) + .join("\n") + else + theme_field_values(theme_ids, [:common, target], name).values.compact.flatten.join("\n") + end + end + + def self.theme_field_values(theme_ids, targets, names) + cache.defer_get_set_bulk( + Array(theme_ids).product(Array(targets), Array(names)), + lambda do |(theme_id, target, name)| + "#{theme_id}:#{target}:#{name}:#{Theme.compiler_version}" + end, + ) do |keys| + keys = keys.map { |theme_id, target, name| [theme_id, Theme.targets[target], name.to_s] } + + keys + .map do |theme_id, target_id, name| + ThemeField.where(theme_id: theme_id, target_id: target_id, name: name) + end + .inject { |a, b| a.or(b) } + .each(&:ensure_baked!) + .map { |tf| [[tf.theme_id, tf.target_id, tf.name], tf.value_baked || tf.value] } + .group_by(&:first) + .transform_values { |x| x.map(&:second) } + .values_at(*keys) end - list_baked_fields(theme_ids, target, name).map { |f| f.value_baked || f.value }.join("\n") end def self.list_baked_fields(theme_ids, target, name) diff --git a/lib/distributed_cache.rb b/lib/distributed_cache.rb index b041b39d1d4..bc710f26564 100644 --- a/lib/distributed_cache.rb +++ b/lib/distributed_cache.rb @@ -20,6 +20,24 @@ class DistributedCache < MessageBus::DistributedCache value end + def defer_get_set_bulk(ks, key_blk, &blk) + found_keys, missing_keys = ks.partition { |k| hash.key?(key_blk.call(k)) } + found_hash = found_keys.map { |key| [key, self[key_blk.call(key)]] }.to_h + + if missing_keys.present? + missing_values = blk.call(missing_keys.freeze) + missing_hash = missing_keys.zip(missing_values).to_h + + Scheduler::Defer.later("#{@key}_bulk_set") do + missing_hash.each { |key, value| self[key_blk.call(key)] = value } + end + + ks.zip(missing_hash.merge(found_hash).values_at(*ks)).to_h + else + found_hash + end + end + def clear(after_commit: true) if after_commit && !GlobalSetting.skip_db? DB.after_commit { super() } diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index f34d7ac0d9a..6ffa42b4fcf 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -1119,4 +1119,104 @@ HTML expect(messages).to include(["development-mode-theme-changed"]) end end + + describe "#lookup_field when a theme component is used in multiple themes" do + fab!(:theme_1) { Fabricate(:theme, user: user) } + fab!(:theme_2) { Fabricate(:theme, user: user) } + fab!(:child) { Fabricate(:theme, user: user, component: true) } + + before_all do + theme_1.add_relative_theme!(:child, child) + theme_2.add_relative_theme!(:child, child) + end + + it "efficiently caches fields of theme component by only caching the fields once across multiple themes" do + child.set_field(target: :common, name: "header", value: "World") + child.save! + + expect(Theme.lookup_field(theme_1.id, :desktop, "header")).to eq("World") + expect(Theme.lookup_field(theme_2.id, :desktop, "header")).to eq("World") + + expect( + Theme.cache.defer_get_set("#{child.id}:common:header:#{Theme.compiler_version}") { raise }, + ).to eq(["World"]) + expect( + Theme.cache.defer_get_set("#{child.id}:desktop:header:#{Theme.compiler_version}") { raise }, + ).to eq(nil) + + expect( + Theme + .cache + .defer_get_set("#{theme_1.id}:common:header:#{Theme.compiler_version}") { raise }, + ).to eq(nil) + expect( + Theme + .cache + .defer_get_set("#{theme_1.id}:desktop:header:#{Theme.compiler_version}") { raise }, + ).to eq(nil) + + expect( + Theme + .cache + .defer_get_set("#{theme_2.id}:common:header:#{Theme.compiler_version}") { raise }, + ).to eq(nil) + expect( + Theme + .cache + .defer_get_set("#{theme_2.id}:desktop:header:#{Theme.compiler_version}") { raise }, + ).to eq(nil) + end + + it "puts the parent value ahead of the child" do + theme_1.set_field(target: :common, name: "header", value: "theme_1") + theme_1.save! + + child.set_field(target: :common, name: "header", value: "child") + child.save! + + expect(Theme.lookup_field(theme_1.id, :desktop, "header")).to eq("theme_1\nchild") + end + + it "puts parent translations ahead of child translations" do + theme_1.set_field(target: :translations, name: "en", value: <<~YAML) + en: + theme_1: "test" + YAML + theme_1.save! + theme_field = ThemeField.order(:id).last + + child.set_field(target: :translations, name: "en", value: <<~YAML) + en: + child: "test" + YAML + child.save! + child_field = ThemeField.order(:id).last + + expect(theme_field.value_baked).not_to eq(child_field.value_baked) + expect(Theme.lookup_field(theme_1.id, :translations, :en)).to eq( + [theme_field, child_field].map(&:value_baked).join("\n"), + ) + end + + it "prioritizes a locale over its fallback" do + theme_1.set_field(target: :translations, name: "en", value: <<~YAML) + en: + theme_1: "hello" + YAML + theme_1.save! + en_field = ThemeField.order(:id).last + + theme_1.set_field(target: :translations, name: "es", value: <<~YAML) + es: + theme_1: "hola" + YAML + theme_1.save! + es_field = ThemeField.order(:id).last + + expect(es_field.value_baked).not_to eq(en_field.value_baked) + expect(Theme.lookup_field(theme_1.id, :translations, :en)).to eq(en_field.value_baked) + expect(Theme.lookup_field(theme_1.id, :translations, :es)).to eq(es_field.value_baked) + expect(Theme.lookup_field(theme_1.id, :translations, :fr)).to eq(en_field.value_baked) + end + end end