From f35305785d13f276fdecaea44eb05a3208c3ff7d Mon Sep 17 00:00:00 2001 From: Daniel Colson Date: Mon, 27 Jul 2020 12:13:15 -0400 Subject: [PATCH] Introduce Journey::Ast to avoid extra ast walks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../lib/action_dispatch/journey/nodes/node.rb | 61 +++++++++++++ .../action_dispatch/journey/path/pattern.rb | 24 ++--- .../lib/action_dispatch/journey/route.rb | 16 ++-- .../lib/action_dispatch/journey/router.rb | 2 +- .../lib/action_dispatch/journey/routes.rb | 4 +- .../lib/action_dispatch/routing/mapper.rb | 37 +++----- actionpack/test/controller/routing_test.rb | 33 +++++++ actionpack/test/journey/nodes/ast_test.rb | 91 +++++++++++++++++++ actionpack/test/journey/path/pattern_test.rb | 16 ---- actionpack/test/journey/route_test.rb | 2 +- actionpack/test/support/path_helper.rb | 3 +- 11 files changed, 215 insertions(+), 74 deletions(-) create mode 100644 actionpack/test/journey/nodes/ast_test.rb diff --git a/actionpack/lib/action_dispatch/journey/nodes/node.rb b/actionpack/lib/action_dispatch/journey/nodes/node.rb index 04503e4010..920e3ce4a1 100644 --- a/actionpack/lib/action_dispatch/journey/nodes/node.rb +++ b/actionpack/lib/action_dispatch/journey/nodes/node.rb @@ -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 diff --git a/actionpack/lib/action_dispatch/journey/path/pattern.rb b/actionpack/lib/action_dispatch/journey/path/pattern.rb index 5314b7b2e6..4cd862f791 100644 --- a/actionpack/lib/action_dispatch/journey/path/pattern.rb +++ b/actionpack/lib/action_dispatch/journey/path/pattern.rb @@ -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 diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb index 38dfcea956..e7bbb31a64 100644 --- a/actionpack/lib/action_dispatch/journey/route.rb +++ b/actionpack/lib/action_dispatch/journey/route.rb @@ -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? diff --git a/actionpack/lib/action_dispatch/journey/router.rb b/actionpack/lib/action_dispatch/journey/router.rb index 27671b65da..7f713d1ab2 100644 --- a/actionpack/lib/action_dispatch/journey/router.rb +++ b/actionpack/lib/action_dispatch/journey/router.rb @@ -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 diff --git a/actionpack/lib/action_dispatch/journey/routes.rb b/actionpack/lib/action_dispatch/journey/routes.rb index 9dfea744e8..12b21657b9 100644 --- a/actionpack/lib/action_dispatch/journey/routes.rb +++ b/actionpack/lib/action_dispatch/journey/routes.rb @@ -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 diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index bbbc386ef5..f25d65a3ba 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -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 diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb index 3da7ab2799..3912aa1a4d 100644 --- a/actionpack/test/controller/routing_test.rb +++ b/actionpack/test/controller/routing_test.rb @@ -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 diff --git a/actionpack/test/journey/nodes/ast_test.rb b/actionpack/test/journey/nodes/ast_test.rb new file mode 100644 index 0000000000..2bfcfcc87e --- /dev/null +++ b/actionpack/test/journey/nodes/ast_test.rb @@ -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 diff --git a/actionpack/test/journey/path/pattern_test.rb b/actionpack/test/journey/path/pattern_test.rb index 78e4dc5752..bad1ee6acd 100644 --- a/actionpack/test/journey/path/pattern_test.rb +++ b/actionpack/test/journey/path/pattern_test.rb @@ -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", diff --git a/actionpack/test/journey/route_test.rb b/actionpack/test/journey/route_test.rb index ffcbea30cc..cb3c6b4199 100644 --- a/actionpack/test/journey/route_test.rb +++ b/actionpack/test/journey/route_test.rb @@ -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 diff --git a/actionpack/test/support/path_helper.rb b/actionpack/test/support/path_helper.rb index fd1cdac063..df61582138 100644 --- a/actionpack/test/support/path_helper.rb +++ b/actionpack/test/support/path_helper.rb @@ -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,