Optimize mime types validation in ActionView::LookupContext

[casperisfine]

Fix: #48156

The assumption here is that in the overwhelming majority of cases, all formats are valid.

So we first check if any of the formats is invalid before duping the details hash and filtering them.

Additionally, by exposing a (non-public) valid_symbols? method, we can check symbols are valid without resporting to Array#% which would needlessly duplicate the formats array.

This optimization is based on a prior refactor of ActionView::Template::Types to avoid delegation.

The Type class was introduced in #23085 for the sole purpose of breaking the dependency of Action View on Action Dispatch.

Unless you are somehow running Action View standalone, this is actually never used.

So instead of delegating, we can use constant swapping, this saves us a useless layer.

Ultimately we could consider moving Mime::Types into Active Support but it requires some more thoughts.

cc @headius @ghiculescu

Also cc @jhawthorn as I see you've optimized the Template::Types delegation in the past.

I still need to benchmark to see how much we gain here, and if another approach may be faster, but I'd like to first check CI is green.

[casperisfine]

Actually I just figured we could shortcircuit all this mode in the vast majority of cases: 4c6bc6c

Dunno if the rest of the optimizations and refactors are still really worth it, but I kinda like them.

@headius could you share the benchmark you were using? If not no big deal.

[casperisfine]

Benchmark:

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

rails = ENV.fetch("RAILS", "edge")

gemfile(true) do
  source "https://rubygems.org"
  if rails == "edge"
    gem "actionpack", github: "rails/rails"
    gem "actionview", github: "rails/rails"
  elsif rails == "local"
    gem "actionpack", path: "/Users/byroot/src/github.com/Shopify/rails"
    gem "actionview", path: "/Users/byroot/src/github.com/Shopify/rails"
  end
  gem "benchmark-ips"
end

require "action_controller"
require "action_view"

details = {:locale=>[:en], :formats=>[:html], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby]}

Benchmark.ips do |x|
  x.report("details_cache_key") {  ActionView::LookupContext::DetailsKey.details_cache_key(details) }
end

main:

Warming up --------------------------------------
   details_cache_key    86.411k i/100ms
Calculating -------------------------------------
   details_cache_key    871.920k (± 1.3%) i/s -      4.407M in   5.055226s

This branch:

Warming up --------------------------------------
   details_cache_key   143.624k i/100ms
Calculating -------------------------------------
   details_cache_key      1.433M (± 1.4%) i/s -      7.181M in   5.013862s

[casperisfine]

This branch without the last commit:

Warming up --------------------------------------
   details_cache_key   134.728k i/100ms
Calculating -------------------------------------
   details_cache_key      1.339M (± 1.1%) i/s -      6.736M in   5.030104s

So I think the other optimizations are worth it to optimize on miss.

[jhawthorn]

Seems good to me

[jhawthorn]

The Type class was introduced in #23085 for the sole purpose of breaking the dependency of Action View on Action Dispatch.

Unless you are somehow running Action View standalone, this is actually never used.

Yeah, essentially it's only purpose is to avoid a dependency on mime-types from ActionView, which I think could be worth revisiting, but the constant reassignment should at least make it a non-issue for performance.

[casperisfine]

I've profiled a bit, but most of the time is now spent computing the hash code for details, so I don't think there's much left to optimize here (aside from larger architectural changes). I'll merge.