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

Beyond Ludicrous Speed #21057

Merged
merged 22 commits into from Jul 30, 2015
Commits
Jump to file or symbol
Failed to load files and symbols.
+157 −68
Diff settings

Always

Just for now

@@ -41,7 +41,11 @@ def url_options
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
@@ -14,7 +14,7 @@ def initialize(routes)
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)
@@ -25,22 +25,22 @@ def generate(name, options, path_parameters, parameterize = nil)
next unless name || route.dispatcher?
missing_keys = missing_keys(route, parameterized_parts)
next unless missing_keys.empty?
next if missing_keys && missing_keys.any?

This comment has been minimized.

@byroot

byroot Jul 30, 2015

Member

Isn't any? doing quite a lot more than !empty?.

empty_array = []
small_array = [1] * 30
bigger_array = [1] * 300

Benchmark.ips do |x|
  x.report('empty !empty?') { !empty_array.empty? }
  x.report('small !empty?') { !small_array.empty? }
  x.report('bigger !empty?') { !bigger_array.empty? }

  x.report('empty any?') { empty_array.any? }
  x.report('small any?') { small_array.any? }
  x.report('bigger any?') { bigger_array.any? }
end
Calculating -------------------------------------
       empty !empty?   132.059k i/100ms
       small !empty?   133.974k i/100ms
      bigger !empty?   133.848k i/100ms
          empty any?   106.924k i/100ms
          small any?    85.525k i/100ms
         bigger any?    86.663k i/100ms
-------------------------------------------------
       empty !empty?      8.522M (± 7.9%) i/s -     42.391M
       small !empty?      8.501M (± 8.5%) i/s -     42.202M
      bigger !empty?      8.434M (± 8.6%) i/s -     41.894M
          empty any?      4.161M (± 8.3%) i/s -     20.743M
          small any?      2.654M (± 5.2%) i/s -     13.256M
         bigger any?      2.642M (± 6.4%) i/s -     13.173M
@byroot

byroot Jul 30, 2015

Member

Isn't any? doing quite a lot more than !empty?.

empty_array = []
small_array = [1] * 30
bigger_array = [1] * 300

Benchmark.ips do |x|
  x.report('empty !empty?') { !empty_array.empty? }
  x.report('small !empty?') { !small_array.empty? }
  x.report('bigger !empty?') { !bigger_array.empty? }

  x.report('empty any?') { empty_array.any? }
  x.report('small any?') { small_array.any? }
  x.report('bigger any?') { bigger_array.any? }
end
Calculating -------------------------------------
       empty !empty?   132.059k i/100ms
       small !empty?   133.974k i/100ms
      bigger !empty?   133.848k i/100ms
          empty any?   106.924k i/100ms
          small any?    85.525k i/100ms
         bigger any?    86.663k i/100ms
-------------------------------------------------
       empty !empty?      8.522M (± 7.9%) i/s -     42.391M
       small !empty?      8.501M (± 8.5%) i/s -     42.202M
      bigger !empty?      8.434M (± 8.6%) i/s -     41.894M
          empty any?      4.161M (± 8.3%) i/s -     20.743M
          small any?      2.654M (± 5.2%) i/s -     13.256M
         bigger any?      2.642M (± 6.4%) i/s -     13.173M

This comment has been minimized.

@schneems

schneems Jul 30, 2015

Member

You're correct, I never knew. it looks like the big difference is this line: https://github.com/ruby/ruby/blob/trunk/array.c#L5524

I'm actually looking to see if we can optimize the case of any? a little for when the array is empty. It will still be slightly slower though. I think it would make sense for this to be:

if missing_keys && !missing_keys.empty?

Want to give me a PR and @ mention me?

@schneems

schneems Jul 30, 2015

Member

You're correct, I never knew. it looks like the big difference is this line: https://github.com/ruby/ruby/blob/trunk/array.c#L5524

I'm actually looking to see if we can optimize the case of any? a little for when the array is empty. It will still be slightly slower though. I think it would make sense for this to be:

if missing_keys && !missing_keys.empty?

Want to give me a PR and @ mention me?

This comment has been minimized.

@byroot

byroot Jul 30, 2015

Member

Sure.

@byroot

byroot Jul 30, 2015

Member

Sure.

