Skip to content

Save some allocations in ActionView digest#40256

Merged
kaspth merged 1 commit intorails:masterfrom
vinistock:save_allocations_in_digest
Dec 18, 2020
Merged

Save some allocations in ActionView digest#40256
kaspth merged 1 commit intorails:masterfrom
vinistock:save_allocations_in_digest

Conversation

@vinistock
Copy link
Contributor

Summary

This PR saves some allocations in ActionView::Digestor.digest. It's just a minor change in how we build the string name.format.dependencies.

Benchmark

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", require: "rails/all"
  gem "benchmark-ips"
  gem "benchmark-memory", require: "benchmark/memory"
end

module ActionView
  class Digestor
    class << self
      def fast_digest(name:, format: nil, finder:, dependencies: nil)
        if dependencies.nil? || dependencies.empty?
          cache_key = "#{name}.#{format}"
        else
          dependencies_suffix = dependencies.flatten.tap(&:compact!).join(".")
          cache_key = "#{name}.#{format}.#{dependencies_suffix}"
        end

        finder.digest_cache[cache_key] || @@digest_mutex.synchronize do
          finder.digest_cache.fetch(cache_key) do
            root = tree(name, finder, name.include?("/_"))

            dependencies.each do |injected_dep|
              root.children << Injected.new(injected_dep, nil, nil)
            end if dependencies

            finder.digest_cache[cache_key] = root.digest(finder)
          end
        end
      end
    end
  end
end

finder = ActionView::LookupContext.new([])
dependencies = %w[fridge phone tv microwave]

Benchmark.ips do |x|
  x.report("digest")      { ActionView::Digestor.digest(name: "comments/_comment", format: :html, finder: finder, dependencies: dependencies) }
  x.report("fast_digest") { ActionView::Digestor.fast_digest(name: "comments/_comment", format: :html, finder: finder, dependencies: dependencies) }
  x.compare!
end

Benchmark.memory do |x|
  x.report("digest")      { ActionView::Digestor.digest(name: "comments/_comment", format: :html, finder: finder, dependencies: dependencies) }
  x.report("fast_digest") { ActionView::Digestor.fast_digest(name: "comments/_comment", format: :html, finder: finder, dependencies: dependencies) }
  x.compare!
end

Results

Warming up --------------------------------------
              digest    36.926k i/100ms
         fast_digest    58.292k i/100ms
Calculating -------------------------------------
              digest    420.332k (±22.6%) i/s -      1.994M in   5.066665s
         fast_digest    639.586k (± 5.5%) i/s -      3.206M in   5.026402s

Comparison:
         fast_digest:   639586.1 i/s
              digest:   420331.7 i/s - 1.52x  (± 0.00) slower

Calculating -------------------------------------
              digest   482.000  memsize (     0.000  retained)
                         5.000  objects (     0.000  retained)
                         2.000  strings (     0.000  retained)
         fast_digest   283.000  memsize (     0.000  retained)
                         4.000  objects (     0.000  retained)
                         3.000  strings (     0.000  retained)

Comparison:
         fast_digest:        283 allocated
              digest:        482 allocated - 1.70x more

@rails-bot rails-bot bot added the actionview label Sep 19, 2020
@rails-bot
Copy link

rails-bot bot commented Dec 18, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 18, 2020
@kaspth kaspth merged commit 3d46ca9 into rails:master Dec 18, 2020
@kaspth
Copy link
Contributor

kaspth commented Dec 18, 2020

Seems ok since there'll be many templates with dependencies in a typical Rails app. Thanks!

@vinistock vinistock deleted the save_allocations_in_digest branch December 18, 2020 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants