PERF: Eager load Theme associations in Stylesheet Manager.

PERF: Eager load Theme associations in Stylesheet Manager.

Before this change, calling StyleSheet::Manager.stylesheet_details for the first time resulted in multiple queries to the database. This is because the code was modelled in a way where each Theme was loaded from the database one at a time.

This PR restructures the code such that it allows us to load all the theme records in a single query. It also allows us to eager load the required associations upfront. In order to achieve this, I removed the support of loading multiple themes per request. It was initially added to support user selectable theme components but the feature was never completed and abandoned because it wasn’t a feature that we thought was worth building.

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 7ec3091..e7fcbdc 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -10,7 +10,7 @@ class ApplicationController < ActionController::Base
   include Hijack
   include ReadOnlyHeader
 
-  attr_reader :theme_ids
+  attr_reader :theme_id
 
   serialization_scope :guardian
 
@@ -448,35 +448,34 @@ class ApplicationController < ActionController::Base
     resolve_safe_mode
     return if request.env[NO_CUSTOM]
 
-    theme_ids = []
+    theme_id = nil
 
-    if preview_theme_id = request[:preview_theme_id]&.to_i
-      ids = [preview_theme_id]
-      theme_ids = ids if guardian.allow_themes?(ids, include_preview: true)
+    if (preview_theme_id = request[:preview_theme_id]&.to_i) &&
+      guardian.allow_themes?([preview_theme_id], include_preview: true)
+
+      theme_id = preview_theme_id
     end
 
     user_option = current_user&.user_option
 
-    if theme_ids.blank?
+    if theme_id.blank?
       ids, seq = cookies[:theme_ids]&.split("|")
-      ids = ids&.split(",")&.map(&:to_i)
-      if ids.present? && seq && seq.to_i == user_option&.theme_key_seq.to_i
-        theme_ids = ids if guardian.allow_themes?(ids)
+      id = ids&.split(",")&.map(&:to_i)&.first
+      if id.present? && seq && seq.to_i == user_option&.theme_key_seq.to_i
+        theme_id = id if guardian.allow_themes?([id])
       end
     end
 
-    if theme_ids.blank?
+    if theme_id.blank?
       ids = user_option&.theme_ids || []
-      theme_ids = ids if guardian.allow_themes?(ids)
+      theme_id = ids.first if guardian.allow_themes?(ids)
     end
 
-    if theme_ids.blank? && SiteSetting.default_theme_id != -1
-      if guardian.allow_themes?([SiteSetting.default_theme_id])
-        theme_ids << SiteSetting.default_theme_id
-      end
+    if theme_id.blank? && SiteSetting.default_theme_id != -1 && guardian.allow_themes?([SiteSetting.default_theme_id])
+      theme_id = SiteSetting.default_theme_id
     end
 
-    @theme_ids = request.env[:resolved_theme_ids] = theme_ids
+    @theme_id = request.env[:resolved_theme_id] = theme_id
   end
 
   def guardian
@@ -635,10 +634,10 @@ class ApplicationController < ActionController::Base
     target = view_context.mobile_view? ? :mobile : :desktop
 
     data =
-      if @theme_ids.present?
+      if @theme_id.present?
         {
-         top: Theme.lookup_field(@theme_ids, target, "after_header"),
-         footer: Theme.lookup_field(@theme_ids, target, "footer")
+         top: Theme.lookup_field(@theme_id, target, "after_header"),
+         footer: Theme.lookup_field(@theme_id, target, "footer")
         }
       else
         {}
@@ -943,9 +942,9 @@ class ApplicationController < ActionController::Base
   end
 
   def activated_themes_json
-    ids = @theme_ids&.compact
-    return "{}" if ids.blank?
-    ids = Theme.transform_ids(ids)
+    id = @theme_id
+    return "{}" if id.blank?
+    ids = Theme.transform_ids(id)
     Theme.where(id: ids).pluck(:id, :name).to_h.to_json
   end
 end
diff --git a/app/controllers/bootstrap_controller.rb b/app/controllers/bootstrap_controller.rb
index 0921e0a..5bb4dc6 100644
--- a/app/controllers/bootstrap_controller.rb
+++ b/app/controllers/bootstrap_controller.rb
@@ -34,7 +34,7 @@ class BootstrapController < ApplicationController
     ).each do |file|
       add_style(file, plugin: true)
     end
-    add_style(mobile_view? ? :mobile_theme : :desktop_theme) if theme_ids.present?
+    add_style(mobile_view? ? :mobile_theme : :desktop_theme) if theme_id.present?
 
     extra_locales = []
     if ExtraLocalesController.client_overrides_exist?