params = options.dup.delete_if do |key, _|
parameterized_parts.key?(key) || route.defaults.key?(key)
end
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]
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
@@ -54,12 +54,12 @@ def clear
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.delete(bad_key)
parameterized_parts.delete_if do |bad_key, _|
!keys_to_keep.include?(bad_key)
end
if parameterize
@@ -110,15 +110,36 @@ def non_recursive(cache, options)
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 = []
missing_keys = nil
tests = route.path.requirements
route.required_parts.each { |key|
if tests.key?(key)
missing_keys << key unless /\A#{tests[key]}\Z/ === parts[key]
case tests[key]
when nil

This comment has been minimized.

@jeremy

jeremy Jul 30, 2015

Member

Changes a presence check to a nil check—can this value legitimately be nil?

@jeremy

jeremy Jul 30, 2015

Member

Changes a presence check to a nil check—can this value legitimately be nil?

This comment has been minimized.

@schneems

schneems Jul 30, 2015

Member

I don't think you can have a required key be nil. I ran this against the test suite and came up with nothing:

          puts tests.inspect if tests.values.include?(nil)
@schneems

schneems Jul 30, 2015

Member

I don't think you can have a required key be nil. I ran this against the test suite and came up with nothing:

          puts tests.inspect if tests.values.include?(nil)
unless parts[key]
missing_keys ||= []
missing_keys << key
end
when RegexCaseComparator
unless RegexCaseComparator::DEFAULT_REGEX === parts[key]
missing_keys ||= []
missing_keys << key
end
else
missing_keys << key unless parts[key]
unless /\A#{tests[key]}\Z/ === parts[key]
missing_keys ||= []
missing_keys << key
end
end
}
missing_keys
@@ -267,9 +267,13 @@ def handle_positional_args(controller_options, inner_options, args, result, path
path_params -= controller_options.keys
path_params -= result.keys

This comment has been minimized.

@bquorning

bquorning Aug 1, 2015

Contributor

I don’t know how often this code path is reached (inside an if args.size < path_params_size block), but the argument for saving an array allocation in 0cbec58 might apply for lines 267 and 268 as well.

Just a thought: Doesn’t the refactoring from foo -= bar.keys to bar.each { |key, _| foo.delete(key) } smell like there’s a method missing on the Array class?

@bquorning

bquorning Aug 1, 2015

Contributor

I don’t know how often this code path is reached (inside an if args.size < path_params_size block), but the argument for saving an array allocation in 0cbec58 might apply for lines 267 and 268 as well.

Just a thought: Doesn’t the refactoring from foo -= bar.keys to bar.each { |key, _| foo.delete(key) } smell like there’s a method missing on the Array class?

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
@@ -594,8 +598,8 @@ class Generator
def initialize(named_route, options, recall, set)
@named_route = named_route
@options = options.dup
@recall = recall.dup
@options = options
@recall = recall
@set = set
normalize_recall!
@@ -617,7 +621,7 @@ def current_controller
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]

This comment has been minimized.

@jeremy

jeremy Jul 30, 2015

Member

Red flag, kind of thing that may cause subtle/untested regressions.

@jeremy

jeremy Jul 30, 2015

Member

Red flag, kind of thing that may cause subtle/untested regressions.

This comment has been minimized.

@schneems

schneems Jul 30, 2015

Member

I agree, it raised some red flags with me when I first did it. Here's the specific commit:

schneems@9060b92

It's a fairly large savings in memory. Here's why I think it's okay

This method moves a key/value pair from recall to options

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[key]
    end
  end
end

I changed it so that recall is preserved i.e. the value is copied not "moved." This method gets called 3 times for the keys :controller, :action, and :id:

# This pulls :controller, :action, and :id out of the recall.
# The recall key is only used if there is no key in the options
# or if the key in the options is identical. If any of
# :controller, :action or :id is not found, don't pull any
# more keys from the recall.
def normalize_controller_action_id!
  use_recall_for(:controller) or return
  use_recall_for(:action) or return
  use_recall_for(:id)
end

Based on the comment every time options would have a key, it should be favored over recall, so it doesn't matter that recall also has the key. But let's not trust comments, let's look at the code. After we are done in this class the formatter is called:

@set.formatter.generate(named_route, options, recall, PARAMETERIZE)

Here the first thing that is done is to merge the two hashes:

def generate(name, options, path_parameters, parameterize = nil)
  constraints = path_parameters.merge(options)

