Revert "PERF: Cache each theme field value once (#23192)" (#23354)

This reverts commit 82a56334a3.

app/models/theme.rb

@@ -350,7 +350,12 @@ class Theme < ActiveRecord::Base
     return "" if theme_id.blank?
 
     theme_ids = !skip_transformation ? transform_ids(theme_id) : [theme_id]
-    (resolve_baked_field(theme_ids, target.to_sym, field) || "").html_safe
+    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
   end
 
   def self.lookup_modifier(theme_ids, modifier_name)
@@ -429,71 +434,25 @@ class Theme < ActiveRecord::Base
   end
 
   def self.resolve_baked_field(theme_ids, target, name)
-    target = target.to_sym
+    if target == :extra_js
-    name = name&.to_sym
+      require_rebake =
-
+        ThemeField.where(theme_id: theme_ids, target_id: Theme.targets[:extra_js]).where(
-    target = :mobile if target == :mobile_theme
+          "compiler_version <> ?",
-    target = :desktop if target == :desktop_theme
+          Theme.compiler_version,
-
+        )
-    case target
+      require_rebake.each { |tf| tf.ensure_baked! }
-    when :extra_js
+      require_rebake
-      get_set_cache("#{theme_ids.join(",")}:extra_js:#{Theme.compiler_version}") do
+        .map(&:theme_id)
-        require_rebake =
+        .uniq
-          ThemeField.where(theme_id: theme_ids, target_id: Theme.targets[:extra_js]).where(
+        .each { |theme_id| Theme.find(theme_id).update_javascript_cache! }
-            "compiler_version <> ?",
+      caches = JavascriptCache.where(theme_id: theme_ids)
-            Theme.compiler_version,
+      caches = caches.sort_by { |cache| theme_ids.index(cache.theme_id) }
-          )
+      return caches.map { |c| <<~HTML.html_safe }.join("\n")
-
-        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")
           <link rel="preload" href="#{c.url}" as="script">
           <script defer src='#{c.url}' data-theme-id='#{c.theme_id}'></script>
         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)

lib/distributed_cache.rb

@@ -20,24 +20,6 @@ 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() }

spec/models/theme_spec.rb

@@ -1119,104 +1119,4 @@ 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