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

Consolidate passes through path ast #39903

Merged
merged 1 commit into from
Jul 22, 2020

Conversation

composerinteralia
Copy link
Member

Along the same lines as #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 25,000 allocations, but every little bit counts!

Benchmark

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:

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

@rails-bot rails-bot bot added the actionpack label Jul 22, 2020
Copy link

@tabfugnic tabfugnic left a comment

Choose a reason for hiding this comment

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

This seems like a really sensible change and remains very easy to read!

Nice job!

actionpack/lib/action_dispatch/routing/mapper.rb Outdated Show resolved Hide resolved
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
```
@kamipo kamipo merged commit f0de473 into rails:master Jul 22, 2020
@composerinteralia composerinteralia deleted the fewer-passes-through-ast branch July 22, 2020 23:23
composerinteralia added a commit to composerinteralia/rails that referenced this pull request Jul 23, 2020
Along the same lines as rails#38901, rails#39903, and rails#39914, this commit removes
one pass through the journey ast in an attempt to improve application
boot time.

Before this commit, `Mapper#path` would iterate through the ast to alter
the regex for custom routes. With this commit we move the regex
alterations to initialize, where we were already iterating through the
ast.

The Benchmark for this change is similar to what we have been seeing in
the other PRs.

```
Warming up --------------------------------------
               after    13.121k i/100ms
              before     7.416k i/100ms
Calculating -------------------------------------
               after    128.469k (± 3.1%) i/s -    642.929k in 5.009391s
              before     76.561k (± 1.8%) i/s -    385.632k in 5.038677s

Comparison:
               after:   128469.4 i/s
              before:    76560.8 i/s - 1.68x  (± 0.00) slower

Calculating -------------------------------------
               after   160.000  memsize (     0.000  retained)
                         3.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
              before   360.000  memsize (     0.000  retained)
                         6.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
               after:        160 allocated
              before:        360 allocated - 2.25x more
```
composerinteralia added a commit to composerinteralia/rails that referenced this pull request Jul 24, 2020
Before this commit #ast and #names were each doing a separate pass over
the journey ast. Since both of these methods are called for every route
definition, we can do that work up front in #initialize, consolidating
into a single pass through the ast.

I think we can also get rid of the `spec` alias, but I am planning to do
that in a separate PR to limit the scope of this one.

This is similar to the work done in rails#38901 and rails#39903, and the benchmark
results are similar:

```
Warming up --------------------------------------
              before     7.791k i/100ms
               after    15.843k i/100ms
Calculating -------------------------------------
              before     78.081k (± 1.4%) i/s -    397.341k in   5.089797s
               after    159.093k (± 1.8%) i/s -    807.993k in   5.080367s

Comparison:
               after:   159093.1 i/s
              before:    78081.2 i/s - 2.04x  (± 0.00) slower

MEMORY
Calculating -------------------------------------
              before   240.000  memsize (    40.000  retained)
                         4.000  objects (     1.000  retained)
                         0.000  strings (     0.000  retained)
               after   120.000  memsize (    40.000  retained)
                         2.000  objects (     1.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
               after:        120 allocated
              before:        240 allocated - 2.00x more
```
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

4 participants