So whether we delete the key in recall (which becomes path_parameters) the constraints hash will be the same.

You might be thinking "well i bet path_parameters is uesed somewhere else" and you would be right

It's used here:

parameterized_parts = extract_parameterized_parts(route, options, path_parameters, parameterize)

Yet once again, it's immediately merged:

def extract_parameterized_parts(route, options, recall, parameterize = nil)
  parameterized_parts = recall.merge(options)

In both cases we will use the key in options if it exists. I'm pretty comfortable with this change, but I would like some more eyes. Maybe I missed a really subtle and untested use-case, for which we should certainly add some tests.

@schneems

schneems Jul 30, 2015

Member

I agree, it raised some red flags with me when I first did it. Here's the specific commit:

schneems@9060b92

It's a fairly large savings in memory. Here's why I think it's okay

This method moves a key/value pair from recall to options

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[key]
    end
  end
end

I changed it so that recall is preserved i.e. the value is copied not "moved." This method gets called 3 times for the keys :controller, :action, and :id:

# This pulls :controller, :action, and :id out of the recall.
# The recall key is only used if there is no key in the options
# or if the key in the options is identical. If any of
# :controller, :action or :id is not found, don't pull any
# more keys from the recall.
def normalize_controller_action_id!
  use_recall_for(:controller) or return
  use_recall_for(:action) or return
  use_recall_for(:id)
end

Based on the comment every time options would have a key, it should be favored over recall, so it doesn't matter that recall also has the key. But let's not trust comments, let's look at the code. After we are done in this class the formatter is called:

@set.formatter.generate(named_route, options, recall, PARAMETERIZE)

Here the first thing that is done is to merge the two hashes:

def generate(name, options, path_parameters, parameterize = nil)
  constraints = path_parameters.merge(options)

So whether we delete the key in recall (which becomes path_parameters) the constraints hash will be the same.

You might be thinking "well i bet path_parameters is uesed somewhere else" and you would be right

It's used here:

parameterized_parts = extract_parameterized_parts(route, options, path_parameters, parameterize)

Yet once again, it's immediately merged:

def extract_parameterized_parts(route, options, recall, parameterize = nil)
  parameterized_parts = recall.merge(options)

In both cases we will use the key in options if it exists. I'm pretty comfortable with this change, but I would like some more eyes. Maybe I missed a really subtle and untested use-case, for which we should certainly add some tests.

This comment has been minimized.

@jeremy

jeremy Jul 30, 2015

Member

@tenderlove stubbed some toes on the same thing—ring a bell?

@jeremy

jeremy Jul 30, 2015

Member

@tenderlove stubbed some toes on the same thing—ring a bell?

This comment has been minimized.

@schneems

schneems Jul 30, 2015

Member

Via campfire AP says he's OK with it. If any weirdness comes up on master about URL generation for the next while i'll be happy to take a look.

@schneems

schneems Jul 30, 2015

Member

Via campfire AP says he's OK with it. If any weirdness comes up on master about URL generation for the next while i'll be happy to take a look.

This comment has been minimized.

@mfazekas

mfazekas Apr 6, 2016

Contributor

See #24438

what was overlooked is a few mutations to @recall

@recall[:action] ||= 'index'

@recall[:action] = @options.delete(:action)

this is now going to mutate the recall passed in by callers.

@mfazekas

mfazekas Apr 6, 2016

Contributor

See #24438

what was overlooked is a few mutations to @recall

@recall[:action] ||= 'index'

@recall[:action] = @options.delete(:action)

this is now going to mutate the recall passed in by callers.

end
end
end
@@ -671,12 +675,18 @@ def use_relative_controller!
# Remove leading slashes from controllers
def normalize_controller!
@options[:controller] = controller.sub(%r{^/}, ''.freeze) if controller
if controller
if m = controller.match(/\A\/(?<controller_without_leading_slash>.*)/)
@options[:controller] = m[:controller_without_leading_slash]
else
@options[:controller] = controller
end
end

This comment has been minimized.

@bquorning

bquorning Aug 1, 2015

Contributor

While this may save on String allocations, I suspect the regex matching will actually make it slower. While the readability may be questionable, using start_with? and [] is much faster:

require 'benchmark/ips'

def sub(controller)
  controller.sub(%r{^/}, ''.freeze) if controller
end

