Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up template lookups by avoiding splats and === #42779

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Jul 13, 2021

Method calls involving splats are slower, and this previously did several levels of splats, which was also hard to grok (I'd struggled with this in the past). Though this PR adds a little repetition I think it's a net benefit (I also expect the definitions of find/find_all/exists to become more different in the future).

This also replaces a === check with a call to Array() which is faster and IMO cleaner.

Benchmark
require "benchmark/ips"
require "action_view"
require "action_pack"
require "action_controller"

class TestController < ActionController::Base
end

ActionView::Base.logger = nil

TestController.view_paths = [File.expand_path("actionview/test/fixtures")]
controller_view = TestController.new.view_context
LOOKUP_CONTEXT = TestController.new.lookup_context

result = Benchmark.ips do |x|
  x.config(:time => 5, :warmup => 2)

  x.report("find_all found") do
    # Single candidate
    LOOKUP_CONTEXT.find_all("basic", ["test"], false)
  end

  x.report("find_all missing") do
    LOOKUP_CONTEXT.find_all("no_template_here", ["test"], true)
  end

  x.report("exists missing") do
    LOOKUP_CONTEXT.exists?("no_template_here", ["test"], true)
  end

  x.report("find_all many results") do
    LOOKUP_CONTEXT.find_all("hello_world", ["test"], false)
  end
end

Before

$ be ruby lookup_benchmark.rb
Warming up --------------------------------------
      find_all found    44.100k i/100ms
    find_all missing    45.622k i/100ms
      exists missing    41.121k i/100ms
find_all many results
                        43.849k i/100ms
Calculating -------------------------------------
      find_all found    437.937k (± 0.4%) i/s -      2.205M in   5.035045s
    find_all missing    455.570k (± 0.3%) i/s -      2.281M in   5.007187s
      exists missing    410.674k (± 0.3%) i/s -      2.056M in   5.006581s
find_all many results
                        438.060k (± 0.3%) i/s -      2.192M in   5.004969s

After

$ be ruby lookup_benchmark.rb
Warming up --------------------------------------
      find_all found    47.950k i/100ms
    find_all missing    50.592k i/100ms
      exists missing    50.084k i/100ms
find_all many results
                        48.004k i/100ms
Calculating -------------------------------------
      find_all found    484.917k (± 0.3%) i/s -      2.445M in   5.043091s
    find_all missing    507.769k (± 0.5%) i/s -      2.580M in   5.081569s
      exists missing    502.863k (± 0.6%) i/s -      2.554M in   5.079695s
find_all many results
                        483.612k (± 0.6%) i/s -      2.448M in   5.062523s

@rails-bot rails-bot bot added the actionview label Jul 13, 2021
def args_for_lookup(name, prefixes, partial, keys, details_options)
name, prefixes = normalize_name(name, prefixes)
details, details_key = detail_args_for(details_options)
[name, prefixes, partial || false, details, details_key, keys]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the || false here seems to avoid passing nil. Isn't it relevant anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe it's relevant anymore. nil is never passed (at least when running the AV test suite) and this shouldn't be called outside the framework (and it isn't passed through from user input that I can see).

actionview/lib/action_view/path_set.rb Outdated Show resolved Hide resolved
Though this ends up with a bit more repetition, I believe it's a little
clearer and is also a decent amount faster.
@jhawthorn jhawthorn merged commit e70b0a4 into rails:main Jul 14, 2021
@jhawthorn jhawthorn deleted the faster_template_lookups branch July 14, 2021 14:06
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.

None yet

3 participants