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

Build Journey::Path::Pattern ast in a single loop #38901

Merged
merged 1 commit into from Apr 8, 2020

Conversation

vinistock
Copy link
Contributor

Summary

This PR makes a small performance enhancement by building Journey::Path::Pattern ast using a single each loop.

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

require "action_dispatch/journey/path/pattern"

module ActionDispatch
  module Journey
    module Path
      class Pattern
        def fast_ast
          @spec.each do |node|
            if node.symbol?
              re = @requirements[node.to_sym]
              node.regexp = re if re
            elsif node.star?
              node = node.left
              node.regexp = @requirements[node.to_sym] || /(.+)/
            end
          end

          @spec
        end
      end
    end
  end
end

path = ActionDispatch::Journey::Path::Pattern.from_string "/page/:id(/:action)(.:format)"

puts "IPS"

Benchmark.ips do |x|
  x.report("ast")      { path.ast }
  x.report("fast_ast") { path.fast_ast }
  x.compare!
end

puts "MEMORY"

Benchmark.memory do |x|
  x.report("ast")      { path.ast }
  x.report("fast_ast") { path.fast_ast }
  x.compare!
end

Results

IPS
Warming up --------------------------------------
                 ast     7.572k i/100ms
            fast_ast    16.195k i/100ms
Calculating -------------------------------------
                 ast     78.133k (± 2.1%) i/s -    393.744k in   5.041539s
            fast_ast    165.113k (± 2.5%) i/s -    825.945k in   5.005666s

Comparison:
            fast_ast:   165112.9 i/s
                 ast:    78132.7 i/s - 2.11x  slower

MEMORY
Calculating -------------------------------------
                 ast   240.000  memsize (     0.000  retained)
                         4.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
            fast_ast    80.000  memsize (     0.000  retained)
                         1.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
            fast_ast:         80 allocated
                 ast:        240 allocated - 3.00x more

@rails-bot rails-bot bot added the actionpack label Apr 8, 2020
@rafaelfranca rafaelfranca merged commit e9845d2 into rails:master Apr 8, 2020
@vinistock vinistock deleted the build_path_ast_in_single_loop branch April 8, 2020 20:22
composerinteralia added a commit to composerinteralia/rails that referenced this pull request Jul 22, 2020
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
```
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
```
composerinteralia added a commit to composerinteralia/rails that referenced this pull request Jul 25, 2020
Before this commit we initialized all Symbols with the default regexp,
then later on reassigned any symbols descending from stars with either
their regexp from `@requirements` or the default greedy regexp.

With this commit we initialize all Symbols descending from Stars with
the greedy regexp at parse time. This allows us to get rid of the star
branch in path/pattern, since any regexps from `@requirements` will
already have been set in the symbol branch of this code.

This is essentially an alternate version of rails#38901. Getting rid of the
extra branch makes some performance work I am doing a bit easier, plus
it saves us a few method calls. Also the constant saves us from creating
the same regexp multiple times, and it is nice to give that regexp a
name.
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

2 participants