def match(controller)
  if controller
    if m = controller.match(/\A\/(?<controller_without_leading_slash>.*)/)
      m[:controller_without_leading_slash]
    else
      controller
    end
  end
end

def start_with(controller)
  if controller
    if controller.start_with?('/'.freeze)
      controller[1..-1]
    else
      controller
    end
  end
end

Benchmark.ips do |x|
  x.report("sub") { sub("no_leading_slash") }
  x.report("match") { match("no_leading_slash") }
  x.report("start_with") { start_with("no_leading_slash") }

  x.compare!
end

Benchmark.ips do |x|
  x.report("sub") { sub("/a_leading_slash") }
  x.report("match") { match("/a_leading_slash") }
  x.report("start_with") { start_with("/a_leading_slash") }

  x.compare!
end
Calculating -------------------------------------
                 sub    59.229k i/100ms
               match    66.314k i/100ms
          start_with    91.586k i/100ms
-------------------------------------------------
                 sub      1.140M (± 9.0%) i/s -      5.686M
               match      1.423M (± 7.1%) i/s -      7.096M
          start_with      3.590M (±10.1%) i/s -     17.768M

Comparison:
          start_with:  3589743.1 i/s
               match:  1423380.7 i/s - 2.52x slower
                 sub:  1140204.8 i/s - 3.15x slower

Calculating -------------------------------------
                 sub    41.670k i/100ms
               match    37.536k i/100ms
          start_with    90.949k i/100ms
-------------------------------------------------
                 sub    653.775k (± 7.6%) i/s -      3.250M
               match    523.276k (±10.9%) i/s -      2.590M
          start_with      2.329M (±12.3%) i/s -     11.460M

Comparison:
          start_with:  2328795.8 i/s
                 sub:   653774.6 i/s - 3.56x slower
               match:   523276.2 i/s - 4.45x slower
@bquorning

bquorning Aug 1, 2015

Contributor

While this may save on String allocations, I suspect the regex matching will actually make it slower. While the readability may be questionable, using start_with? and [] is much faster:

require 'benchmark/ips'

def sub(controller)
  controller.sub(%r{^/}, ''.freeze) if controller
end

def match(controller)
  if controller
    if m = controller.match(/\A\/(?<controller_without_leading_slash>.*)/)
      m[:controller_without_leading_slash]
    else
      controller
    end
  end
end

def start_with(controller)
  if controller
    if controller.start_with?('/'.freeze)
      controller[1..-1]
    else
      controller
    end
  end
end

Benchmark.ips do |x|
  x.report("sub") { sub("no_leading_slash") }
  x.report("match") { match("no_leading_slash") }
  x.report("start_with") { start_with("no_leading_slash") }

  x.compare!
end

Benchmark.ips do |x|
  x.report("sub") { sub("/a_leading_slash") }
  x.report("match") { match("/a_leading_slash") }
  x.report("start_with") { start_with("/a_leading_slash") }

  x.compare!
end
Calculating -------------------------------------
                 sub    59.229k i/100ms
               match    66.314k i/100ms
          start_with    91.586k i/100ms
-------------------------------------------------
                 sub      1.140M (± 9.0%) i/s -      5.686M
               match      1.423M (± 7.1%) i/s -      7.096M
          start_with      3.590M (±10.1%) i/s -     17.768M

Comparison:
          start_with:  3589743.1 i/s
               match:  1423380.7 i/s - 2.52x slower
                 sub:  1140204.8 i/s - 3.15x slower

Calculating -------------------------------------
                 sub    41.670k i/100ms
               match    37.536k i/100ms
          start_with    90.949k i/100ms
-------------------------------------------------
                 sub    653.775k (± 7.6%) i/s -      3.250M
               match    523.276k (±10.9%) i/s -      2.590M
          start_with      2.329M (±12.3%) i/s -     11.460M

Comparison:
          start_with:  2328795.8 i/s
                 sub:   653774.6 i/s - 3.56x slower
               match:   523276.2 i/s - 4.45x slower
end
# Move 'index' action from options to recall
def normalize_action!
if @options[:action] == 'index'
if @options[:action] == 'index'.freeze

This comment has been minimized.

@Johnius

Johnius Aug 6, 2015

Maybe it's a stupid question. But why do you freeze strings?

@Johnius

Johnius Aug 6, 2015

Maybe it's a stupid question. But why do you freeze strings?

This comment has been minimized.

