Skip to content

Commit

Permalink
Consolidate passes through path ast
Browse files Browse the repository at this point in the history
Along the same lines as rails#38901,
this commit makes a small performance enhancement by iterating through
the ast once instead of twice when initializing a Mapping.

This stemmed from work to improve the boot time of an application with
3500+ routes. This patch only shaves off about 70ms from our boot time,
and about 25000 allocations, but every little bit counts!

Benchmark
---

```rb

require "bundler/inline"

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

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

  # Activate the gem you are reporting the issue against.
  gem "rails", path: "/Users/daniel/Desktop/oss/rails/rails"
  gem "benchmark-ips"
  gem "benchmark-memory", require: "benchmark/memory"
end

require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  config.session_store :cookie_store, key: "cookie_store_key"
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger = config.logger

  routes.draw do
    get("/*wildcard", to: "controller#index")
  end
end
```

With the benchmarking code inserted into Mapper#initialize:

```rb
after = -> {
  path_params = []
  wildcard_options = {}
  ast.each do |node|
    if node.symbol?
      path_params << node.to_sym
    elsif node.star? && formatted != false
      # Add a constraint for wildcard route to make it non-greedy and match the
      # optional format part of the route by default.
      wildcard_options[node.name.to_sym] ||= /.+?/
    end
  end

  wildcard_options.merge(options)
}

before = -> {
  path_params = ast.find_all(&:symbol?).map(&:to_sym)

  add_wildcard_options(options, formatted, ast)
}

puts "IPS"

Benchmark.ips do |x|
  x.report("before") { before.call }
  x.report("after") { after.call }
  x.compare!
end

puts "MEMORY"

Benchmark.memory do |x|
  x.report("before") { before.call }
  x.report("after") { after.call }
  x.compare!
end
```

The results are:

```
IPS
Warming up --------------------------------------
              before    14.352k i/100ms
               after    30.852k i/100ms
Calculating -------------------------------------
              before    135.675k (± 3.7%) i/s -    688.896k in   5.084368s
               after    288.126k (± 3.3%) i/s -      1.450M in   5.038072s

Comparison:
               after:   288126.4 i/s
              before:   135675.1 i/s - 2.12x  (± 0.00) slower

MEMORY
Calculating -------------------------------------
              before   360.000  memsize (     0.000  retained)
                         7.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
               after   200.000  memsize (     0.000  retained)
                         4.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

comparison:
               after:        200 allocated
              before:        360 allocated - 1.80x more  end
```
  • Loading branch information
composerinteralia committed Jul 22, 2020
1 parent 98ddee2 commit 6a2adeb
Showing 1 changed file with 12 additions and 14 deletions.
26 changes: 12 additions & 14 deletions actionpack/lib/action_dispatch/routing/mapper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,19 @@ def initialize(set:, ast:, controller:, default_action:, to:, formatted:, via:,
@internal = options.delete(:internal)
@scope_options = scope_params[:options]

path_params = ast.find_all(&:symbol?).map(&:to_sym)
path_params = []
wildcard_options = {}
ast.each do |node|
if node.symbol?
path_params << node.to_sym
elsif formatted != false && node.star?
# Add a constraint for wildcard route to make it non-greedy and match the
# optional format part of the route by default.
wildcard_options[node.name.to_sym] ||= /.+?/
end
end

options = add_wildcard_options(options, formatted, ast)
options = wildcard_options.merge!(options)

options = normalize_options!(options, path_params, scope_params[:module])

Expand Down Expand Up @@ -233,18 +243,6 @@ def intern(object)
object.is_a?(String) ? -object : object
end

def add_wildcard_options(options, formatted, path_ast)
# Add a constraint for wildcard route to make it non-greedy and match the
# optional format part of the route by default.
if formatted != false
path_ast.grep(Journey::Nodes::Star).each_with_object({}) { |node, hash|
hash[node.name.to_sym] ||= /.+?/
}.merge options
else
options
end
end

def normalize_options!(options, path_params, modyoule)
if path_params.include?(:controller)
raise ArgumentError, ":controller segment is not allowed within a namespace block" if modyoule
Expand Down

0 comments on commit 6a2adeb

Please sign in to comment.