kotovalexarian-likes-github
/
rails--rails
mirror of https://github.com/rails/rails.git
1
0
Fork 0
Code Releases Activity

Introduce Journey::Ast to avoid extra ast walks 

This commit introduces a new `Journey::Ast` class that wraps the root
node of an ast. The purpose of this class is to reduce the number of
walks through the ast by taking a single pass through each node and
caching values for later use.

To avoid retaining additional memory, we clear out these ast objects
after eager loading.

Benefits
---

* Performance improvements (see benchmarks below)
* Keeps various ast manipulations together in a single class, rather
  than scattered throughout
* Adding some names to things will hopefully make this code a little
  easier to follow for future readers

Benchmarks
---

We benchmarked loading a real routes file with > 3500 routes.
master was at a9336a67b0 when we ran these. Note that these benchmarks
also include a few small changes that we didn't include in this commit,
but that we will follow up with after this gets merged - these
additional changes do not change the benchmarks significantly.

Time:

```
master      - 0.798 ± 0.024 (500 runs)
this branch - 0.695 ± 0.029 (500 runs)
```

Allocations:

```
master      - 980149 total allocated objects
this branch - 931357 total allocated objects
```

Stackprof:

Seeing `ActionDispatch::Journey::Visitors::Each#visit` more frequently
on the stack is what led us down this path in the first place. These
changes seem to have done the trick.

```
master:
  TOTAL    (pct)     SAMPLES    (pct)     FRAME
    52   (0.5%)          52   (0.5%)     ActionDispatch::Journey::Nodes::Node#symbol?
    58   (0.5%)          45   (0.4%)     ActionDispatch::Journey::Scanner#scan
    45   (0.4%)          45   (0.4%)     ActionDispatch::Journey::Nodes::Cat#type
    43   (0.4%)          43   (0.4%)     ActionDispatch::Journey::Visitors::FunctionalVisitor#terminal
    303  (2.7%)          43   (0.4%)     ActionDispatch::Journey::Visitors::Each#visit
    69   (0.6%)          40   (0.4%)     ActionDispatch::Routing::Mapper::Scope#each

this commit:
  TOTAL    (pct)     SAMPLES    (pct)     FRAME
    82   (0.6%)          42   (0.3%)     ActionDispatch::Journey::Scanner#next_token
    31   (0.2%)          31   (0.2%)     ActionDispatch::Journey::Nodes::Node#symbol?
    30   (0.2%)          30   (0.2%)     ActionDispatch::Journey::Nodes::Node#initialize
```

See also the benchmark script in https://github.com/rails/rails/pull/39935#issuecomment-887791294

Co-authored-by: Eric Milford <ericmilford@gmail.com>
This commit is contained in:
Daniel Colson 2020-07-27 12:13:15 -04:00
parent 1bf3ce896c
commit f35305785d
No known key found for this signature in database
GPG Key ID: 88A364BBE77B1353
11 changed files with 215 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
actionpack/lib/action_dispatch/journey/nodes/node.rb
View File

@ -4,6 +4,67 @@ require "action_dispatch/journey/visitors"
module ActionDispatch
module Journey # :nodoc:
class Ast # :nodoc:
delegate :find_all, :left, :right, :to_s, :to_sym, :type, to: :tree
attr_reader :names, :path_params, :tree, :wildcard_options, :terminals
alias :root :tree
def initialize(tree, formatted)
@tree = tree
@path_params = []
@names = []
@symbols = []
@stars = []
@terminals = []
@wildcard_options = {}
visit_tree(formatted)
end
def requirements=(requirements)
# inject any regexp requirements for `star` nodes so they can be
# determined nullable, which requires knowing if the regex accepts an
# empty string.
(symbols + stars).each do |node|
re = requirements[node.to_sym]
node.regexp = re if re
end
end
def route=(route)
terminals.each { |n| n.memo = route }
end
def glob?
stars.any?
end
private
attr_reader :symbols, :stars
def visit_tree(formatted)
tree.each do |node|
if node.symbol?
path_params << node.to_sym
names << node.name
symbols << node
elsif node.star?
stars << node
if 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
if node.terminal?
terminals << node
end
end
end
end
module Nodes # :nodoc:
class Node # :nodoc:
include Enumerable

