Skip to content

Conversation

skipkayhil
Copy link
Member

Motivation / Background

Previously, #find_routes would take all of the routes that match the current request and eagerly generate MatchData and path_parameters for each route.

Detail

This commit changes #find_routes to only perform the computation one route at a time, since the computation will never be needed for routes in the list after #serve returns.

This change improves the performance of RouteSet#call by ~10% when #find_routes finds two routes, and ~60% when #find_routes finds ten routes.

Additional information

Before:

Warming up --------------------------------------
  10 matching routes     1.182k i/100ms
   2 matching routes     1.967k i/100ms
   1 matching routes     2.221k i/100ms
Calculating -------------------------------------
  10 matching routes     11.846k (± 3.7%) i/s -     60.282k in   5.095922s
   2 matching routes     19.871k (± 3.5%) i/s -    100.317k in   5.054796s
   1 matching routes     21.904k (± 3.8%) i/s -    111.050k in   5.077449s

Comparison:
   1 matching routes:    21904.0 i/s
   2 matching routes:    19870.6 i/s - 1.10x  slower
  10 matching routes:    11845.9 i/s - 1.85x  slower

After:

Warming up --------------------------------------
  10 matching routes     1.888k i/100ms
   2 matching routes     2.215k i/100ms
   1 matching routes     2.312k i/100ms
Calculating -------------------------------------
  10 matching routes     18.623k (± 3.7%) i/s -     94.400k in   5.076043s
   2 matching routes     22.210k (± 3.6%) i/s -    112.965k in   5.092873s
   1 matching routes     22.953k (± 4.1%) i/s -    115.600k in   5.045017s

Comparison:
   1 matching routes:    22952.9 i/s
   2 matching routes:    22210.4 i/s - same-ish: difference falls within error
  10 matching routes:    18622.8 i/s - 1.23x  slower

Benchmark:

require "bundler/inline"

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

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

  gem "actionpack", path: "~/src/github.com/skipkayhil/rails"
  gem "benchmark-ips"
end

require "action_dispatch"

routes = ActionDispatch::Routing::RouteSet.new
routes.draw do
  get "/a", to: ->(e) { [200, {}, ["a"]] }

  2.times do |i|
    is = "b" + i.to_s
    get "/b", to: ->(e) { [200, {}, [is]] }
  end

  10.times do |i|
    is = "c" + i.to_s
    get "/c", to: ->(e) { [200, {}, [is]] }
  end
end

one_env = {
  "REQUEST_METHOD" => "GET",
  "SCRIPT_NAME" => "",
  "rack.input" => File.open("/dev/null"),
  "PATH_INFO" => "/a",
}.freeze

two_env = {
  "REQUEST_METHOD" => "GET",
  "SCRIPT_NAME" => "",
  "rack.input" => File.open("/dev/null"),
  "PATH_INFO" => "/b",
}.freeze

ten_env = {
  "REQUEST_METHOD" => "GET",
  "SCRIPT_NAME" => "",
  "rack.input" => File.open("/dev/null"),
  "PATH_INFO" => "/c",
}.freeze

raise unless routes.call(one_env.dup)[2] == ["a"]
raise unless routes.call(two_env.dup)[2] == ["b0"]
raise unless routes.call(ten_env.dup)[2] == ["c0"]

require "benchmark/ips"

Benchmark.ips do |x|
  x.report("10 matching routes") { routes.call(ten_env.dup) }
  x.report("2 matching routes") { routes.call(two_env.dup) }
  x.report("1 matching routes") { routes.call(one_env.dup) }
  x.compare!
end

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Previously, `#find_routes` would take all of the routes that match the
current request and eagerly generate `MatchData` and `path_parameters`
for each route.

This commit changes `#find_routes` to only perform the computation one
route at a time, since the computation will never be needed for routes
in the list after `#serve` returns.

This change improves the performance of `RouteSet#call` by ~10% when
`#find_routes` finds two routes, and ~60% when `#find_routes` finds ten
routes.

Before:

```
Warming up --------------------------------------
  10 matching routes     1.182k i/100ms
   2 matching routes     1.967k i/100ms
   1 matching routes     2.221k i/100ms
Calculating -------------------------------------
  10 matching routes     11.846k (± 3.7%) i/s -     60.282k in   5.095922s
   2 matching routes     19.871k (± 3.5%) i/s -    100.317k in   5.054796s
   1 matching routes     21.904k (± 3.8%) i/s -    111.050k in   5.077449s

Comparison:
   1 matching routes:    21904.0 i/s
   2 matching routes:    19870.6 i/s - 1.10x  slower
  10 matching routes:    11845.9 i/s - 1.85x  slower
```

After:

```
Warming up --------------------------------------
  10 matching routes     1.888k i/100ms
   2 matching routes     2.215k i/100ms
   1 matching routes     2.312k i/100ms
Calculating -------------------------------------
  10 matching routes     18.623k (± 3.7%) i/s -     94.400k in   5.076043s
   2 matching routes     22.210k (± 3.6%) i/s -    112.965k in   5.092873s
   1 matching routes     22.953k (± 4.1%) i/s -    115.600k in   5.045017s

Comparison:
   1 matching routes:    22952.9 i/s
   2 matching routes:    22210.4 i/s - same-ish: difference falls within error
  10 matching routes:    18622.8 i/s - 1.23x  slower
```

Benchmark:

```
require "bundler/inline"

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

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

  gem "actionpack", path: "~/src/github.com/skipkayhil/rails"
  gem "benchmark-ips"
end

require "action_dispatch"

routes = ActionDispatch::Routing::RouteSet.new
routes.draw do
  get "/a", to: ->(e) { [200, {}, ["a"]] }

  2.times do |i|
    is = "b" + i.to_s
    get "/b", to: ->(e) { [200, {}, [is]] }
  end

  10.times do |i|
    is = "c" + i.to_s
    get "/c", to: ->(e) { [200, {}, [is]] }
  end
end

one_env = {
  "REQUEST_METHOD" => "GET",
  "SCRIPT_NAME" => "",
  "rack.input" => File.open("/dev/null"),
  "PATH_INFO" => "/a",
}.freeze

two_env = {
  "REQUEST_METHOD" => "GET",
  "SCRIPT_NAME" => "",
  "rack.input" => File.open("/dev/null"),
  "PATH_INFO" => "/b",
}.freeze

ten_env = {
  "REQUEST_METHOD" => "GET",
  "SCRIPT_NAME" => "",
  "rack.input" => File.open("/dev/null"),
  "PATH_INFO" => "/c",
}.freeze

raise unless routes.call(one_env.dup)[2] == ["a"]
raise unless routes.call(two_env.dup)[2] == ["b0"]
raise unless routes.call(ten_env.dup)[2] == ["c0"]

require "benchmark/ips"

Benchmark.ips do |x|
  x.report("10 matching routes") { routes.call(ten_env.dup) }
  x.report("2 matching routes") { routes.call(two_env.dup) }
  x.report("1 matching routes") { routes.call(one_env.dup) }
  x.compare!
end
```
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.

2 participants