@@ -51,7 +51,7 @@ class BootstrapController < ApplicationController
     ).map { |f| script_asset_path(f) }
 
     bootstrap = {
-      theme_ids: theme_ids,
+      theme_ids: [theme_id],
       title: SiteSetting.title,
       current_homepage: current_homepage,
       locale_script: locale,
@@ -75,15 +75,14 @@ class BootstrapController < ApplicationController
 private
   def add_scheme(scheme_id, media)
     return if scheme_id.to_i == -1
-    theme_id = theme_ids&.first
 
-    if style = Stylesheet::Manager.color_scheme_stylesheet_details(scheme_id, media, theme_id)
+    if style = Stylesheet::Manager.new(theme_id: theme_id).color_scheme_stylesheet_details(scheme_id, media)
       @stylesheets << { href: style[:new_href], media: media }
     end
   end
 
   def add_style(target, opts = nil)
-    if styles = Stylesheet::Manager.stylesheet_details(target, 'all', theme_ids)
+    if styles = Stylesheet::Manager.new(theme_id: theme_id).stylesheet_details(target, 'all')
       styles.each do |style|
         @stylesheets << {
           href: style[:new_href],
@@ -117,11 +116,11 @@ private
 
     theme_view = mobile_view? ? :mobile : :desktop
 
-    add_if_present(theme_html, :body_tag, Theme.lookup_field(theme_ids, theme_view, 'body_tag'))
-    add_if_present(theme_html, :head_tag, Theme.lookup_field(theme_ids, theme_view, 'head_tag'))
-    add_if_present(theme_html, :header, Theme.lookup_field(theme_ids, theme_view, 'header'))
-    add_if_present(theme_html, :translations, Theme.lookup_field(theme_ids, :translations, I18n.locale))
-    add_if_present(theme_html, :js, Theme.lookup_field(theme_ids, :extra_js, nil))
+    add_if_present(theme_html, :body_tag, Theme.lookup_field(theme_id, theme_view, 'body_tag'))
+    add_if_present(theme_html, :head_tag, Theme.lookup_field(theme_id, theme_view, 'head_tag'))
+    add_if_present(theme_html, :header, Theme.lookup_field(theme_id, theme_view, 'header'))
+    add_if_present(theme_html, :translations, Theme.lookup_field(theme_id, :translations, I18n.locale))
+    add_if_present(theme_html, :js, Theme.lookup_field(theme_id, :extra_js, nil))
 
     theme_html
   end
diff --git a/app/controllers/qunit_controller.rb b/app/controllers/qunit_controller.rb
index 8bb951b..98f7d70 100644
--- a/app/controllers/qunit_controller.rb
+++ b/app/controllers/qunit_controller.rb
@@ -43,7 +43,7 @@ class QunitController < ApplicationController
       return
     end
 
-    request.env[:resolved_theme_ids] = [theme.id]
+    request.env[:resolved_theme_id] = theme.id
     request.env[:skip_theme_ids_transformation] = true
   end
 
diff --git a/app/controllers/stylesheets_controller.rb b/app/controllers/stylesheets_controller.rb
index d944688..427f2b9 100644
--- a/app/controllers/stylesheets_controller.rb
+++ b/app/controllers/stylesheets_controller.rb
@@ -19,7 +19,8 @@ class StylesheetsController < ApplicationController
     params.require("id")
     params.permit("theme_id")
 
-    stylesheet = Stylesheet::Manager.color_scheme_stylesheet_details(params[:id], 'all', params[:theme_id])
+    manager = Stylesheet::Manager.new(theme_id: params[:theme_id])
+    stylesheet = manager.color_scheme_stylesheet_details(params[:id], 'all')
     render json: stylesheet
   end
   protected
@@ -40,16 +41,19 @@ class StylesheetsController < ApplicationController
       # we hold off re-compilation till someone asks for asset
       if target.include?("color_definitions")
         split_target, color_scheme_id = target.split(/_(-?[0-9]+)/)
-        Stylesheet::Manager.color_scheme_stylesheet_link_tag(color_scheme_id)
+
+        Stylesheet::Manager.new.color_scheme_stylesheet_link_tag(color_scheme_id)
       else
-        if target.include?("theme")
-          split_target, theme_id = target.split(/_(-?[0-9]+)/)
-          theme = Theme.find_by(id: theme_id) if theme_id.present?
-        else
-          split_target, color_scheme_id = target.split(/_(-?[0-9]+)/)
-          theme = Theme.find_by(color_scheme_id: color_scheme_id)
-        end
-        Stylesheet::Manager.stylesheet_link_tag(split_target, nil, theme&.id)
+        theme_id =
+          if target.include?("theme")
+            split_target, theme_id = target.split(/_(-?[0-9]+)/)

[... diff too long, it was truncated ...]

GitHub sha: 8e3691d5370bb95d99fe750f46287763721fcc9c

This commit appears in #13397 which was approved by OsamaSayegh and davidtaylorhq. It was merged by tgxworld.