24
actionpack/lib/action_dispatch/journey/path/pattern.rb
View File

@ -4,15 +4,16 @@ module ActionDispatch
module Journey # :nodoc:
module Path # :nodoc:
class Pattern # :nodoc:
attr_reader :spec, :requirements, :anchored
attr_reader :ast, :names, :requirements, :anchored
alias :spec :ast
def initialize(ast, requirements, separators, anchored)
@spec = ast
@ast = ast
@requirements = requirements
@separators = separators
@anchored = anchored
@names = nil
@names = ast.names
@optional_names = nil
@required_names = nil
@re = nil
@ -27,21 +28,12 @@ module ActionDispatch
required_names
offsets
to_regexp
nil
end
def ast
@spec.find_all(&:symbol?).each do |node|
re = @requirements[node.to_sym]
node.regexp = re if re
end
@spec
@ast = nil
end
def requirements_anchored?
# each required param must not be surrounded by a literal, otherwise it isn't simple to chunk-match the url piecemeal
terminals = ast.find_all { |t| t.is_a?(Nodes::Terminal) }
terminals = ast.terminals
terminals.each_with_index { |s, index|
next if index < 1
@ -60,10 +52,6 @@ module ActionDispatch
true
end
def names
@names ||= spec.find_all(&:symbol?).map(&:name)
end
def required_names
@required_names ||= names - optional_names
end

16
actionpack/lib/action_dispatch/journey/route.rb
View File

@ -5,7 +5,7 @@ module ActionDispatch
module Journey
class Route
attr_reader :app, :path, :defaults, :name, :precedence, :constraints,
:internal, :scope_options
:internal, :scope_options, :ast_root
alias :conditions :constraints
@ -70,24 +70,20 @@ module ActionDispatch
@path_formatter = @path.build_formatter
@scope_options = scope_options
@internal = internal
@ast_root = @path.ast.root
@path.ast.route = self
end
def eager_load!
path.eager_load!
ast
parts
required_defaults
nil
end
def ast
@decorated_ast ||= begin
decorated_ast = path.ast
decorated_ast.find_all(&:terminal?).each { |n| n.memo = self }
# inject any regexp requirements for `star` nodes so they can be determined nullable, which requires knowing if the regex accepts an empty string.
decorated_ast.find_all(&:star?).each { |n| n.regexp = path.requirements[n.name.to_sym] unless path.requirements[n.name.to_sym].nil? }
decorated_ast
end
path.ast
end
# Needed for `bin/rails routes`. Picks up succinctly defined requirements
@ -144,7 +140,7 @@ module ActionDispatch
end
def glob?
path.spec.any?(Nodes::Star)
ast.glob?
end
def dispatcher?

2
actionpack/lib/action_dispatch/journey/router.rb
View File

@ -77,7 +77,7 @@ module ActionDispatch
def visualizer
tt = GTG::Builder.new(ast).transition_table
groups = partitioned_routes.first.map(&:ast).group_by(&:to_s)
groups = partitioned_routes.first.map(&:ast_root).group_by(&:to_s)
asts = groups.values.map(&:first)
tt.visualizer(asts)
end

4
actionpack/lib/action_dispatch/journey/routes.rb
View File

@ -50,8 +50,8 @@ module ActionDispatch
def ast
@ast ||= begin
asts = anchored_routes.map(&:ast)
Nodes::Or.new(asts)
nodes = anchored_routes.map(&:ast_root)
Nodes::Or.new(nodes)
end
end

37
actionpack/lib/action_dispatch/routing/mapper.rb
View File

@ -70,7 +70,7 @@ module ActionDispatch
ANCHOR_CHARACTERS_REGEX = %r{\A(\\A|\^)|(\\Z|\\z|\$)\Z}
OPTIONAL_FORMAT_REGEX = %r{(?:\(\.:format\)+|\.:format|/)\Z}
attr_reader :requirements, :defaults, :to, :default_controller,
attr_reader :path, :requirements, :defaults, :to, :default_controller,
:default_action, :required_defaults, :ast, :scope_options
def self.build(scope, set, ast, controller, default_action, to, via, formatted, options_constraints, anchor, options)
@ -121,29 +121,17 @@ module ActionDispatch
@to = intern(to)
@default_controller = intern(controller)
@default_action = intern(default_action)
@ast = ast
@anchor = anchor
@via = via
@internal = options.delete(:internal)
@scope_options = scope_params[:options]
ast = Journey::Ast.new(ast, formatted)
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 = ast.wildcard_options.merge!(options)
options = wildcard_options.merge!(options)
options = normalize_options!(options, ast.path_params, scope_params[:module])
options = normalize_options!(options, path_params, scope_params[:module])
split_options = constraints(options, path_params)
split_options = constraints(options, ast.path_params)
constraints = scope_params[:constraints].merge Hash[split_options[:constraints] || []]
@ -157,7 +145,7 @@ module ActionDispatch
@blocks = blocks(options_constraints)
end
requirements, conditions = split_constraints path_params, constraints
requirements, conditions = split_constraints ast.path_params, constraints
verify_regexp_requirements requirements.map(&:last).grep(Regexp)
formats = normalize_format(formatted)
@ -166,13 +154,18 @@ module ActionDispatch
@conditions = Hash[conditions]
@defaults = formats[:defaults].merge(@defaults).merge(normalize_defaults(options))
if path_params.include?(:action) && !@requirements.key?(:action)
if ast.path_params.include?(:action) && !@requirements.key?(:action)
@defaults[:action] ||= "index"
end
@required_defaults = (split_options[:required_defaults] || []).map(&:first)
ast.requirements = @requirements
@path = Journey::Path::Pattern.new(ast, @requirements, JOINED_SEPARATORS, @anchor)
end
JOINED_SEPARATORS = SEPARATORS.join # :nodoc:
def make_route(name, precedence)
Journey::Route.new(name: name, app: application, path: path, constraints: conditions,
required_defaults: required_defaults, defaults: defaults,
@ -184,12 +177,6 @@ module ActionDispatch
app(@blocks)
end
JOINED_SEPARATORS = SEPARATORS.join # :nodoc:
def path
Journey::Path::Pattern.new(@ast, requirements, JOINED_SEPARATORS, @anchor)
end
def conditions
build_conditions @conditions, @set.request_class
end

33
actionpack/test/controller/routing_test.rb
View File

@ -923,6 +923,39 @@ class LegacyRouteSetTests < ActiveSupport::TestCase
assert_not_nil hash
assert_equal %w(cc ac), [hash[:controller], hash[:action]]
end
def test_drawing_more_routes_after_eager_loading
rs = ::ActionDispatch::Routing::RouteSet.new
rs.disable_clear_and_finalize = true
rs.draw do
get "/plain" => "c#plain"
get "/:symbol" => "c#symbol"
get "/glob/*" => "c#glob"
get "/with#anchor" => "c#with_anchor"
end
hash = rs.recognize_path "/symbol"
assert_not_nil hash
assert_equal %w(c symbol), [hash[:controller], hash[:action]]
rs.eager_load!
rs.draw do
get "/more/plain" => "c#plain"
get "/more/:symbol" => "c#symbol"
get "/more/glob/*" => "c#glob"
get "/more/with#anchor" => "c#with_anchor"
end
hash = rs.recognize_path "/symbol"
assert_not_nil hash
assert_equal %w(c symbol), [hash[:controller], hash[:action]]
hash = rs.recognize_path "/more/symbol"
assert_not_nil hash
assert_equal %w(c symbol), [hash[:controller], hash[:action]]
end
end
class RouteSetTest < ActiveSupport::TestCase

91
actionpack/test/journey/nodes/ast_test.rb Normal file
View File

@ -0,0 +1,91 @@
# frozen_string_literal: true
require "abstract_unit"
module ActionDispatch
module Journey
module Nodes
class TestAst < ActiveSupport::TestCase
def test_ast_sets_regular_expressions
requirements = { name: /(tender|love)/, value: /./ }
path = "/page/:name/:value"
tree = Journey::Parser.new.parse(path)
ast = Ast.new(tree, true)
ast.requirements = requirements
nodes = ast.root.grep(Nodes::Symbol)
assert_equal 2, nodes.length
nodes.each do |node|
assert_equal requirements[node.to_sym], node.regexp
end
end
def test_sets_memo_for_terminal_nodes
route = Object.new
tree = Journey::Parser.new.parse("/path")
ast = Ast.new(tree, true)
ast.route = route
nodes = ast.root.grep(Nodes::Terminal)
nodes.each do |node|
assert_equal route, node.memo
end
end
def test_contains_glob
tree = Journey::Parser.new.parse("/*glob")
ast = Ast.new(tree, true)
assert_predicate ast, :glob?
end
def test_does_not_contain_glob
tree = Journey::Parser.new.parse("/")
ast = Ast.new(tree, true)
assert_not_predicate ast, :glob?
end
def test_names
tree = Journey::Parser.new.parse("/:path/:symbol")
ast = Ast.new(tree, true)
assert_equal ["path", "symbol"], ast.names
end
def test_path_params
tree = Journey::Parser.new.parse("/:path/:symbol")
ast = Ast.new(tree, true)
assert_equal [:path, :symbol], ast.path_params
end
def test_wildcard_options_when_formatted
tree = Journey::Parser.new.parse("/*glob")
ast = Ast.new(tree, true)
wildcard_options = ast.wildcard_options
assert_equal %r{.+?}, wildcard_options[:glob]
end
def test_wildcard_options_when_false
tree = Journey::Parser.new.parse("/*glob")
ast = Ast.new(tree, false)
wildcard_options = ast.wildcard_options
assert_nil wildcard_options[:glob]
end
def test_wildcard_options_when_nil
tree = Journey::Parser.new.parse("/*glob")
ast = Ast.new(tree, nil)
wildcard_options = ast.wildcard_options
assert_equal %r{.+?}, wildcard_options[:glob]
end
end
end
end
end

16
actionpack/test/journey/path/pattern_test.rb
View File

@ -133,22 +133,6 @@ module ActionDispatch
assert_no_match(path, "/page/loving")
end
def test_ast_sets_regular_expressions
requirements = { name: /(tender|love)/, value: /./ }
path = build_path(
"/page/:name/:value",
requirements,
SEPARATORS,
true
)
nodes = path.ast.grep(Nodes::Symbol)
assert_equal 2, nodes.length
nodes.each do |node|
assert_equal requirements[node.to_sym], node.regexp
end
end
def test_match_data_with_group
path = build_path(
"/page/:name",

2
actionpack/test/journey/route_test.rb
View File

@ -24,7 +24,7 @@ module ActionDispatch
path = path_from_string "/:controller(/:action(/:id(.:format)))"
route = Route.new(name: "name", app: app, path: path)
route.ast.grep(Nodes::Terminal).each do |node|
route.ast.root.grep(Nodes::Terminal).each do |node|
assert_equal route, node.memo
end
end

3
actionpack/test/support/path_helper.rb
View File

@ -7,9 +7,10 @@ module ActionDispatch
build_path(string, {}, "/.?", true)
end
def build_path(path, requirements, separators, anchored)
def build_path(path, requirements, separators, anchored, formatted = true)
parser = ActionDispatch::Journey::Parser.new
ast = parser.parse path
ast = Journey::Ast.new(ast, formatted)
ActionDispatch::Journey::Path::Pattern.new(
ast,
requirements,