From 83ee043c6834914607849ae9cd3b9eab6b41702c Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 24 Jul 2015 22:32:01 -0500 Subject: Decrease string allocations in url_options The request.script_name is dup-d which allocates an extra string. It is most commonly an empty string "". We can save a ton of string allocations by checking first if the string is empty, if so we can use a frozen empty string instead of duplicating an empty string. This change buys us 35,714 bytes of memory and 893 fewer objects per request. --- actionpack/lib/action_controller/metal/url_for.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/url_for.rb b/actionpack/lib/action_controller/metal/url_for.rb index 5a0e5c62e4..dbf7241a14 100644 --- a/actionpack/lib/action_controller/metal/url_for.rb +++ b/actionpack/lib/action_controller/metal/url_for.rb @@ -41,7 +41,11 @@ module ActionController if original_script_name options[:original_script_name] = original_script_name else - options[:script_name] = same_origin ? request.script_name.dup : script_name + if same_origin + options[:script_name] = request.script_name.empty? ? "".freeze : request.script_name.dup + else + options[:script_name] = script_name + end end options.freeze else -- cgit v1.2.3 From 9b8258814e695fe7fbb728456498fd0fd8709f5c Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 24 Jul 2015 22:38:34 -0500 Subject: Speed up journey extract_parameterized_parts Micro optimization: `reverse.drop_while` is slower than `reverse_each.drop_while`. This doesn't save any object allocations. Second, `keys_to_keep` is typically a very small array. The operation `parameterized_parts.keys - keys_to_keep` actually allocates two arrays. It is quicker (I benchmarked) to iterate over each and check inclusion in array manually. This change buys us 1774 fewer objects per request --- actionpack/lib/action_dispatch/journey/formatter.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index c0566c6fc9..545b67422d 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -54,11 +54,12 @@ module ActionDispatch def extract_parameterized_parts(route, options, recall, parameterize = nil) parameterized_parts = recall.merge(options) - keys_to_keep = route.parts.reverse.drop_while { |part| + keys_to_keep = route.parts.reverse_each.drop_while { |part| !options.key?(part) || (options[part] || recall[part]).nil? } | route.required_parts - (parameterized_parts.keys - keys_to_keep).each do |bad_key| + parameterized_parts.each do |bad_key, _| + next if keys_to_keep.include?(bad_key) parameterized_parts.delete(bad_key) end -- cgit v1.2.3 From 097ec6fb7c7e1854cdd96113213b555ae7415953 Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 24 Jul 2015 22:47:34 -0500 Subject: Speed up journey missing_keys Most routes have a `route.path.requirements[key]` of `/[-_.a-zA-Z0-9]+\/[-_.a-zA-Z0-9]+/` yet every time this method is called a new regex is generated on the fly with `/\A#{DEFAULT_INPUT}\Z/`. OBJECT ALLOCATIONS BLERG! This change uses a special module that implements `===` so it can be used in a case statement to pull out the default input. When this happens, we use a pre-generated regex. This change buys us 1,643,465 bytes of memory and 7,990 fewer objects per request. --- actionpack/lib/action_dispatch/journey/formatter.rb | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index 545b67422d..985cf8b947 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -111,15 +111,27 @@ module ActionDispatch routes end + module RegexCaseComparator + DEFAULT_INPUT = /[-_.a-zA-Z0-9]+\/[-_.a-zA-Z0-9]+/ + DEFAULT_REGEX = /\A#{DEFAULT_INPUT}\Z/ + + def self.===(regex) + DEFAULT_INPUT == regex + end + end + # Returns an array populated with missing keys if any are present. def missing_keys(route, parts) missing_keys = [] tests = route.path.requirements route.required_parts.each { |key| - if tests.key?(key) - missing_keys << key unless /\A#{tests[key]}\Z/ === parts[key] - else + case tests[key] + when nil missing_keys << key unless parts[key] + when RegexCaseComparator + missing_keys << key unless RegexCaseComparator::DEFAULT_REGEX === parts[key] + else + missing_keys << key unless /\A#{tests[key]}\Z/ === parts[key] end } missing_keys -- cgit v1.2.3 From 0cbec58ae48279ae5e9fdf6bbdbceb32183215dd Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 24 Jul 2015 23:19:15 -0500 Subject: Decrease route_set allocations In handle_positional_args `Array#-=` is used which allocates a new array. Instead we can iterate through and delete elements, modifying the array in place. Also `Array#take` allocates a new array. We can build the same by iterating over the other element. This change buys us 106,470 bytes of memory and 2,663 fewer objects per request. --- actionpack/lib/action_dispatch/routing/route_set.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 42512cad91..43e35366a9 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -267,9 +267,13 @@ module ActionDispatch path_params -= controller_options.keys path_params -= result.keys end - path_params -= inner_options.keys - path_params.take(args.size).each do |param| - result[param] = args.shift + inner_options.each do |key, _| + path_params.delete(key) + end + + args.each_with_index do |arg, index| + param = path_params[index] + result[param] = arg if param end end -- cgit v1.2.3 From 1a14074fb525cde4a439a1fb7515fe417c37ff96 Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 24 Jul 2015 23:27:35 -0500 Subject: Reduce hash allocations in route_set When generating a url with `url_for` the hash of arguments passed in, is dup-d and merged a TON. I wish I could clean this up better, and might be able to do it in the future. This change removes one dup, since it's literally right after we just dup-d the hash to pass into this constructor. This may be a breaking, change but the tests pass...so :shipit: we can revert if it causes problems This change buys us 205,933 bytes of memory and 887 fewer objects per request. --- actionpack/lib/action_dispatch/routing/route_set.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 43e35366a9..d93989a449 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -598,7 +598,7 @@ module ActionDispatch def initialize(named_route, options, recall, set) @named_route = named_route - @options = options.dup + @options = options @recall = recall.dup @set = set -- cgit v1.2.3 From bff61ba27cb113b90f210a30173a8e66602615ca Mon Sep 17 00:00:00 2001 From: schneems Date: Sat, 25 Jul 2015 00:25:47 -0500 Subject: Avoid calling to_s on nil in journey/formatter When `defaults[key]` in `generate` in the journey formatter is called, it often returns a `nil` when we call `to_s` on a nil, it allocates an empty string. We can skip this check when the default value is nil. This change buys us 35,431 bytes of memory and 887 fewer objects per request. Thanks to @matthewd for help with the readability --- actionpack/lib/action_dispatch/journey/formatter.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index 985cf8b947..95e1d2deb2 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -32,8 +32,8 @@ module ActionDispatch defaults = route.defaults required_parts = route.required_parts - parameterized_parts.delete_if do |key, value| - value.to_s == defaults[key].to_s && !required_parts.include?(key) + parameterized_parts.keep_if do |key, value| + defaults[key].nil? || value.to_s != defaults[key].to_s || required_parts.include?(key) end return [route.format(parameterized_parts), params] -- cgit v1.2.3 From 045cdd3a3d50b06019361cf7bec57f2521facff6 Mon Sep 17 00:00:00 2001 From: schneems Date: Sat, 25 Jul 2015 00:30:43 -0500 Subject: Freeze a string in comparator Saves 888 string objects per request. --- actionpack/lib/action_dispatch/routing/route_set.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index d93989a449..84aeb65007 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -680,7 +680,7 @@ module ActionDispatch # Move 'index' action from options to recall def normalize_action! - if @options[:action] == 'index' + if @options[:action] == 'index'.freeze @recall[:action] = @options.delete(:action) end end -- cgit v1.2.3 From 3fb9e802436a5e3b5733ea9d5cb3964a32a3d8f9 Mon Sep 17 00:00:00 2001 From: schneems Date: Sat, 25 Jul 2015 10:52:11 -0500 Subject: Only allocate new string when needed Instead of calling `sub` on every link_to call for controller, we can detect when the string __needs__ to be allocated and only then create a new string (without the leading slash), otherwise, use the string that is given to us. Saves 888 string objects per request, 35,524 bytes. --- actionpack/lib/action_dispatch/routing/route_set.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 84aeb65007..848b20b054 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -675,7 +675,13 @@ module ActionDispatch # Remove leading slashes from controllers def normalize_controller! - @options[:controller] = controller.sub(%r{^/}, ''.freeze) if controller + if controller + if m = controller.match(/\A\/(?.*)/) + @options[:controller] = m[:controller_without_leading_slash] + else + @options[:controller] = controller + end + end end # Move 'index' action from options to recall -- cgit v1.2.3 From 1993e2ccbd7c5651278ea30bdc9d8034f5197945 Mon Sep 17 00:00:00 2001 From: schneems Date: Sat, 25 Jul 2015 16:58:22 -0500 Subject: Avoid hash duplication by skipping mutation If we don't mutate the `recall` hash, then there's no reason to duplicate it. While this change doesn't get rid of that many objects, each hash object it gets rid of was massive. Saves 888 string objects per request, 206,013 bytes (thats 0.2 mb which is kinda a lot). --- actionpack/lib/action_dispatch/routing/route_set.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 848b20b054..a006a146ed 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -599,7 +599,7 @@ module ActionDispatch def initialize(named_route, options, recall, set) @named_route = named_route @options = options - @recall = recall.dup + @recall = recall @set = set normalize_recall! @@ -621,7 +621,7 @@ module ActionDispatch def use_recall_for(key) if @recall[key] && (!@options.key?(key) || @options[key] == @recall[key]) if !named_route_exists? || segment_keys.include?(key) - @options[key] = @recall.delete(key) + @options[key] = @recall[key] end end end -- cgit v1.2.3 From 4d2ccc119cac2dc2757d3d977059feba9db858d2 Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 27 Jul 2015 01:45:29 -0500 Subject: Remove array allocation THe only reason we were allocating an array is to get the "missing_keys" variable in scope of the error message generator. Guess what? Arrays kinda take up a lot of memory, so by replacing that with a nil, we save: 35,303 bytes and 886 objects per request --- actionpack/lib/action_dispatch/journey/formatter.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index 95e1d2deb2..7b091a35ab 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -14,7 +14,7 @@ module ActionDispatch def generate(name, options, path_parameters, parameterize = nil) constraints = path_parameters.merge(options) - missing_keys = [] + missing_keys = nil # need for variable scope match_route(name, constraints) do |route| parameterized_parts = extract_parameterized_parts(route, options, path_parameters, parameterize) @@ -40,7 +40,7 @@ module ActionDispatch end message = "No route matches #{Hash[constraints.sort_by{|k,v| k.to_s}].inspect}" - message << " missing required keys: #{missing_keys.sort.inspect}" unless missing_keys.empty? + message << " missing required keys: #{missing_keys.sort.inspect}" if missing_keys && missing_keys.any? raise ActionController::UrlGenerationError, message end -- cgit v1.2.3 From 61dae882546f643050c47e573161a467e156c704 Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 27 Jul 2015 01:52:45 -0500 Subject: Remove (another) array allocation We don't always need an array when generating a url with the formatter. We can be lazy about allocating the `missing_keys` array. This saves us: 35,606 bytes and 889 objects per request --- actionpack/lib/action_dispatch/journey/formatter.rb | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index 7b091a35ab..d6daf5280b 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -25,7 +25,7 @@ module ActionDispatch next unless name || route.dispatcher? missing_keys = missing_keys(route, parameterized_parts) - next unless missing_keys.empty? + next if missing_keys && missing_keys.any? params = options.dup.delete_if do |key, _| parameterized_parts.key?(key) || route.defaults.key?(key) end @@ -122,16 +122,25 @@ module ActionDispatch # Returns an array populated with missing keys if any are present. def missing_keys(route, parts) - missing_keys = [] + missing_keys = nil tests = route.path.requirements route.required_parts.each { |key| case tests[key] when nil - missing_keys << key unless parts[key] + unless parts[key] + missing_keys ||= [] + missing_keys << key + end when RegexCaseComparator - missing_keys << key unless RegexCaseComparator::DEFAULT_REGEX === parts[key] + unless RegexCaseComparator::DEFAULT_REGEX === parts[key] + missing_keys ||= [] + missing_keys << key + end else - missing_keys << key unless /\A#{tests[key]}\Z/ === parts[key] + unless /\A#{tests[key]}\Z/ === parts[key] + missing_keys ||= [] + missing_keys << key + end end } missing_keys -- cgit v1.2.3 From 22f592401444a83b7b2d3c2165ac666d69955d08 Mon Sep 17 00:00:00 2001 From: schneems Date: Thu, 30 Jul 2015 12:34:48 -0500 Subject: Use delete_if instead of each; delete(key) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is slightly faster: ``` Calculating ------------------------------------- each; delete 35.166k i/100ms delete_if 36.416k i/100ms ------------------------------------------------- each; delete 478.026k (± 8.5%) i/s - 2.391M delete_if 485.123k (± 7.9%) i/s - 2.440M ``` --- actionpack/lib/action_dispatch/journey/formatter.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index d6daf5280b..d839fec48d 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -58,9 +58,8 @@ module ActionDispatch !options.key?(part) || (options[part] || recall[part]).nil? } | route.required_parts - parameterized_parts.each do |bad_key, _| - next if keys_to_keep.include?(bad_key) - parameterized_parts.delete(bad_key) + parameterized_parts.delete_if do |bad_key, _| + !keys_to_keep.include?(bad_key) end if parameterize -- cgit v1.2.3