@schneems

schneems Aug 6, 2015

Member

This explains the freeze method for strings 
http://tmm1.net/ruby21-fstrings/

This article explains why you might want I to use frozen strings for performance with some benchmarks http://www.sitepoint.com/unraveling-string-key-performance-ruby-2-2/

@schneems

schneems Aug 6, 2015

Member

This explains the freeze method for strings 
http://tmm1.net/ruby21-fstrings/

This article explains why you might want I to use frozen strings for performance with some benchmarks http://www.sitepoint.com/unraveling-string-key-performance-ruby-2-2/

This comment has been minimized.

@dmitry

dmitry Aug 7, 2015

Contributor

Isn't it better to have a constant defined in a class/module?

@dmitry

dmitry Aug 7, 2015

Contributor

Isn't it better to have a constant defined in a class/module?

This comment has been minimized.

@schneems
@schneems
@recall[:action] = @options.delete(:action)
end
end
@@ -60,7 +60,7 @@ def javascript_include_tag(*sources)
tag_options = {
"src" => path_to_javascript(source, path_options)
}.merge!(options)
content_tag(:script, "", tag_options)
content_tag("script".freeze, "", tag_options)
}.join("\n").html_safe
end
@@ -681,7 +681,7 @@ def time_tag(date_or_time, *args, &block)
content = args.first || I18n.l(date_or_time, :format => format)
datetime = date_or_time.acts_like?(:time) ? date_or_time.xmlschema : date_or_time.iso8601
content_tag(:time, content, options.reverse_merge(:datetime => datetime), &block)
content_tag("time".freeze, content, options.reverse_merge(:datetime => datetime), &block)
end
end
@@ -809,7 +809,7 @@ def select_month
1.upto(12) do |month_number|
options = { :value => month_number }
options[:selected] = "selected" if month == month_number
month_options << content_tag(:option, month_name(month_number), options) + "\n"
month_options << content_tag("option".freeze, month_name(month_number), options) + "\n"
end
build_select(:month, month_options.join)
end
@@ -971,7 +971,7 @@ def build_options(selected, options = {})
tag_options[:selected] = "selected" if selected == i
text = options[:use_two_digit_numbers] ? sprintf("%02d", i) : value
text = options[:ampm] ? AMPM_TRANSLATION[i] : text
select_options << content_tag(:option, text, tag_options)
select_options << content_tag("option".freeze, text, tag_options)
end
(select_options.join("\n") + "\n").html_safe
@@ -991,11 +991,11 @@ def build_select(type, select_options_as_html)
select_options[:class] = [select_options[:class], type].compact.join(' ') if @options[:with_css_classes]
select_html = "\n"
select_html << content_tag(:option, '', :value => '') + "\n" if @options[:include_blank]
select_html << content_tag("option".freeze, '', :value => '') + "\n" if @options[:include_blank]
select_html << prompt_option_tag(type, @options[:prompt]) + "\n" if @options[:prompt]
select_html << select_options_as_html
(content_tag(:select, select_html.html_safe, select_options) + "\n").html_safe
(content_tag("select".freeze, select_html.html_safe, select_options) + "\n").html_safe
end
# Builds a prompt option tag with supplied options or from default options.
@@ -1012,7 +1012,7 @@ def prompt_option_tag(type, options)
I18n.translate(:"datetime.prompts.#{type}", :locale => @options[:locale])
end
prompt ? content_tag(:option, prompt, :value => '') : ''
prompt ? content_tag("option".freeze, prompt, :value => '') : ''
end
# Builds hidden input tag for date part and value.
@@ -456,7 +456,7 @@ def option_groups_from_collection_for_select(collection, group_method, group_lab
option_tags = options_from_collection_for_select(
group.send(group_method), option_key_method, option_value_method, selected_key)
content_tag(:optgroup, option_tags, label: group.send(group_label_method))
content_tag("optgroup".freeze, option_tags, label: group.send(group_label_method))
end.join.html_safe
end
@@ -528,7 +528,7 @@ def grouped_options_for_select(grouped_options, selected_key = nil, options = {}
body = "".html_safe
if prompt
body.safe_concat content_tag(:option, prompt_text(prompt), value: "")
body.safe_concat content_tag("option".freeze, prompt_text(prompt), value: "")
end
grouped_options.each do |container|
@@ -541,7 +541,7 @@ def grouped_options_for_select(grouped_options, selected_key = nil, options = {}
end
html_attributes = { label: label }.merge!(html_attributes)
body.safe_concat content_tag(:optgroup, options_for_select(container, selected_key), html_attributes)
body.safe_concat content_tag("optgroup".freeze, options_for_select(container, selected_key), html_attributes)
end
body
@@ -577,7 +577,7 @@ def time_zone_options_for_select(selected = nil, priority_zones = nil, model = :
end
zone_options.safe_concat options_for_select(convert_zones[priority_zones], selected)
zone_options.safe_concat content_tag(:option, '-------------', value: '', disabled: true)
zone_options.safe_concat content_tag("option".freeze, '-------------', value: '', disabled: true)
zone_options.safe_concat "\n"
zones = zones - priority_zones
@@ -140,15 +140,15 @@ def select_tag(name, option_tags = nil, options = {})
end
if include_blank
option_tags = content_tag(:option, include_blank, value: '').safe_concat(option_tags)
option_tags = content_tag("option".freeze, include_blank, value: '').safe_concat(option_tags)
end
end
if prompt = options.delete(:prompt)
option_tags = content_tag(:option, prompt, value: '').safe_concat(option_tags)
option_tags = content_tag("option".freeze, prompt, value: '').safe_concat(option_tags)
end
content_tag :select, option_tags, { "name" => html_name, "id" => sanitize_to_id(name) }.update(options.stringify_keys)
content_tag "select".freeze, option_tags, { "name" => html_name, "id" => sanitize_to_id(name) }.update(options.stringify_keys)
end
# Creates a standard text field; use these text fields to input smaller chunks of text like a username
@@ -568,7 +568,7 @@ def image_submit_tag(source, options = {})
# # => <fieldset class="format"><p><input id="name" name="name" type="text" /></p></fieldset>
def field_set_tag(legend = nil, options = nil, &block)
output = tag(:fieldset, options, true)
output.safe_concat(content_tag(:legend, legend)) unless legend.blank?
output.safe_concat(content_tag("legend".freeze, legend)) unless legend.blank?
output.concat(capture(&block)) if block_given?
output.safe_concat("</fieldset>")
end
@@ -47,8 +47,8 @@ def escape_javascript(javascript)
# tag.
#
# javascript_tag "alert('All is good')", defer: 'defer'
#
# Returns:
#
# Returns:
# <script defer="defer">
# //<![CDATA[
# alert('All is good')
@@ -70,7 +70,7 @@ def javascript_tag(content_or_options_with_block = nil, html_options = {}, &bloc
content_or_options_with_block
end
content_tag(:script, javascript_cdata_section(content), html_options)
content_tag("script".freeze, javascript_cdata_section(content), html_options)
end
def javascript_cdata_section(content) #:nodoc:
@@ -22,9 +22,10 @@ module TagHelper
TAG_PREFIXES = ['aria', 'data', :aria, :data].to_set
PRE_CONTENT_STRINGS = {
:textarea => "\n"
}
PRE_CONTENT_STRINGS = Hash.new { "".freeze }
PRE_CONTENT_STRINGS[:textarea] = "\n"
PRE_CONTENT_STRINGS["textarea"] = "\n"
# Returns an empty HTML tag of type +name+ which by default is XHTML
# compliant. Set +open+ to true to create an open tag compatible
@@ -143,24 +144,25 @@ def escape_once(html)
def content_tag_string(name, content, options, escape = true)
tag_options = tag_options(options, escape) if options
content = ERB::Util.unwrapped_html_escape(content) if escape
"<#{name}#{tag_options}>#{PRE_CONTENT_STRINGS[name.to_sym]}#{content}</#{name}>".html_safe
"<#{name}#{tag_options}>#{PRE_CONTENT_STRINGS[name]}#{content}</#{name}>".html_safe
end
def tag_options(options, escape = true)
return if options.blank?
attrs = []
output = ""
sep = " ".freeze
options.each_pair do |key, value|
if TAG_PREFIXES.include?(key) && value.is_a?(Hash)
value.each_pair do |k, v|
attrs << prefix_tag_option(key, k, v, escape)
output << sep + prefix_tag_option(key, k, v, escape)

This comment has been minimized.

@bquorning

bquorning Aug 1, 2015

Contributor

I was of the understanding that sep + prefix_tag_option(key, k, v, escape) would allocate a new string object. This could be avoided by shoveling twice instead of once:

output << sep
output << prefix_tag_option(key, k, v, escape)
@bquorning

bquorning Aug 1, 2015

Contributor

I was of the understanding that sep + prefix_tag_option(key, k, v, escape) would allocate a new string object. This could be avoided by shoveling twice instead of once:

output << sep
output << prefix_tag_option(key, k, v, escape)

This comment has been minimized.

@schneems

schneems Aug 2, 2015

Member

Looks like it's faster

require 'benchmark/ips'

sep    = " ".freeze

Benchmark.ips do |x|
  x.report("string +") {
    output = ""
    sep    = " ".freeze
    output << sep + "foo"
  }

  x.report("string <<") {
    output = ""
    sep    = " ".freeze
    output << sep
    output << "foo"
  }
  x.report("array") {
    array = []
    array << "foo"
    array.join(" ")
  }
end

gives us

Calculating -------------------------------------
            string +    82.243k i/100ms
           string <<    86.053k i/100ms
               array    60.794k i/100ms
-------------------------------------------------
            string +      2.220M (±10.9%) i/s -     11.021M
           string <<      2.430M (±10.6%) i/s -     12.047M
               array      1.158M (± 8.7%) i/s -      5.775M

Looks like 10% faster. Care to give me a PR and @-mention me?

@schneems

schneems Aug 2, 2015

Member

Looks like it's faster

require 'benchmark/ips'

sep    = " ".freeze

Benchmark.ips do |x|
  x.report("string +") {
    output = ""
    sep    = " ".freeze
    output << sep + "foo"
  }

  x.report("string <<") {
    output = ""
    sep    = " ".freeze
    output << sep
    output << "foo"
  }
  x.report("array") {
    array = []
    array << "foo"
    array.join(" ")
  }
end

gives us

Calculating -------------------------------------
            string +    82.243k i/100ms
           string <<    86.053k i/100ms
               array    60.794k i/100ms
-------------------------------------------------
            string +      2.220M (±10.9%) i/s -     11.021M
           string <<      2.430M (±10.6%) i/s -     12.047M
               array      1.158M (± 8.7%) i/s -      5.775M

Looks like 10% faster. Care to give me a PR and @-mention me?

end
elsif BOOLEAN_ATTRIBUTES.include?(key)
attrs << boolean_tag_option(key) if value
output << sep + boolean_tag_option(key) if value
elsif !value.nil?
attrs << tag_option(key, value, escape)
output << sep + tag_option(key, value, escape)
end
end
" #{attrs * ' '}" unless attrs.empty?
output unless output.empty?
end
def prefix_tag_option(prefix, key, value, escape)
@@ -177,7 +179,7 @@ def boolean_tag_option(key)
def tag_option(key, value, escape)
if value.is_a?(Array)
value = escape ? safe_join(value, " ") : value.join(" ")
value = escape ? safe_join(value, " ".freeze) : value.join(" ".freeze)
else
value = escape ? ERB::Util.unwrapped_html_escape(value) : value
end
@@ -184,9 +184,9 @@ def link_to(name = nil, options = nil, html_options = nil, &block)
html_options = convert_options_to_data_attributes(options, html_options)
url = url_for(options)
html_options['href'] ||= url
html_options["href".freeze] ||= url
content_tag(:a, name || url, html_options, &block)
content_tag("a".freeze, name || url, html_options, &block)
end
# Generates a form containing a single button that submits to the URL created
@@ -471,7 +471,7 @@ def mail_to(email_address, name = nil, html_options = {}, &block)
encoded_email_address = ERB::Util.url_encode(email_address).gsub("%40", "@")
html_options["href"] = "mailto:#{encoded_email_address}#{extras}"
content_tag(:a, name || email_address, html_options, &block)
content_tag("a".freeze, name || email_address, html_options, &block)
end
# True if the current request URI was generated by the given +options+.
@@ -520,7 +520,7 @@ def retrieve_template_keys
def retrieve_variable(path, as)
variable = as || begin
base = path[-1] == "/" ? "" : File.basename(path)
base = path[-1] == "/".freeze ? "".freeze : File.basename(path)
raise_invalid_identifier(path) unless base =~ /\A_?(.*)(?:\.\w+)*\z/
$1.to_sym
end
Oops, something went wrong.