From 1fd8f6df5fc91819d3b8ba65393757caa5c0dd54 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Tue, 27 Apr 2021 14:33:43 +0100
Subject: [PATCH] PERF: Improve theme stylesheet compilation performance
 (#12850)

When building the `scss_load_paths`, we were creating a full export of the theme (including uploads), and not cleaning it up. With many uploads, this can be extremely slow (because it downloads every upload from S3), and the lack of cleanup could cause a disk to fill up over time.

This commit updates the ZipExporter to provide a `with_export_dir` API, which takes care of cleanup. It also adds a kwarg which allows exporting only extra_scss fields. This should make things much faster for themes with many uploads.
---
 app/models/theme.rb             |  9 +++++----
 app/models/theme_field.rb       | 12 +++++++-----
 lib/stylesheet/manager.rb       | 12 ++++++++++--
 lib/theme_store/zip_exporter.rb | 28 ++++++++++++++++++----------
 4 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/app/models/theme.rb b/app/models/theme.rb
index 3c5ce549344..1f208336753 100644
--- a/app/models/theme.rb
+++ b/app/models/theme.rb
@@ -601,11 +601,12 @@ class Theme < ActiveRecord::Base
     find_disable_action_log&.created_at
   end
 
-  def scss_load_paths
-    return if self.extra_scss_fields.empty?
+  def with_scss_load_paths
+    return yield([]) if self.extra_scss_fields.empty?
 
-    @exporter ||= ThemeStore::ZipExporter.new(self)
-    ["#{@exporter.export_dir}/scss", "#{@exporter.export_dir}/stylesheets"]
+    ThemeStore::ZipExporter.new(self).with_export_dir(extra_scss_only: true) do |dir|
+      yield ["#{dir}/stylesheets"]
+    end
   end
 
   def scss_variables
diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb
index ab3d96d263a..9ed1e64cff8 100644
--- a/app/models/theme_field.rb
+++ b/app/models/theme_field.rb
@@ -375,11 +375,13 @@ class ThemeField < ActiveRecord::Base
   def compile_scss(prepended_scss = nil)
     prepended_scss ||= Stylesheet::Importer.new({}).prepended_scss
 
-    Stylesheet::Compiler.compile("#{prepended_scss} #{self.theme.scss_variables.to_s} #{self.value}",
-      "#{Theme.targets[self.target_id]}.scss",
-      theme: self.theme,
-      load_paths: self.theme.scss_load_paths
-    )
+    self.theme.with_scss_load_paths do |load_paths|
+      Stylesheet::Compiler.compile("#{prepended_scss} #{self.theme.scss_variables.to_s} #{self.value}",
+        "#{Theme.targets[self.target_id]}.scss",
+        theme: self.theme,
+        load_paths: load_paths
+      )
+    end
   end
 
   def compiled_css(prepended_scss)
diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb
index 6a864178b52..2d92ed8c5e2 100644
--- a/lib/stylesheet/manager.rb
+++ b/lib/stylesheet/manager.rb
@@ -246,7 +246,7 @@ class Stylesheet::Manager
     end
 
     rtl = @target.to_s =~ /_rtl$/
-    css, source_map = begin
+    css, source_map = with_load_paths do |load_paths|
       Stylesheet::Compiler.compile_asset(
         @target,
          rtl: rtl,
@@ -254,7 +254,7 @@ class Stylesheet::Manager
          theme_variables: theme&.scss_variables.to_s,
          source_map_file: source_map_filename,
          color_scheme_id: @color_scheme&.id,
-         load_paths: theme&.scss_load_paths
+         load_paths: load_paths
       )
     rescue SassC::SyntaxError => e
       if Stylesheet::Importer::THEME_TARGETS.include?(@target.to_s)
@@ -373,6 +373,14 @@ class Stylesheet::Manager
     @theme == :nil ? nil : @theme
   end
 
+  def with_load_paths
+    if theme
+      theme.with_scss_load_paths { |p| yield p }
+    else
+      yield nil
+    end
+  end
+
   def theme_digest
     if [:mobile_theme, :desktop_theme].include?(@target)
       scss_digest = theme.resolve_baked_field(:common, :scss)
diff --git a/lib/theme_store/zip_exporter.rb b/lib/theme_store/zip_exporter.rb
index 3cc4a94b937..edc8639f8a5 100644
--- a/lib/theme_store/zip_exporter.rb
+++ b/lib/theme_store/zip_exporter.rb
@@ -25,11 +25,22 @@ class ThemeStore::ZipExporter
     FileUtils.rm_rf(@temp_folder)
   end
 
-  def export_to_folder
+  def with_export_dir(**kwargs)
+    export_to_folder(**kwargs)
+
+    yield File.join(@temp_folder, @export_name)
+  ensure
+    cleanup!
+  end
+
+  private
+
+  def export_to_folder(extra_scss_only: false)
     destination_folder = File.join(@temp_folder, @export_name)
     FileUtils.mkdir_p(destination_folder)
 
     @theme.theme_fields.each do |field|
+      next if extra_scss_only && !field.extra_scss_field?
       next unless path = field.file_path
 
       # Belt and braces approach here. All the user input should already be
@@ -50,19 +61,16 @@ class ThemeStore::ZipExporter
       File.write(path, content)
     end
 
-    File.write(File.join(destination_folder, "about.json"), JSON.pretty_generate(@theme.generate_metadata_hash))
+    if !extra_scss_only
+      File.write(
+        File.join(destination_folder, "about.json"),
+        JSON.pretty_generate(@theme.generate_metadata_hash)
+      )
+    end
 
     @temp_folder
   end
 
-  def export_dir
-    export_to_folder
-
-    File.join(@temp_folder, @export_name)
-  end
-
-  private
-
   def export_package
     export_to_folder