Mange / roadie-rails

Making HTML emails comfortable for the Rails rockstars
MIT License
363 stars 65 forks source link

Performance issue when inlining style #75

Open PikachuEXE opened 7 years ago

PikachuEXE commented 7 years ago

We have email jobs running a long time (for even 1 email) We are monitoring the time spent with NewRelic Is there anyway to find out more about what's going on in it?

Screenshots

time_spent

screen shot 2017-07-01 at 2 59 27 pm

Code

Our integration code with trace:

# frozen_string_literal: true

require "rails"
require "action_controller"
require "contracts"
require "memoist"
require "roadie"
require "roadie-rails"

require "new_relic/agent/method_tracer"

module Shared::MailerMixins
  module WithRoadieIntegration
    # I don't want to include the constants into the class as well
    module Concern
      def self.included(base)
        base.extend ClassMethods
      end

      include ::NewRelic::Agent::MethodTracer

      def mail(*args, &block)
        super.tap do |m|
          options = roadie_options
          next unless options

          trace_execution_scoped(
            [
              [
                "WithRoadieIntegration",
                "Roadie::Rails::MailInliner.new(m, options).execute",
              ].join("/"),
            ],
          ) do
            Roadie::Rails::MailInliner.new(m, options).execute
          end
        end
      end

      private

      def roadie_options
        ::Rails.application.config.roadie.tap do |options|
          options.asset_providers = [UserAssetsProvider.new]
          options.external_asset_providers = [UserAssetsProvider.new]
          options.keep_uninlinable_css = false
          options.url_options = url_options.slice(*[
            :host,
            :port,
            :path,
            :protocol,
            :scheme,
          ])
        end
      end
      add_method_tracer(
        :roadie_options,
        "WithRoadieIntegration/roadie_options",
      )
    end

    class UserAssetsProvider
      extend(
        ::Memoist,
      )
      include(
        ::Contracts::Core,
        ::Contracts::Builtin,
      )

      include ::NewRelic::Agent::MethodTracer

      ABSOLUTE_ASSET_PATH_REGEXP = /\A#{Regexp.escape("//")}.+#{Regexp.escape("/assets/")}/i

      Contract String => Maybe[Roadie::Stylesheet]
      def find_stylesheet(name)
        return nil unless file_exists?(name)

        Roadie::Stylesheet.new("whatever", stylesheet_content(name))
      end
      add_method_tracer(
        :find_stylesheet,
        "UserAssetsProvider/find_stylesheet",
      )

      Contract String => Roadie::Stylesheet
      def find_stylesheet!(name)
        stylesheet = find_stylesheet(name)

        if stylesheet.nil?
          raise Roadie::CssNotFound.new(
            name,
            "does not exists",
            self,
          )
        end

        stylesheet
      end
      add_method_tracer(
        :find_stylesheet!,
        "UserAssetsProvider/find_stylesheet!",
      )

      private

      def file_exists?(name)
        if assets_precompiled?
          File.exists?(local_file_path(name))
        else
          sprockets_asset(name)
        end
      end
      memoize :file_exists?

      # If on-the-fly asset compilation is disabled, we must be precompiling assets.
      def assets_precompiled?
        !Rails.configuration.assets.compile
      rescue
        false
      end

      def local_file_path(name)
        asset_path = asset_path(name)
        if asset_path.match(ABSOLUTE_ASSET_PATH_REGEXP)
          asset_path.gsub!(ABSOLUTE_ASSET_PATH_REGEXP, "assets/")
        end

        File.join(Rails.public_path, asset_path)
      end
      memoize :local_file_path
      add_method_tracer(
        :local_file_path,
        "UserAssetsProvider/local_file_path",
      )

      def sprockets_asset(name)
        asset_path = asset_path(name)
        if asset_path.match(ABSOLUTE_ASSET_PATH_REGEXP)
          asset_path.gsub!(ABSOLUTE_ASSET_PATH_REGEXP, "")
        end

        # Strange thing is since rails 4.2
        # name is passed in like
        # `/assets/mailer-a9c96bd713d0b091297b82053ccd9155b933c00a53595812d755825d1747f42d.css`
        # Before any processing
        # And since `sprockets_asset` is used for preview
        # We just "fix" the name by removing the
        #
        # Regexp taken from gem `asset_sync`
        # https://github.com/AssetSync/asset_sync/blob/v1.2.1/lib/asset_sync/storage.rb#L142
        #
        # Modified to match what we need here (we need `.css` suffix)
        if asset_path =~ /-[0-9a-fA-F]{32,}\.css$/
          asset_path.gsub!(/-[0-9a-fA-F]{32,}\.css$/, ".css")
        end

        Rails.application.assets.find_asset(asset_path)
      end
      add_method_tracer(
        :sprockets_asset,
        "UserAssetsProvider/sprockets_asset",
      )

      def asset_path(name)
        name.gsub(%r{^[/]?assets/}, "")
      end

      Contract String => String
      def stylesheet_content(name)
        if assets_precompiled?
          File.read(local_file_path(name))
        else
          # This will compile and return the asset
          sprockets_asset(name).to_s
        end.strip
      end
      memoize :stylesheet_content
      add_method_tracer(
        :stylesheet_content,
        "UserAssetsProvider/stylesheet_content",
      )
    end
  end
end
Mange commented 7 years ago

Wow, yeah that is really bad. I have no good options for you, I'm afraid. Here's my least worst picks for now:

  1. Try to create this issue locally and run with a profiler. Rubyprof was pretty good last time I debugged something like this. It'll allow you to save a HTML report where you can see which methods took the longest time and how many times they were called and from where, etc. It could help you.
  2. Fork the gem and add some tracing stuff to it, like the NewRelic method tracers you use in the code above. Then ship it and collect the data through NewRelic in production.

I also encourage you to time all emails and log them somewhere to see if it's all emails of this type or if it is outliers. If it's outliers you might be able to log the input HTML somewhere to make it easier to replicate in development.

If you are able to identify where the problem comes from, I'd be interested in hearing it.

I apologize for not being able to help you more with this.

Mange commented 7 years ago

I looked at the consume_stylesheets method. It goes through all stylesheet rules and searches the whole document for nodes matching them. If the number of selectors grow so will the runtime of this method. Same with larger documents.

All in all with M number of selectors and N number of elements in the tree, the searcher will look at M×N elements. This grows quickly if they get large.

PikachuEXE commented 7 years ago

We removed the unused styles and the time consumed in consume_stylesheets has gone down. Are there other methods to reduce the time?

Mange commented 7 years ago

Not that I can think of right now. I could help more with some profiler output.